なぜそのコードは読むのが苦痛だったか？

はじめに

こんばんは。僕は今、フロントがVB、サーバがJava1.4、バージョン管理ツールがCVS（ただし管理しているとは言っていない）という、レガシーと言っても怒られはしなそうなシステムの保守チームにおります。

ユーザの問い合わせを受けたヘルプデスクの方から依頼を受けて、調査のためにコードを読む、ということも少なくありません。今回は「こういう操作をしたらこんなエラーが出たんだけどなんで？」という問い合わせに始まり、ちょっと珍しくVBのコードを読むことになりました。

VB経験はほぼゼロなのでとりあえずコードを見てみたものの、あまりにも「言語がどうとかいう話じゃなくツライ…」という状況でした。しかしただ愚痴って終わってはどこかの誰かさんに生産性がないと怒られそうですし、「こうするとわかりにくいんだな」と自戒を含めた再確認にも役立ちそうなので、今回の記事を書いてみたいと思います。

ひとつひとつは大したこと無く、自分でもやっちゃうこともあるようなものですが、重なると可読性も保守性もとんでもないことになるんだと改めて実感しました。

前提

• コードはJavaに置き換えて例とさせていただきます。
• 業務仕様やデータ構造や言語仕様にあまり関わらない部分を主に扱います。イメージとして読んでいただければ幸いです。

その1.状況面の不穏さ

正となる情報が無い

要件定義書や設計書やテストコードといったものは無く、問い合わせをしてきたユーザさんも、問い合わせを受けたヘルプデスクの方も、現場のメンバも、誰も「何が正しい動きなのか」知りません。エラーが出たことが正しいのかどうかすら誰もわかりません。なので、本当に純粋に「なぜそのエラーが出たのか」を探ることになります。（この時点では、わりかし楽そうな印象でした）

実行環境が無い

アプリのログで役立ちそうなものは出力されておらず、Visual Studioのようなデバッグができる環境もありません。環境は、申請して許可が降りると使用できるらしいのですが、僕は社内環境のユーザIDを申請してから得るまでに3週間かかった記憶を思い出し、ひとまずはテキストエディタでソースコードを開いて読むことにします。

なんだか嫌な予感がする噂

初期開発は10年ほどは前らしく、当時は大炎上も大炎上したらしいという噂を聞きます。このソースを書いた人はおろか、初期開発に関わった人は全員離任しているとも聞きます。うーん、嫌な予感がします。

その2.ソースコード第一印象の不安感

メソッドが長い

メインで読まなくてはならなそうなメソッドは、それぞれ約600行、約200行。ま、まぁメソッド作るの禁止されてて数千行のメソッドが乱立している現場にいたこともある僕にとってはまだまだ短い部類だ…！（もっとも、数千行現場はコメントやドキュメントがめちゃ充実していたし、きっちりデータ構造を理解しているメンバも多かった）

ネストが12重になっている

中央部は左がスッカスカでした。ネストは特に理由なければ浅い方が好きですし、どうしても12重にしなければならない理由は思いつきませんでした……。

その3.素敵な変数たちのお出迎え

次々に宣言される何かのフラグ

boolean ariFlg;
boolean flg;
boolean checkFlg;
boolean okFlg;

多少変数名が長くなっても、何のためのフラグでtrueになるのはどういうときかイメージしやすい方が読みやすいと思います。せめてコメントを付してほしいです。ちなみにどこでも使われていないフラグもありました。

やたら否定やorやandと戯れる何かのフラグ

if (!flg || okFlg && !checkflg) {
    if (!ariFlg) {
    } 
}

もちろん、実際には12重ネスト＆600行メソッドなので、もっともっと複雑です。個人的には、むやみやたらに否定を使ったり、別々の意図で使っているフラグをあっちでもこっちでも組み合わせて判定するのは頭が非常に混乱するので好きではありません。

何か意図がありそうなループカウンタ

for (int k = 0; k < list.size(); k++) {
    for (int h = 0; h < list.size(); h++) {
        for (int g = 0; g < keys.size(); g++) {
        }
    }
}

ループカウンタがアルファベット順でもなく、かといって何かの頭文字というわけでもなさそうで、しかも終了条件が被ってたり被ってなかったりする。

いかにもループカウンタっぽい何か

int ii;
for (int i = 0; i < list.size(); i++) {
    if (条件式) {
        ii = i;
        map.put(ii, i);
    }
}

iとii…ネストしたループ処理かと思いきや、mapのkey/valueがiiとiらしい。何なん…だ？

何が入るのかわかりやすい親切命名

int mainasuIchi = -1;

まさかローマ字読みで「まいなすいち」だと思わず、パッと見た時何かの英単語かと思いました。定数じゃないしこれ-2とか-3も入るんじゃないだろうな、と不安になりましたが、結果それはなかったです。最期まで-1でした。

何が入るのかわかりやすい親切命名（入るとは言ってない）

arrLst.put(key, value);  // arrLstはHashMap型。もちろん実際にはこのコメント無し。

ひとつのメソッドで気にし無くてはならない変数は少ない方がわかりやすいと思います。ただでさえ何百行もあるメソッドで仕様書や設計書やデバッグ環境がない状態でこれは頭が追いつかないです…。スーパーエンジニアはこんな罠なんて屁でもないんでしょうか。

定番のマジックナンバー

int[] checkNum = {0, 1, 2, 3};
int[] kubun = {1, 2, 3};

むしろよく見知ったものに出会えて嬉しいくらいです。

スペルミス

String maccingMsg;

多分何かが何かとマッチングしたらここにメッセージを入れるんでしょう。スペルミスは誰しもやってしまうことがあるでしょう。ただし、様々な混乱変数を見た後でこれに出会うと、「これ単にスペルミスしたんじゃなくて、matchingMsgもいるんじゃないか？　その上でもう一種類つくるためにこれを作ったんじゃないか？」と不安になって、matchでソースコードを検索することになります。

その4.不安を煽るコメント

ずば抜けてふわっとしているもの

// 0とした
// さらにやる
// エラーだよ
// 確認する
// カウントしておく
// チェック

なぜ？　何を？　といったことがわからないので、次のネストに潜るのが怖いです。もちろん、コンパクトなメソッドだったり、クラスやメソッドの作り的にこれをチェックするんだなという想像がつきやすかったり、自分用のツールだったり、こういったコメントもアリな場面もあると思います。ただ、リリース物であり、この先何年も保守をすることがわかりきっている複雑で大規模でメンバも入れ替わるようなシステムでは避けるのが無難と思います。炎上していて「とにかく動かさなきゃ」と焦る経験は僕もありますが、少しコメントや変数名に気をつけるとバグったときに救われたりして結果的に燃え広がらないことにも繋がります。

ウソかどうかを真っ先に確かめたくなるもの

// 変数aが10で、変数bが20の時処理する
if (a == 10) {
    // 処理
}

変数bの判定を行っていません。処理の中でまだまだネストは続いているので後で行うのでしょうか。それともここに到達しているってことは変数bは20なんでしょうか。コメントが間違っているんでしょうか。コメント通り実装できていなくて、変数bのチェックをしていないというバグが潜んでいるんでしょうか……。

一瞬考えさせられるもの

// OKが一つならOK

わかります、わかりますよ。多分何かをチェックしてOKとなるものが一つならチェックOKなんでしょう。間違ってません。間違ってません。

ウソではないけどモヤっとするもの

for (条件式) {
    // flgがtrueなら処理実施
    if (!flg) {
        continue;
    }

    // 処理
} 

「flgがfalseなら、!で否定になるのでtrueになってcontinueで、trueのときは!flgがfalseだからif文には入らずに処理実行、つまりコメントは間違って無い……よね？」と悶々と考え込むことになります。これがネスト、ネスト、ネスト、、、、だと、頭がいっぱいいっぱいになって、すべてを投げ出して草原のハンモックでお昼寝したくなります。

おわりに

昔一瞬参加したプロジェクトで、「ミスは担当者が当時ベストを尽くした結果である。それを責めることはしないこと。」といったことがwikiに書いてあって、すごく良いなと思ったことがあります。僕は最初に入った会社がしっかり自社で研修をしてくれるところ、かつ自社製品や受託案件を内製するのが中心でした。しかし、もし研修もロクにやってくれない企業でいきなり客先に飛ばされてフォローもレビューもなく……だったら、もっとひどいコードを書いていたかもしれないし、とっくにITとは無縁の仕事をしていたかもしれません。読んでいるときは苦痛でしかたなかったですが、今回こういったコードに出会ったことで記事が一つかけましたし、自分なりに可読性を気にかけようと改めて思うきっかけにもなりました。生産的にいきましょう！