ネストは悪ではない話

言語

PHPの話。

発端

リーダブルコード ―より良いコードを書くためのシンプルで実践的なテクニック (Theory in practice)

前に社内LTしたときに、

ネストを浅くしよう

っていって、必要ないなら即returnして、残りの部分への意識的負荷を減らしましょう、みたいなことを言った。

よかったこと

開発チームの中で、ここでreturnすればネストを減らせる、みたいなレビューがよく出た。

よくなかったこと

「ネストを減らす」→「よいことだ」という刷り込みのようなことが起こってしまった。
本当なら、
「可読性を良くする」→「ネストを減らす」という順番だったはずなのに、手段と目的が入れ替わってしまった。

何が起きたか

配列内の特定の要素にだけ処理をしたい、というコードがこのようになっていた。

<?php

$books = array(
    array('id' => 1, 'color' => 'red',    'price' => 1000),
    array('id' => 2, 'color' => 'blue',   'price' =>  400),
    array('id' => 3, 'color' => 'yellow', 'price' => 1200),
    array('id' => 4, 'color' => 'black',  'price' => 1800),
    array('id' => 5, 'color' => 'white',  'price' =>  900),
);

// id が 5 の book を100円値上げしたい！

foreach ($books as &$book) {
    if ($book['id'] != 5) {
        continue;
    }

    $book['price'] += 100;
}

この例では、idが5のときだけ、priceに100を足し込むことをしたかった。
確かに、これでも期待するように動く。

足し込む処理のネストを入れないことで、「ネストを減らす」という「目的」が達成されている。

何が良くなかったか

foreachのブロックの一段浅いネストで、「priceに100を足す」とされていて、すべての要素に処理されるかのような誤解を招く。

どうすべきか

ループのブロックで行われる一番浅いネストでは、要素の全てに適用するような処理を書くべきだった。

逆に特定の要素にのみ当てたい処理は、ifでネストして「特別感」を出すべき。

foreach (array_expression as $value) {
    // このレベルでは すべての $value に適用することを書く

    if (is_true($value)) { // $value が何かの条件であれば
        // ここに特別なことを書く
    }
}

上記の例だとこう。

<?php

foreach ($books as &$book) {
    if ($book['id'] == 5) {
        $book['price'] += 100;
        break;
    }
}

あと、booksの中の要素の並びは基本的に不規則なので、
目的の要素でなければcontinue、よりも
目的の要素を見つけたらbreak、の方が余計なことをしない。

そもそも

books.detect{|book| book[:id] == 1}[:price] += 100

フィルタリングしてから処理、という方が正しい（分かりやすい）スタンスであるとも思うが、
PHPでこれをどうやればいいのかよく分からない…。