Edited at

複雑な if-else を解消するちょっとした心がけ


背景

コーディング初心者のコードレビューをしていると、しばしば if-else の深いネストが散見されます。SonarQube などのメトリクスツールでも「cyclomatic complexity が高すぎる」と怒られるやつです。

条件分岐を徹底的に減らしていこうとすると Strategy パターンやら State パターンといったデザインパターンの話がよく出てきますが、そこまでいかずとも、ちょっとした心がけでコードが見やすくなるよ、というお話です。初心者向けです。


以下は商品が購入可能かどうかを判定するコードです。

public boolean canPurchase() {

boolean result;

// ログインしているか
if (isLoggedIn()) {
// 在庫があるか
if (existsStock()) {
// 代引きの場合
if (isPaymentMethodCash()) {
result = true;
// クレジット払いの場合
} else {
// クレジットカードが登録されているか
if (isCreditCardRegistered()) {
result = true;
} else {
result = false;
}
}
} else {
result = false;
}
} else {
result = false;
}

return result;
}

典型的な、ネストが深くなってしまった例で、非常に処理を追いにくいコードになっています。不慣れな人のコードを見ていると以下のように、ついつい正常系のワークフローを意識してしまっている気がします。

if (1_正常系) {

if (2_正常系) {
...
...
... // 長い処理
...
...
} else {
// 2_異常系
return null;
}
} else {
// 1_異常系
throw new SomeException();
}

しかし、ログインしていない、在庫がないといった「特殊な」ケースにおいては、他の条件(支払い方法やカード登録の有無)はどうでも良く、無条件で返却値が決まります(あるいは、例外が発生します)。よってこれらのケースは、他のロジックから分離してあげましょう。

まずは、ログイン条件を分離してあげます。

public boolean canPurchase() {

// ログインしていなければ無条件で false
if (!isLoggedIn()) {
return false;
}

// 在庫があるか
if (existsStock()) {
// 代引きの場合
if (isPaymentMethodCash()) {
return true;
// クレジット払いの場合
} else {
// クレジットカードが登録されているか
if (isCreditCardRegistered()) {
return true;
} else {
return false;
}
}
} else {
return false;
}
}

最初のコードではローカル変数 result に真偽値を詰めて最後に return result していましたが、今回は直ちに真偽値を return しています。このように一時変数を使わず、返り値が確定した時点で返却する方法を早期リターンと呼ぶようです。

また上記のコードのように、冒頭で特殊ケースの早期リターンを行い、以降はその特殊ケースを対象外とする方法をガード節と呼びます。こうすれば else 句を使わずに済み、ネストが解消されます。

在庫条件も分離してみましょう。

public boolean canPurchase() {

// ログインしてなければ無条件で false
if (!isLoggedIn()) {
return false;
}

// 在庫がなければ無条件で false
if (!existsStock()) {
return false;
}

// 代引きの場合
if (isPaymentMethodCash()) {
return true;
// クレジット払いの場合
} else {
// クレジットカードが登録されているか
if (isCreditCardRegistered()) {
return true;
} else {
return false;
}
}
}

ネストがだいぶ解消され、処理を追いやすくなったと思います。ちょっとした労力で相当コードの見通しが良くなるコスパのよい方法なので、ぜひ心がけてみてください。

※ ガード節の適用範囲

ここでさらに踏み込むと、代引きの場合は必ず true になっているので、ここにもガード節を適用するとさらにネストを減らせます。

public boolean canPurchase() {

// ログインしてなければ無条件で false
if (!isLoggedIn()) {
return false;
}

// 在庫がなければ無条件で false
if (!existsStock()) {
return false;
}

// 代引きの場合無条件で true
if (isPaymentMethodCash()) {
return true;
}

// クレジットカードが登録されているか
if (isCreditCardRegistered()) {
return true;
} else {
return false;
}
}

ただ、これは好みや方針の問題かもしれませんが、個人的にはやらない方が良いと思います。理由としては、ガード節はあくまで特殊な例をあらかじめ弾くものであり、代引きかクレジットカード払いか、はどちらも正常ケースにおける一般的な条件だからです。対等な条件であれば、if-else で並列に並べるべきだと思います。また以下のように、ガード節の順番が変わると問題が生じてしまいます。

/** まずい例。ログインしてなくても代引きなら true が返る **/

public boolean canPurchase() {

// 代引きの場合無条件で true
if (isPaymentMethodCash()) {
return true;
}

// ログインしてなければ無条件で false
if (!isLoggedIn()) {
return false;
}

// 在庫がなければ無条件で false
if (!existsStock()) {
return false;
}

// クレジットカードが登録されているか
if (isCreditCardRegistered()) {
return true;
} else {
return false;
}
}

ガード節は独立して(順番に依存せず)判定できる範囲に留めておくべきかな、と思っています。