コーディングイディオムの奇妙な慣習

先日仕事で緊急に、他人の作ったVisual Basicのコードを改修するタスクを受け持った。

恥ずかしながら、VBは人生初だったが、Visual Studio様とGoogle様のお力でなんとか凌いだ。確かに言語としての敷居は低い。出だしの瞬間的な生産性はかなり高めだ。静止摩擦係数が低いという感じか。静的に型宣言を要求されるため、Java使いには意外と馴染む。ただ、互換性維持のためなのだろうが、同じことを実現するための関数の系統がいくつもあって、どれを選択すべきなのかわからないことが多かった。MSDNを真面目に読めば書いてあるのかもしれないが。

元のコードは典型的な要リファクタリングコードだった。マーチンファウラー先生曰く「臭う」箇所があまりに多くて鼻が麻痺しそうだった。処理はいくつかの関数に分けられていたが基本的に一本道であり、同じ処理がコードのあちこちに何度も出てくる。やたらとグローバル変数が宣言されている。ローカル変数のスコープが無意味に広い。if文のネストが尋常じゃなく深い。そして何よりも、テストコードが一切存在しなかった、いわゆる「レガシーコード」だ。

いろいろ手をつけたかったが、最終的に断念した。期間があまりに短かった上、仕様が毎日どころか数時間毎に変更されていくためだ。修正箇所を極力限定的にし、テストの項目数を極小にしなければ、どう考えても納期に間に合わなかった。直したくて直したくてウズウズする気持ちを抑えて、最小限の修正で実装を終えた。

結果、バグを作りこんでしまった。テストが甘かったといえばそれまでなのだが、初めから自分が書いていれば発生するはずのない類のバグだった。今回はあまりに酷い例だったが、経験上意外とこういうコードを書く人が多く見られるため、自戒も込めてメモを残しておく。


1. ローカル変数の宣言箇所

これは本当に奇妙なのだが、変数の宣言をサブルーチンの先頭にまとめて行うコードを大変よく見かける。正直メリットを一切思いつかない。ひょっとすると、Cのようなガベージコレクタを持たない言語では、領域を確保した後に開放するのを忘れないようにするために、チェックする箇所を限定するという目的があるのかもしれない。しかしほとんどのローカル変数は対象外だし、Javaを含めてGCを持つ言語では百害あって一利もない。典型的な害は以下のような例だ。

String var = null;
for (...) {
    if (...) {
        var = "foo";
    }
    System.out.println(var);
}

for文の中でしか利用しないにも関わらず、先頭で予め宣言したために変数varがfor文の外のスコープを汚染している。もっと問題なのは、変数varの値がループの1つ前の処理に依存していることだ。上記の例の場合、forループの初回はif文の条件に合致した場合、varの値は"foo"になるが、合致しなかった場合は宣言時に代入したnullになる。2回目もif文の条件に合致した場合は同じく"foo"になるが、合致しなかった場合は初回の結果に依存する。

要するにfor文中の条件分岐のいずれかのルートで、varの代入を忘れると嘘の値になるという厄介なバグを作ることになる。今回僕が作りこんだバグはコレだ。正しく動作させるためには以下のようにする必要がある。

String var;
for (...) {
    if (...) {
        var = "foo";
    } else {
        var = "bar";
    }
    System.out.println(var);
}

このような単純な例で見落とす可能性は低いが、このfor文の中の処理が長くなり、分岐が複雑多岐にわたるようになれば徐々に見落としが発生する可能性が増す。また、代入を忘れた場合もなんらかの値が入っているため、たまたま前ループで正しい値が代入されることでテストを通過してしまう可能性が高い。

このような見落としがそもそも発生しないようにすることは簡単で、以下のようにすれば良い。

for (...) {
    String var;
    if (...) {
        var = "foo";
    } else {
        var = "bar";
    }
    System.out.println(var);
}

変数varをfor文の中で宣言するだけだ。変数がforループ中、毎回宣言し直されるため、前回の値が誤って代入されることは絶対にない。昔はループのたびに変数宣言が行われることによるパフォーマンスの劣化を懸念するような意見があったが、ハードウェアのスペックが十分高くなった現在では、パフォーマンス劣化の危険性がバグを作り込む危険性に吊り合わない。そもそも、現在のJava VMでは上記のように都度宣言する記述を行っても、コンパイラの最適化によってforループの外で宣言する形に修正されるため、パフォーマンスの劣化自体が発生しない。そしてコンパイラが自動的に実行する最適化は、人が都度あれこれ気を使って行う最適化よりも概ね正確で効果的だ。変数は常に最小のスコープで、使うときに宣言すべきだ。

2. 無意味な変数初期化

1.で挙げた修正後のコードを、以下のように書く人もよく見かける。

for (...) {
    String var = "bar";
    if (...) {
        var = "foo";
    }
    System.out.println(var);
}

こちらについては、必ずしもマズいとは言い切れ無い。特に上記のような単純な場合はコード量を減らす効果がある。しかし、僕はこのような書き方はあまり好きじゃない（そもそも上記のような単純な例なら、三項演算子を使えという話になってしまうがそれは置いておいて）。なぜなら上記の場合、if文の条件に合致するときに変数varの値は一旦"bar"によって初期化され、その後"foo"という値になる。この動作は非常に奇妙に感じる。もっと極端な例では以下のようなコードも見たことがある。

for (...) {
    String var = "";
    if (...) {
        var = "foo";
    } else {
        var = "bar";
    }
    System.out.println(var);
}

変数varを宣言した際に、「とりあえず」空文字で初期化しておくのだ。この空文字が実際に使われることは一度もない。空文字の代わりにnullでも同じことだ。宣言時に即初期化しろという強迫観念のようなものが存在するのだろうか。

このような記述は、コンパイラが持っている折角のチェック機構をひとつ潰すことになる。初期化を行わなかった場合、例えば以下のように記述すると、コンパイル時にエラーが発生する。

for (...) {
    String var;
    if (...) {
        var = "foo";
    }
    System.out.println(var);
}

なぜなら、if文の条件に合致しなかった場合に変数varの初期化が行われないため、後でvarを参照しようとしたときに困ってしまうからだ。else文でvarに明示的に値を代入しない限り、コンパイルは通らない。

このような単純な例では恩恵が得られないように感じるかもしれないが、分岐が複雑になっていけばこれに救われる可能性が高くなってくる。ある分岐で、うっかりvarへの代入を忘れてしまったとき、とりあえずvarの初期化をしてしまっていると、そのとりあえずの値が採用されてしまい、コンパイル自体は通ってしまう。その分岐ルートをテストしなければ、間違いに気づかない。最悪、とりあえずの値が期待する値とたまたま一致していると、バグに気づかないことになる。宣言時に初期化を行わなければ、すべてのルートで代入が行われていない限りコンパイル自体が通らない。


結局、1と2両方を心がけることで、作りこんでしまったバグをコンパイラが見つけてくれる。これを利用しない手はない。もしコーディング規約等でこれらの奇妙な慣習が義務付けられているのであれば、規約自体の見直しを行う時が来たということになるだろう。