早期リターン(アーリーリターン)とは、リダーブルコードやその他の本で推奨されている書き方です。ガード節でnullチェックや範囲チェックして、想定外の値のときは、すぐにリターンします。else
で続けません。
Code language: plaintext (plaintext)function foo() { if ($a == null) { return 何か; } メインロジック return 何か; }
早期リターンではない書き方です。else
で続けます。
Code language: plaintext (plaintext)function foo() { if ($a == null) { return 何か; } else { メインロジック return 何か; } }
入り口は一つ、出口は一つと習っていると、elseで書きがちですが、ガード節と早期リターンの組み合わせは確かに読みやすいと思います。
さらに、phpmd
という静的解析ツールは「else
を使わないように」と警告してきます!
基本的に
if
文にelse
は必要ないね。if
の条件を書き直してelse
をなくそう、読みやすくなるよ。そのためには、アーリーリターンを使用すること。でも、コードを小さなメソッドに分けなければいけないだろうね。単純な割り当てなら、三項演算子を使うことができるよ。

else
禁止、まじで?

いくらなんでもそれはないわ、何か意図があるはずよ
すると、phpmd
の「else
を使わないように」警告は、オブジェクト指向エクササイズが由来ということがわかりました。
オブジェクト指向エクササイズは、ThoughtWorksアンソロジーという本(マーチンファウラーも著者の一人)の第5章で、Jeff Bay氏が提唱している9つのルールです。
本の「else
禁止」の該当箇所を読むと、
if
〜else
のネストはやめて、簡潔にしよう。
ネストが深くて追跡できない条件文やスクロールしなければ読めないcase
文にはうんざりだよ。
else
句を使わずに書き直す方法を考えてみよう、else
禁止はいい練習になるよ。早期リターン。(早期リターンは使いすぎるとわかりにくくなってしまうから注意してね)
三項演算子。
ポリモーフィズム。
Strategy
パターンやNullObject
パターンも試してみて。
if〜else〜
以外の新しい方法を身に着けよう、そのために強制的にelse
句禁止で書いたらどうかな、ということです。
「何がなんでもelse
禁止、早期リターンしなさい」とは言っていないですよね。
特に注目したいのは、次の一文です。ぼくが書き足したのではなく、本に書いてあります。
早期リターンは使いすぎるとわかりにくくなってしまうから注意してね
早期リターンを使いすぎると、本来のロジックとは違うものになってしまう可能性があります。
次のような if〜elseif〜
があります。
Code language: plaintext (plaintext)if (条件1) { 何か処理 } elseif (条件2) { 何か処理 } else { 何か処理 }
早期リターンを使って、次のように書きかえます。
Code language: plaintext (plaintext)if (条件1) { 何か処理 return; } if (条件2) { 何か処理 return; } 何か処理 return;

読みやすくなった気がするけど、何が悪いのかな?
将来、別の誰かが、if
と if
の間に、別の処理を書けてしまいます!
Code language: plaintext (plaintext)if (条件1) { 何か処理 return; } ★新しい処理★ if (条件2) { 何か処理 return; } ★新しい処理★ 何か処理 return;
条件2が成立するとき、どのような処理をするのか、わかりにくくなってしまいました。
このように一つの if 〜 elseif
と書くべきところを、複数のif
で書いてしまったために起きた重大なバグがありました。
AppleのSSLバグ(CVE-2014-1266)です。goto
バグとして有名であり、wikipediaの「到達不能コード」に例として紹介されています。
Code language: plaintext (plaintext)static OSStatus SSLVerifySignedServerKeyExchange(SSLContext *ctx, bool isRsa, SSLBuffer signedParams, uint8_t *signature, UInt16 signatureLen) { OSStatus err; ... if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) goto fail; if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) goto fail; goto fail; // 常に実行される if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) // このif文が実行されることはない goto fail; ... fail: SSLFreeBuffer(&signedHashes); SSLFreeBuffer(&hashCtx); return err; }
波カッコをつけていたらどうでしょうか。
Code language: plaintext (plaintext)if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; goto fail; }
2つめのgoto fail
は実行されることはなく、バグを防げました。
次のように波カッコをつけていたらどうでしょうか。
Code language: plaintext (plaintext)if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } goto fail;
2つめのgoto fail
が変だと気づき、削除していれば、バグを防げました。
しかし、インデントが変と思って、インデントを修正して、残してしまうかもしれません。この場合は、バグを防げません。
Code language: plaintext (plaintext)if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } goto fail;
つまり、波カッコをつけると、このバグを防ぐ可能性は高いですが、波カッコをつけただけでは、完全に防ぐことはありません。
if 〜 elseif
で続けるとどうでしょうか。
Code language: plaintext (plaintext)if ((err = SSLHashSHA1.update(&hashCtx, &serverRandom)) != 0) { goto fail; } else if ((err = SSLHashSHA1.update(&hashCtx, &signedParams)) != 0) { goto fail; } else if ((err = SSLHashSHA1.final(&hashCtx, &hashOut)) != 0) { goto fail; }
2つめの goto fail
の入り込むすきがありません。
(ロジックの流れだけから判断するなら)このバグは、goto
が原因というよりは、
(1)波カッコをつけていなかった
(2)「if 〜 elseif
」ではなく、「複数の if
」で書いていた
の2点が原因だと考えます。
話しがずれますが、このコードをよく見ると、メインの処理は複数分岐ではなく、逐次処理であることがわかります。
Code language: plaintext (plaintext)SSLHashSHA1.update(&hashCtx, &serverRandom); SSLHashSHA1.update(&hashCtx, &signedParams); SSLHashSHA1.final(&hashCtx, &hashOut);
このメイン処理にエラー処理を追加するとしたら、if 〜 elseif
ではなく、複数のif
で書いたとしてもしかたがないかなと思います。
話しを戻すと、

「複数の if
」で書いてもさ、「if 〜 elseif
」と同じ動きしているなら、読みやすいほうがいいんじゃないの?
同じ動きをしているのは、同じ動きをするように書いているからです。しかし「複数の if
」はたった1行挿入しただけで、「if 〜 elseif
」とは違う動きになってしまいます。
リーダブルコードで推奨しているのは、ガード節と早期リターンです。if〜elseif
で書くべきところを早期リターンで書いてしまうのは、早期リターンを拡大解釈しすぎだと思います。
if〜elseif
で書くべきところを早期リターンのほうがいいかなと思ったら、Apple SSLバグを思い出してください。

多くの人がリーダブルコードを読んでいるのはいいことだけど、経験者まで拡大解釈しているのは困ったことだわ