リーダブルコードをさらに改良する（アンチ・アーリーリターン）

2021.05.04

リーダブルコードで推奨されているアーリーリターン。このアーリーリターンのデメリットについて考え、アーリーリターンを使わない方法で改良します。アーリーリターンに違和感を感じる人は、違和感の答えが見つかるかもしれません。

関数から早く返す

まず、リーダブルコードの説明です。

関数で複数のreturn文を使ってはいけないと思っている人がいる。アホくさ。関数から早く返すのはいいことだ。むしろ望ましいこともある。例えば、

リーダブルコード p91
// コード1：リーダブルコードから引用
public boolean Contains(String str, String substr) {
    if (str == null || substr == null) return false;
    if (substr.equals("")) return true;

    ...
}Code language: Java (java)

このような「ガード節」を使わずに実装するとすごく不自然な実装になる。

リーダブルコード p91

「アホくさ」とズバッと言い切っているので、ガード節とアーリーリターンは正しい！ガード節が自然！ガード節を使わないのは不自然！と思いこんでしまった人も多いでしょう。

以前から、ガード節とアーリーリターンはありました。リーダブルコードが広めたと言っていいと思います。

たしかにガード節とアーリーリターンすることで、メインロジックのネストが減ります。

ところが、このコードには改良の余地があります。

１つめのガード節は、false を返し、２つめのガード節は、trueを返しています。どうもバランスが悪いです。２つめはガード節ではないように思います。

そのバランスの悪さから、この書き方には、行き当たりばったり感を感じるんですね。こんな感じで書いたのかなーって想像してみると、

最初は、メインロジックから書きはじめました。

public boolean Contains(String str, String substr) {
    ...
}Code language: Java (java)

途中で、そういえばnullチェックしなきゃな、と追加し、

public boolean Contains(String str, String substr) {
    if (str == null || substr == null) return false;

    ...
}
Code language: Java (java)

そうそう、substrが空文字列の場合はtrueを返せばいいかな、と追加し、

public boolean Contains(String str, String substr) {
    if (str == null || substr == null) return false;
    if (substr.equals("")) return true;

    ...
}
Code language: Java (java)

この形になった。

つまり、さきほどのガード節とアーリーリターンは、よく考えずに書きながら、あとから思いついて書いたという印象を持ってしまうんです。パッチ的というか。

想像ね

また複数ifは、ifとifの間に関係ないコードを挿入できてしまうことも、敬遠する理由です。

public boolean Contains(String str, String substr) {
    if (str == null || substr == null) return false;
    // ここに何か書ける
    if (substr.equals("")) return true;

    ...
}
Code language: Java (java)

それでは、改良してみましょう。

if 〜 elseif 〜で書きます。

// コード2：if 〜 elseif 〜、中間変数fを使わない
public boolean Contains(String str, String substr) {
    if (str == null || substr == null) {
        return false;
    } else if (substr.equals("")) {
        return true;
    } else {
        return ContainsNormal(str, substr);
    }
}

public boolean ContainsNormal(String str, String substr) {
    ...
}Code language: Java (java)

Containsメソッドが「3分岐があるぞー、余計なことは書くなよー」と言っているようです。

if〜elseif〜を使って、最後にreturnするように書いてみます。

// コード3：if 〜 elseif 〜、最後の一箇所でreturn
public boolean Contains(String str, String substr) {
    bool f;

    if (str == null || substr == null) {
        f = false;
    } else if (substr.equals("")) {
        f = true;
    } else {
        f = ContainsNormal(str, substr);
    }

    return f;
}

public boolean ContainsNormal(String str, String substr) {
    ...
}Code language: Java (java)

リーダブルコードで言われているほど、不自然ではないと思います。最後の return f; にブレークポイントを設定するだけでfの値を見れますし。

不自然ではないね

ネストを浅くする

まず、リーダブルコードの説明です。

以下は、比較的単純な例だ。自分がどの条件ブロックにいるのかを常に確認しているのがわかるだろうか。

リーダブルコード p93
if (user_result == SUCCESS) {
    if (permission_result != SUCCESS) {
        reply.WriteError("error reading permissions");
        reply.Done();
        return;
    }
    reply.WriteErrors("");
} else {
    reply.WriteErrors(user_result);
}
reply.Done();Code language: C# (cs)

最初は単純なコードだった。

リーダブルコード p94
if (user_result == SUCCESS) {
    reply.WriteErrors("");
} else {
    reply.WriteErrors(user_result);
}
reply.Done();Code language: C# (cs)

ここに新しいコードが追加された。

リーダブルコード p94
if (user_result == SUCCESS) {
    if (permission_result != SUCCESS) {
        reply.WriteError("error reading permissions");
        reply.Done();
        return;
    }
    reply.WriteErrors("");
} else {
    reply.WriteErrors(user_result);
}
reply.Done();
Code language: C# (cs)

コードを改善していこう。ネストを削除するには「失敗ケース」をできるだけ早めに関数から返せばいい。

リーダブルコード p95
if (user_result != SUCCESS) {
    reply.WriteErrors(user_result);
    reply.Done();
    return;
}

if (permission_result != SUCCESS) {
    reply.WriteError("error reading permissions");
    reply.Done();
    return;
}

reply.WriteErrors("");
reply.Done();Code language: C# (cs)

一見、きれいで読みやすく見えます。

これをお手本としている人が多いのよね

ところが、このコードには問題が３つあります。

１つ目は、ネストは減りましたが、ロジックがわかりやすくなっていないことです。

ネストの深いコードは理解しにくい。ネストが深くなると、読み手は「精神的スタック」に条件をプッシュしなければならない。

p93

「精神的スタックのpush量」をreply.WriteErrorsに到達するまでのifの回数とします。これを数えると、合計5回です。

if (user_result == SUCCESS) {
    if (permission_result != SUCCESS) {
        reply.WriteError("error reading permissions"); // 2回
        reply.Done();
        return;
    }
    reply.WriteErrors(""); // 2回
} else {
    reply.WriteErrors(user_result); // 1回
}
reply.Done();Code language: C# (cs)

改善後のコードも、合計5回です。

if (user_result != SUCCESS) {
    reply.WriteErrors(user_result); // 1回
    reply.Done();
    return;
}

if (permission_result != SUCCESS) {
    reply.WriteError("error reading permissions"); // 2回
    reply.Done();
    return;
}

reply.WriteErrors(""); // 2回
reply.Done();Code language: C# (cs)

２つ目は、reply.WriteErrors()、reply.Done()を3箇所に書いていることです。無理にまとめる必要はありませんが、このコードでは１箇所にまとめることができそうです。

３つ目は、user_result != SUCESS や permission_result != SUCESS を失敗ケースとしていますが、本当に失敗ケースでしょうか？

それぞれreplyを呼んでいます。そしてメイン処理もreplyを呼んでいるだけで、これといったメイン処理をしていません。この処理のメイン処理は、replyですね。

つまり、user_result != SUCESS や permission_result != SUCESSはメイン処理のreplyを呼んでいるのであって、失敗ケースではありません。これらのifはガード節ではありません。アーリーリターンするのは間違いです。

次のように何もせずにreturnしているなら、ガード節とアーリーリターンです。今回のコードはこのパターンではありません。

if (user_result != SUCCESS) {
    return;
}

if (permission_result != SUCCESS) {
    return;
}

reply.WriteErrors("");
reply.Done();Code language: C# (cs)

３分岐の処理を見ていくと、エラーメッセージだけが違うことに気づきます。

決定表は次のようになります。

#user_resultpermission_resultエラーメッセージ
1== SUCESS== SUCESS""
2↑!= SUCESS"error reading permissions"
3!= SUCESSーーーuser_result

それでは改良していきましょう。

バージョン1、ネストを浅くするために、if〜elseif〜を使います。それぞれの分岐の条件を追跡しやすくするために、条件式を冗長に書きました。

string result;

if (user_result == SUCCESS && permission_result == SUCCESS) {
    // #1
    result = "";
} else if (user_result == SUCCESS && permission_result != SUCCESS) {
    // #2
    result = "error reading permissions";
} else {
    // if (user_result != SUCCESS) {
    // #3
    result = user_result;
}

reply.WriteErrors(result);
reply.Done();Code language: C# (cs)

気持ち的には、#3にもif条件を書いて、elseには「到達しないはず」とコメントしておきたいのですが、lintをかけるとelseに何もないよと警告されたり、lintを黙らせるために「result = "not reachable"」を書くのはやりすぎかもと思ってがまんすることが多いです。

string result;

if (user_result == SUCCESS && permission_result == SUCCESS) {
    // #1
    result = "";
} else if (user_result == SUCCESS && permission_result != SUCCESS) {
    // #2
    result = "error reading permissions";
} else if (user_result != SUCCESS) {
    // #3
    result = user_result;
} else {
    // 到達しないはず
    result = "not reachable";
}

reply.WriteErrors(result);
reply.Done();Code language: C# (cs)

バージョン2、エラーメッセージの決定を別メソッドに分けます。

string getErrorResult(string user_result, string permission_result) {
    if (user_result == SUCCESS && permission_result == SUCCESS) {
        // #1
        return  "";
    } else if (user_result == SUCCESS && permission_result != SUCCESS) {
        // #2
        return "error reading permissions";
    } else {
        // if (user_result != SUCCESS) {
        // #3
        return user_result;
    }
}

result = getErrorResult(user_result, permission_result);
reply.WriteErrors(result);
reply.Done();Code language: C# (cs)

さて、３つの問題はどうなったでしょうか。

１つ目のロジックがわかりやすくなっていない問題、「精神的スタックのpush量」です。#1から#3までに通過するifを数えると、合計5回、元のコードの5回と同じです。しかし、#2は、実質的に、直上のifだけを確認すればいいので、実質1回とカウントすると、合計4回です。

string getErrorResult(string user_result, string permission_result) {
    if (user_result == SUCCESS && permission_result == SUCCESS) {
        // #1
        return  ""; // 1回
    } else if (user_result == SUCCESS && permission_result != SUCCESS) {
        // #2
        return "error reading permissions"; // 2回（実質1回）
    } else {
        // if (user_result != SUCCESS) {
        // #3
        return user_result; // 2回
    }
}
Code language: C# (cs)

カウントの仕方はあやしいけど、決定表と合わせてぐっと読みやすくなったわ。

２つ目のreply.WriteError()、reply.Done()が3箇所にある問題は、エラーメッセージを決める処理と、表示処理を分けることで解決しました。

３つ目のガード節とメインロジックのバランスが悪い問題は、ガード節を使わないことで解決しました。

ループ内部のネストを削除する

まず、リーダブルコードの説明です。

早めに返す技法はいつでも使えるわけではない。ループ内部でネストしたコードを見てみよう。

リーダブルコード p95
for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL) {
        non_null_count++;

        if (results[i]->name != "") {
            cout << "Considering candidate..." << endl;
            ...
        }
    }
}Code language: C++ (cpp)

早めに返すのと同じようなことをループ内部で行うには、continue を使う。

リーダブルコード p95
for (int i = 0; i < results.size(); i++) {
    if (results[i] == NULL) continue;
    non_null_count++;

    if (results[i]->name == "") continue;
    cout << "Considering candidate..." << endl;
    ...
}
Code language: C++ (cpp)

ネストは減りました。しかし、本当に読みやすくなったでしょうか？読みやすくなったとは思えません。

continueはバッドプラクティスだと思うんだけど、世間的には違うのかしら？

１つめの問題は、continueです。

ループ内の return、break、continueはロジックを追跡しにくくなります。returnは△、breakも△、continueは×です。

continueは見慣れないのと、「下へ続く条件」は「continue条件」を反転するので、一番理解しにくいです。

２つめの問題は、ifのネストは減りましたが、if自体は減っていないので、ifの追跡量は減っていないことです。

ネストの深いコードは理解しにくい。ネストが深くなると、読み手は「精神的スタック」に条件をプッシュしなければならない。

p93

元のコードでは、cout << "Considering cadidate..." <<endl; の行に到達する条件を調べるために、2つのifを確認しなければいけませんでした。

改善後のコードも同じく2つのifを確認しなければいけません。

それどころか、暗黙のelseがあるので、continue条件を反転して考えなければいけません。精神的スタックの負担は増えたとも言えます。

ネストは減りましたが、頭の中では暗黙のelseを追加しながら、コードを追跡しているわけです。

for (int i = 0; i < results.size(); i++) {
    if (results[i] == NULL) continue;
    else {
        non_null_count++;

        if (results[i]->name == "") continue;
        else {
           cout << "Considering candidate..." << endl;
           ...
        }
    }
}
Code language: C++ (cpp)

これでは、元のロジックを複雑にしてしまっています。

まずバージョン1です。non_null_countの処理とcout以降の処理を、2つのforループに分けます。

for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL) {
        non_null_count++;
    }
}

for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL) {
        if (results[i]->name != "") {
            cout << "Considering candidate..." << endl;
            ...
        }
    }
}Code language: C++ (cpp)

バージョン1.1です。cout以降の処理が長いのであれば、別メソッドに分けます。

for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL) {
        non_null_count++;
    }
}

for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL && results[i]->name != "") {
        fuga(results[i]);
    }
}

void fuga(Result result) {
    cout << "Considering candidate..." << endl;
    ...
}Code language: C++ (cpp)

null判定をメソッド側に移動してもいいと思います。

for (int i = 0; i < results.size(); i++) {
    if (results[i] != NULL) {
        non_null_count++;
    }
}

for (int i = 0; i < results.size(); i++) {
    fuga(results[i]);
}

void fuga(Result result) {
    if (results == NULL || results->name == "") {
        return;
    }

    cout << "Considering candidate..." << endl;
    ...
}Code language: C++ (cpp)

アンチ・アーリーリターン

アーリーリターンを敬遠する理由です。

アーリーリターンはelseを使いません。

elseを使わないことが不自然で気になってしかたがありません。

if (a == 0) {
    // 処理1
    return;
}

// 処理2
return;Code language: plaintext (plaintext)

なぜ気になるのかというと、

（１）読む人は、ifの後ろ、処理2の前に、頭の中で else を補完していますよね。暗黙のelseというか、読む人の想像力に頼っています。別の言い方をすれば、ここには見えないelseがありますよ、想像してくださいね、と強要しています。

（２）せっかくelseがあるのに、書く人からロジックの表現力を奪っています。elseを使えば自然にelseを表現できることを、不自然に表現せざるをえません。

今、if〜をわざわざ、次のようには書きません。（ブロックのない言語、elseのない言語では、 ifをこのように表現することがあります）

if (a == 0) goto IF_123;
goto END_123;
IF_123:
    // 処理1
END_123:Code language: plaintext (plaintext)

普通に次のように書けるからです。

if (a == 0) {
    // 処理1
}Code language: plaintext (plaintext)

if〜else〜をわざわざ、次のようには書きません。

if (a == 0) goto IF_123;
goto ELSE_123;
IF_123:
    // 処理1
goto END_123;
ELSE_123:
    // 処理2
END_123:Code language: plaintext (plaintext)

普通に次のように書けるからです。

if (a == 0) {
    // 処理1
} else {
    // 処理2
}Code language: plaintext (plaintext)

アーリーリターンの returnは、上の goto END_123; と使い方と似ていると思うんですね。わざわざ、遠回りな表現をしていると思います。

（３）アーリーリターンで「インデントを減らす」ところで満足してしまい、開発者の思考を「ロジックの整理」から遠ざけているように思います。

（４）if〜else〜はそれ自体で１つです。アーリーリターンは、ifで１つ、続く処理で１つ、２つの部分に分かれます。違うものです。

ifの後ろに、想定していなかった「処理Ｘ」を挿入できてしまいます。

if (a == 0) {
    // 処理1
    return;
}

// 処理Ｘ

// 処理2
return;Code language: plaintext (plaintext)

詳しいことは、次の記事に書きました。

早期リターンとApple SLLバグ
早期リターン（アーリーリターン）とは、リダーブルコードやその他の本で推奨されている書き方です。ガード節でnullチェックや範囲チェックして、想定外の値のときは、すぐにリターンします。else で続けません。 function foo() {...
www.ninton.co.jp
2021.04.26

（５）アーリーリターンと普通のifが混在すると、読みにくいです。どちらかが間違っているかもしれないと思ってしまうからです。

次のコードは、a == 0の returnは正しいのでしょうか？また、a == 1 にreturnがないのは正しいのでしょうか？

if (a == 0) {
    // 処理1
    return;
}

if (a == 1) {
    // 処理2
}

// 処理3
return;Code language: plaintext (plaintext)

読みやすさの基本定理
コードは他の人が最短時間で理解できるように書かなければいけない。

p3

アーリーリターンに頼りすぎると、読みやすさの基本定理に反しているように思います。

迷わないように、全てアーリーリターンで書けばいいんじゃないの？

．．．（そういうのを見たことがあるので、少し納得）

たかがアーリーリターン、好きに使っていいんじゃないの？

ガード節とアーリーリターンならいいんだけど、アーリーリターンを使いすぎだと思うわ。

リーダブルコードの「インデントを浅くして読みやすくする」は合っているんだけど、「インデントを浅くするため（のアーリーリターン）」だけが一人歩きしちゃっているのよ。

まとめ

リーダブルコードでは、インデントを浅くして読みやすくする、そのためのアーリーリターン、continueを使う方法を説明していました。

この記事では、

（１）複数ifとアーリーリターンの代わりに、if〜elseif〜 を使う方法。
（２）continueの代わりに、メソッドに分ける方法。

を説明しました。

別の視点からは、ポリモーフィズムを使って改良できるよ、○○を使って．．．と思う人がいるでしょう。

つまり、何が言いたいかというと、読みやすくするための改良に終わりはないってことです。

誤解しないでほしいのですが、リーダブルコードは読みやすく、内容もよく、おすすめの本です。

なお、アーリーリターン派は圧倒的多数です。開発チーム内で、アーリーリターンにしたほうがいいよとレビューが返ってきたら、そうですねと修正してカドをたてないようにしましょう。

タイトルとURLをコピーしました