条件判断の記述方法

質問

いつも下記のような記述でまよってしまいます。
どちらがスマートな書き方なのでしょうか。
もしくはそもそもの書き方がおかしいでしょうか。

[記述方法1]
foreach( string s in k.keys )
{
if( s == "ok" )
goto OK;
}

//NGな処理
return false;

//OKな処理
OK:

return true;

[記述方法2]
bool ok_flg = false;

foreach( string s in k.keys )
{
if( s == "ok" )
{
ok_flg = true;
break;
}
}

if( ok_flg )
{
//OKな処理
}else{
//NGな処理
}

return ok_flg;

回答No.4

anmochi

長文です。#1です。

> そのメソッドのためだけに存在する
> 変な名前の細かいプライベートメソッドだらけになってしまいます。
　これはクラス設計上の問題、つまりどこまでをメソッドに切り分けるべきか、という話なんだけど、実際これはセンスの問題ではないんだよね。あるべきところにあるべきものを配置すればよいだけで。
　つまり、長くなったからメソッドに切り分けしようとして変な名前のメソッドが出来上がる場合、それはメソッドに切り出すべきではないという事。分ける理由として「長いから」は不適切だよね。
　意味、つまり、「処理単位」で切り分けると、そのメソッドにつける名前は「その処理をあらわす言葉」になる。すごく自然な話だよね。長くなったら、C/C++で言うifなどのブロック単位に分けていくのではなく、ブロック内部またはロジック中で一処理として切り出せないかを分析する。そこで分ける理由が無けりゃ(意味的に細分化できなければ)分けるべきではないんだよ。逆に言やたとえ一行のロジックであってもそれが一つの処理をこなすのであれば分けた方がアルゴリズムは見えやすくなる。
int getMax(int x, int y) { return x > y ? x : y; }
なんてその典型例だよね。一行でも、「xとyを比較してxが大きい場合はxを返す、そうでなければyを返す」ではなくて、「xとyの大きい方を返す」という論理的な処理をルーチン化したものだ。
　VBでも、こういうプロシージャを作った事あるんじゃないかな。
Public Sub CenterForm(ByRef f as Form)
f.Left = (Screen.Width - f.Width) / 2
f.Top = (Screen.Height - f.Height) / 2
End Sub
　これも、たった2行なのに分かれているのは、決して「窓の位置を画面の幅-窓の幅を2で割った値にする」という事したいんじゃなくって「窓の位置を画面の中央に」という事をしたいので切り出していて、だからメソッド名がCenterFormになっている訳だ。処理の意味で切り出すという事。

　今回のお題だと、キーの中の何かがOKであればというifブロック内を機械的に切り出した場合、メソッド名はOnValidKeyExist()のような名前になるけど、この名前は「処理の内容」を表していないよね。言うなれば処理の状況を説明する名前だ。これは構造化手法の切り出し方。フローチャートの一部分をサブルーチン化しているという事になる。これだったら、逆に有効がキーがあるかどうかを切り出す方が良いでしょう。
private boolean isValidKeyExist() {
foreach(string s in k.keys) {
if(s == "ok") return true;
}
return false;
}
　こっちを切り出すと、「有効なキーがあるかどうか」という一つの論理的な処理を担当する(すべき)メソッドができあがる。呼び出す側は
if(isValidKeyExist()) {
// OKな処理
} else {
// NGな処理
}
こっちの方がすっきりしないかな？

　この「処理単位の切り出し方」を覚えると、フラグがすごく良い意味を持つようになる。フラグやgotoの悪い側面である、「プログラムの見通しが悪くなる」という事が発生しなくなるんだ。
　フラグだから悪、gotoだから悪っていうんじゃなくて、フラグやgotoが大小の処理間にまたがらない事が重要なんだね。

まとめると、
・意味でプログラムを切り出す(ルーチン化する)事が肝要
・適切に切り出された一つの処理内であればフラグも綺麗な状態遷移を行う
って事です。

お礼

詳細な回答ありがとうございます。
大変参考になります。

>分ける理由として「長いから」は不適切だよね。

そうなんですよ。
いつも切り出しするときに大きな欲求を占めるのが長くなりそうだなーでした。
それが、特にif文やswitch文のブロックの中だと取りあえず外に出しとけって感じになっちゃてますね。
結果、変な名前のプライベートメソッドが変な階層でできてしまいます。

>・意味でプログラムを切り出す(ルーチン化する)事が肝要
>・適切に切り出された一つの処理内であればフラグも綺麗な状態遷移を行う

上記を意識しながらがんばってみたいと思います。

回答No.3

notnot

このケースであれば、goto を使いますね。
一時的変数は出来るだけ使わないに越したことは無いです。
フラグ変数を使う場合は、変数の未定義化が出来る言語ならok_flgを参照した直後で未定義化すればいいのですが、C++ だと、
//これ以降 ok_flg は使わない
とかコメントで書くのかな。

goto文の使用については、
STEP1. goto文を使うことを覚える
STEP2. goto文を使わない書き方を覚える
STEP3. goto文も使うことを覚える
と言われています。

お礼

回答ありがとうございます。
VB暦が長いへぼプログラマーなので
ロジックの中にデータベース処理などが絡んでくると
フラグとgotoでいつも悩んでいます。

回答No.2

noname#31077

gotoラベルを用いる人は嫌われる傾向にあります。
前者はやめましょう。

後者はフラグを用いた分岐処理ですね。
フラグは多用すると可読性を非常に欠くコードになってしまいます。
しかし、今回の使い方は局所的なのでいいと思います。
よって後者が良いと思いますよ。

人にもよりますけど、私ならフラグは用いません。
foreach( string s in k.keys )
{
　　if( s == "ok" )
　　{
　　　　//OKな処理
　　　　return true;
　　}
}
// NGな処理
return false;

とします。
参考になりました。？

補足

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

フラグとはちょっと離れてしまうかもしれませんが、
if( s == "ok" )
{
//OK処理がちょっと長い
　・
　・
}
となることが多く、切り出しを行うと
そのメソッドのためだけに存在する変な名前の細かいプライベートメソッドだらけになってしまいます。

そうなるとラベルを付けてメソッド内で処理してしまいたくなる衝動にかられるんですが、やはり嫌われますよね。

回答No.1

anmochi

これって、1個でもOKな奴があったらOKな処理をしてから呼び出し元に制御を返すの？

じゃあ
foreach( string s in k.keys ) {
if( s == "ok" ) {
//OKな処理
return true;
}
}
//NGな処理　ここにたどり着いたのであればOKが一つも無かったという事
return false;
で良いんじゃないかなぁ。

お礼

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