- 2014/4/28 07:03
前回の「変数名の変更」で説明したコードは、まだリファクタリングの余地があります。
今回は「制御フラグの削除」のリファクタリングをしてみましょう。
●
前回のコード
private bool ContainsTargetValue(int[] values, int targetValue)
{
bool foundTargetValue = false;
for (int i = 0; i < values.Length; i++)
{
if (values[i] == targetValue)
{
foundTargetValue = true;
}
}
return foundTargetValue;
}
このメソッドは「int型配列の中に対象の値が含まれるかを調べる」メソッドで、対象の値が含まれていたら「true」を返します。
ですから「foundTargetValue = true;」実行後はループする必要がありません。それではfor文をbreakしましょう。
●
for文break後のコード
private bool ContainsTargetValue(int[] values, int targetValue)
{
bool foundTargetValue = false;
for (int i = 0; i < values.Length; i++)
{
if (values[i] == targetValue)
{
foundTargetValue = true;
break;
}
}
return foundTargetValue;
}
ここでメソッドをもう一度読み返してみます。
「foundTargetValue」という変数は、初期値が「false」で目的の値が含まれていたら「true」になり、最後return文でその値が返されます。それ以外のことは何もしていません。
だとすると目的の値が含まれていたらreturn文で「true」を返す、含まれていなかったらreturn文で「false」を返すという構成でも問題ないことに気付きます。
そうですこのメソッドでは、そもそも制御フラグが必要なかったのです。それでは制御フラグを削除した形にリファクタリングしてみます。
●
制御フラグ削除後のコード
private bool ContainsTargetValue(int[] values, int targetValue)
{
for (int i = 0; i < values.Length; i++)
{
if (values[i] == targetValue)
{
return true;
}
}
return false;
}
コードがかなり簡潔に表現できたと思います。return文が2つ以上あることを嫌う人はいますが、後々の保守性が向上しますので無駄な制御フラグは必ず削除してください。
コードを保守する人は、コード全体を漠然と追いかけるのではなく「この場合はどう動くのか？」にスコープを当てて追いかけます。適宜return文で復帰してあるとそれ以上「無駄なコードを読まなくてよい」というメリットがあるのです。
私の経験上そのメリットの恩恵は計り知れません。トラブル対応で一分一秒が惜しいときに、整理されたメソッドにどれだけ助けられたことか、逆に意味不明な制御フラグにどれだけ苦しめられたことか。。
全ての制御フラグを削除しろとは言いませんが、明らかに無駄な制御フラグは削除するように心がけましょう。