複雑な条件の書き方

質問

こんにちは。c#初心者です。（今回の質問はcに近い言語ならどれでも大差ないと思います）

　複雑な条件を使って処理を分岐する場合、

if ( A || (B && (C || D || E)) && F) // A～Fは条件
else

　のようになったりすることもあると思いますが、それがさらに複雑化して

if ( value1 < A || (B && ( (value1 = values[index]) == C || D || E)) && (value1 = values[value2]) == F )
else

（ A～Fはフィールド(インスタンス)変数やstaticな変数で、それ以外はローカル ）

　のように代入まで入ってきているようなコードがあった場合は、さら乱雑になるので

bool satisfied(ref int value, int value2, int[] values, int index)
{
　　if ( value1 < A ) return true;
　　if ( !B ) return false;
　　value1 = values[index];
　　if ( value1 != C && !D && !E ) return false;
　　value1 = values[value2];
　　return value1 = F;
}

　のようなメソッドに抽出して

if ( satisfied(ref value1, value2, values, index) ) …
else …

のようにしたほうが良いのでしょうか？

　ちなみにこれらの条件は（初心者が見る限り多分）これ以上細かく分離できず、再利用される場所がありません。

　そのためメソッドにしてしまわず、インデントなどを駆使して

if ( value1 < A ||
　　(
　　　　B &&
　　　　(
　　　　　　(value1 = values[index]) == C
　　　　　　|| D || E
　　　　)
　　)
　　&& (value1 = values[value2]) == F )
else

　のようにするだけでも良いのでしょうか？（何だかやりすぎて、逆に見にくくなった気が…）

　この辺の事はよくわからないので皆さんのご意見を伺わせてください。

回答No.6

kekyo0 · ベストアンサー

最初の方の回答でもありましたが、やっぱりテスト可能性に着目するのが良いと思います。
今のままでは、漏れのないテストを行うのが、（このコード量にして）既に苦痛です。

＃昔は書き方だけでパフォーマンスに影響がありましたが、今やそんな事を気にするのは
＃シェーダプログラムぐらいでしょう。

条件の判定を細かく分割するのは良いアイデアです。最も、分割すべき節は考える必要があります（その判定の意味が明確になるように分割することが重要だと考えます）。
そのように分割し、個々の式の「意味」が正しいかどうかを、ユニットテストでテストするようにします。

テストが通った個々の式を集成したテストを書く頃には、コード全体について安心感が得られるはずです。

既に見ているかもしれませんが、参考までに。

http://www.atmarkit.co.jp/fdotnet/nagile/nagile02/nagile02_03.html

お礼

ご指摘ありがとうございます。

　NAgilerは初めて見ましたが、ためになりました。特にサイバラさんのは最高でした。

　とりあえず、NAgilerは非常に有用だと思うのでこれから実践したいと思います。ありがとうございました。

回答No.8

kmee

ifの条件中に代入式を書けるのは、Cの特徴で、場合によってはすっきりとわかりやすく、また、「いかにもCのプログラム」という感じがあるのですが

例のように複雑になったら、そんなCらしさは捨てた方がいいです。
とりあえずは、別の言語のように「代入は文で、ifの条件式中に書けない」ものとして、条件を整理するのがいいのでは?

お礼

そうですよね。１つのステートメントにインクリメント程度ならまだしも、代入まで、それも複数あると読みにくいですよね。
　日ごろからそんなコードにはならないように心がけてはいるので、自分は問題ないですが、他人さんの書いたコードだけはどうしようもないです。

回答No.7

wormhole

本題とは関係ありませんが、

＞　ユニットテストですか。魅力的ではあるんですけどね。一度はNUnitに手を伸ばそうとしたのですが、
＞
＞「使い方がイマイチ、アセンブリがどうとか」
＞→「ユニットテストが無いならstaticチェッカー(Contract)を使えばいいじゃない」

ユニットテストとstaticチェッカーは用途が違いますよ。
またユニットテストはNUnitなど使えば便利なのは確かですが別に必須というわけではないです。
要は作ったクラスやメソッドが仕様通りに動作するのか確認できればいいだけなんで。

お礼

ご指摘ありがとうございます。

　いえいえ、バグを減らすためのものと、仕様を明記するためのものということは知っていますが、原因不明のバグが発生したときがあって、そういうのを見つけるために使おうかと（結局使えずにひたすらステップインの連続）。
　原因は本来変化するべきタイミングで変数が変化していなかった（ちゃんと連動していなかった）事なのですが、これがまた厄介で、
「正常値 → 不正値」の変化（明示的な不正値への変化）なら、
(バグ要因の変数を変化させている部分だけ)ざっと挙動を追うだけで見つけられるのですが、
「正常範囲 → 別の正常範囲」の変化（暗黙的な不正値への変化）の変化は、何しろバグ要因の値が一切変化していないわけで、探すのに苦労しました。

　この場合、「UnderLimit <= Value && Value <= UpperLimit」が常に満たされているかどうかを確認すればよいので、Invariantな契約を使えばすぐに発見できたんですね（「Invariant」は静的チェッカー使わなくても十分役に立つし、比較的簡単に作れるので属性を駆使して、何とかもどきを作りました）。

　また何かありましたら、ご指摘お願いします。

回答No.5

hidebun

多分、私が書いたサンプルは、あなたのif文を読み違えているように思いますが、
それは難読だから、ということで(^^;)

ともかく、trueになる条件をツラツラと書き連ねて行けば、どんなに条件が増えても、
怖くありません。

お礼

回答ありがとうございます。

> 多分、私が書いたサンプルは、
> あなたのif文を読み違えているように思いますが、

　elseが使われていないので、検索を行う用な判定や、手前の判定でtrueの場合、後ろが例外をスローする場合があったりすればまずいかもしれません。

> ともかく、trueになる条件をツラツラと書き連ねて行けば、
> どんなに条件が増えても、怖くありません。

　うまく使えば便利そうです。ただ、「&(&&)」を使う場合はfalseを返したほうがいい場合もあるかもしれません。

回答No.4

hidebun

判定を関数化するのは、良いアイデアだと思います。
気になるのは、条件に応じて、trueやらfalseやらをバラバラに返そうとしているところです。

if文の判定条件は少し変更するだけでバグになってしまうので、複雑な判定は避けたほうが良いです。
例えば「trueになる条件に着目する」という方針で検討してみては？

１つずつの条件を読むなら簡単で、どういう条件ならsatisfiedなのか？が明確になります
。
bool satisfied(...) {

　bool ret = false; // 初期条件

　// 以下、retをtrueにする条件文だけ書く

　//必要に応じてsatisfied条件を説明する
　if ( value1 < A ) {
　　ret = true;
　}
　//必要に応じてsatisfied条件を説明する
　value1 = values[index]);
　if ( B && (value1 == C) || C || D || E ) {
　　ret = true;
　}
　//必要に応じてsatisfied条件を説明する
　value1 = values[value2];
　if ( value1 == F ) {
　　ret = true;
　}

//上記のsatisfied条件の判定に引っかからなければ、falseが返る
　return ret;
}

無駄な判定を避けたいなら、
ret = true;
return;
としても良いです。

こうしておくと、判定の優先順位の変更が必要になった場合も、入れ替えにより、容易に対応できます。

お礼

回答ありがとうございます。

> 判定を関数化するのは、良いアイデアだと思います。
　人に言われると思いつきでも嬉しいものですね。

> if文の判定条件は少し変更するだけでバグになってしまうので、
> 複雑な判定は避けたほうが良いです。
> 例えば「trueになる条件に着目する」という方針で検討してみては？

　一考の価値はありそうです。参考にさせてもらいます。

回答No.3

jjon-com

質問文に掲載された３例の中では，

　　if ( value1 < A ) return true;
　　if ( !B ) return false;
　　value1 = values[index];
　　if (!(value1 == C || value1 == D || value1 == E)) return false;
　　value1 = values[value2];
　　if ( value1 != F ) return false;
　　return true;

という，真偽が決定したものから次々と振り分けていくコードが一番わかりやすかったです。

メソッドにするか否かは別の問題で，上記コードを例えばbool satisfied;という論理型変数に対する真偽値設定に置き換えても，判定の読みやすさは変わらないでしょう。

お礼

回答ありがとうございます。

> 上記コードを例えばbool satisfied;という論理型変数に対する真偽値設定に
> 置き換えても，判定の読みやすさは変わらないでしょう。

　そんな感じのことをしたかったのですが、最初はgoto使わないと判定なら抜けられないと諦めていたのですが、elseを使えば何とかなりそうですね。

　そうすると

bool statisfied;

if ( value1 < A ) satisfied = true;
else
{
　　if ( !B ) satisfied = false;
　　else
　　{
　　　　value1 = values[index];
　　　　if ( value1 != C && !D && !E ) satisfied = false;
　　　　else
　　　　{
　　　　　　value1 = values[value2];
　　　　　　satisfied = value1 == F;
　　　　}
　　}
}

　のようになるのですが、if, elseが深くなってしまうのが嫌い(elseで階層にするぐらいならreturnで抜けたい)な性分で、if(else)が３つも階層になっていると、こう、なんと言うか、もやもやすると言うか、うずうずするタイプなんです。
　やっぱり何ともならないものなんですかね？

回答No.2

eclipse2maven

条件文は　命題が真の場合と偽の場合の２つにわけ、そのツリー構造で分けていきます。

そうすれば、抜けるケースがなくなります。　　　複雑になってもツリー構造がわかれば、絵がかけます。

また、これが大事ですが、抜ける落ちるケースがなくなります。

きれいに求めようとすると、抜ける落ちるケースが、出て、それのバグ取りに時間がかかってしまうことが一番の問題です。

お礼

回答ありがとうございます。
　ツリー構造ですか。最近は意識したことなかったですね。今後気をつけようと思います。

回答No.1

wormhole

私ならユニットテストのやりやすさも考えてメソッド化します。

お礼

回答ありがとうございます。

　ユニットテストですか。魅力的ではあるんですけどね。一度はNUnitに手を伸ばそうとしたのですが、

「使い方がイマイチ、アセンブリがどうとか」
→「ユニットテストが無いならstaticチェッカー(Contract)を使えばいいじゃない」
→「Expressには対応しておりません」
→「いいもん。自分で似たのを作るもん！（飽くまでお金払わない）」
→「ソースコードを読むためにkm(yacc)」
→「不可解なところでsyntax error」
→「レキサーは異常なし」
→「パーサを解読してやるぜ！」
→「高級言語とは思えぬ可読性」
→「とりあえず変数だけでも読みやすく」
→「とりあえずshift、reduceらしきものをメソッドに抽出」
→「―――見える！！」
→「ifがウルトラカオス」
→「OKWave行こうぜ！」
→「（今）やっぱり諦めようぜ！」

　とりあえず、参考になりました。ありがとうございます。