プログラムで「コイツ大丈夫か？」と思われやすいコード 8選

IT業界の人間ぽい話題

まぁ僕は世の中でシステム・エンジニアと呼ばれる職種なわけなんですけども。
長くこの仕事をやってると、誰しも必ず他人のソースコードに「あれ？このコード大丈夫か？」と思う瞬間があるんだよね。

経験が浅いとか深いとか関係なく、イマイチなコードを作るやつは作る。
世の中、思いの外「動けばいい」と思ってる人多いんだよねぇ。
どうしてそういう価値観になってしまうのか理解できないのだけど。
「研究者じゃなくてシステム開発をお仕事にしているんだから、ちゃんと動いてバグがないのが良い事。設計の質は考えなくていい」ってスタンスのヤツ、冗談じゃなくて結構いるからね。マジで。ビックリするよね。ちゃんと動いてバグが出ないために設計の質を高める必要があるって話なのに、そこが分かってない奴がしたり顔で言ってくるんだぜ？ 信じられねぇ。

プログラムコードはその組織の名刺であり顔であるってことを意識しないでホイホイと低品質なソースコードを「納品物で～す☆」なんて出してくる組織に鉄槌が下されることを期待しつつ、IT業界の人間っぽいグチを書いていこうかなと。
そういうワケで、グチを吐き出しを兼ねて私の基準では「イマイチ」なコードの数々を書き出してみようと思いまする。

私の嫌いなコードの数々

メソッド名がjudge○○

これね、先頭にコレを書きたい。それぐらいにコレはマジでやめた方がいい。
このメソッド名見た瞬間に、そのソースコード全体のクオリティは低いだろうと想像してしまう。
理由は3つ。

1. たぶん「判断」のつもりで judge としているのだろうが、プログラムは常に何かの判断があるのでメソッド名が処理の説明になってない
2. 関数名に judge としかつけられないということは、メソッド内で複数の処理が行われている可能性が高い
3. もっと適切な英単語があるはずで、他のプログラムを見ていれば自然と知識として入ってくるはずなのに、その知識がない

1 について

メソッド名は、パッと見てなんとなく処理が分かるようにつけなきゃいけない。そうでないと間違えやすくなるだけ。
その意識を持たずにコーディングしてるというのがバレてしまうと、全体のクオリティを疑われても仕方がないと思う。

たとえば「電話番号が設定されているか判断する」というメソッドに対して

boolean judgePhoneNumberSetting()

と名前つけるような感じ。
日本語をそのまんま英語にしました、というつもりなんだろうが、コードだけみてこのメソッドが何の処理しているか分かるか？ って話。
全然わからない。PhoneNumber の Setting を judge するって、設定されているかどうかなのか、それとも特定の番号かチェックしているのか、中身を見るまで分からない。

分かりやすく書くなら

boolean isPhoneNumberSet()

でいい。シンプルだし、意味がすぐ分かる。

2 について

そのメソッドで処理する内容を説明するメソッド名を付けよう、という意識はあるのだが、そのメソッドでやっていることは「判断」としか言いようがないから仕方なく judge を使っている、というケース。
なぜ「判断」としか言いようがないかというと、まず間違いなく「複数の処理を1つにまとめている」から。

boolean judgePhoneNumberSetting(int phoneNum) {
    People id = getCallerId();
    boolean ret = false;
    if (id.isEmployee()) {
        //社員ならそのまま登録
        setPhoneNum(phoneNum);
    } else {
        //それ以外は先頭に03を付与
        setPhoneNum("03" + phoneNum);
        //戻り値を true にする
        ret = true;
    }
    return ret;
}

CallerId を取得し、それが社員かどうか判断して値をセットし、さらに社員以外の時だけ true を返す、という処理。
結局呼び出し元からすれば、戻り値の true か false が重要だし、かといって中でやってる他の処理もあるから is○○ って名前にもしづらいし……。
そんな理由から「まぁ判断してるから」ってことで judge に到達するのだと思うけども。
こういう名前付けに悩んだときは、大概処理内容がおかしい。

本来ならこれはメソッドを分けるべき。

boolean isEmployee() {
    getCallerId().isEmployee();
}

void setPhoneNumById(String phoneNum) {
    if (!isEmployee()) {
        phoneNum = "03" + phoneNum;
    }
    setPhoneNum(phoneNum);
}

単純に2つの機能が混ざっているから名前を付けるのに迷ってしまうだけ。

3 について

たとえば is○○ でいいのに judge○○ という名前にしているとか、 should とか IfNeeded とか has○○ とか「ありがちな書き方」ってのがあるのに、それをしないってことは「そういう知識がない」と思われても仕方がない。
当然 judge○○ にするメリットは何もないんだから、知らない＝クオリティが低いと思われてあたりまえ。

メソッド名がcheck○○

これも judge とほぼ同じだけども。
judge と同様に check だって「メソッドの説明にならない」ってのが悪い。
ただ check はたまに見かけるよね。良くないネーミングだとは思うから、避けるべきだと思うけどねぇ。

メソッドに2つ以上の機能を実装している

これも judge で説明したこととかぶってしまうのだけど、メソッドに 2 つ以上の機能を実装しないのが望ましい。
なぜなら名前つけづらくなるから。
名前つけづらくなるってことは、後から読む人に伝わりにくいってこと。ってことはバグが埋め込まれやすいってこと。
ただ「これ分けたら面倒くさすぎる」ってときもあるから、その辺は程度問題。あまりに潔癖すぎても、今度はコーディングが進まないからね。
バランス感覚もってやりましょう。

なんでもかんでも○○Manager

これもまた似たような意味だけど、よく見るんだよなぁ○○Manager クラス。
そこら中になんとかManager がいて、なんとかManagerから値を取得して、なんとかManagerに渡したりしてる。
絶対ダメってわけじゃなくて、節度をもって使わないとダメだろうってことね。
Array だったり List だったり Map だったり Container だったり、そういう単純な名前で代用できるんじゃないの？ ってケースは結構多い気がする。
何でもかんでも Manager にしちゃうと、あっちの Manager とこっちの Manager で使い方が違って混乱するんだよね。
やめてほしい。

1つのutilityクラスが万能

なんかファイルのオープンクローズもするし、GUIのボタン処理の共通処理も実装されてるし、エラーコードとメッセージの取得処理まで入ってる。
そんな何でも屋のutilityクラス作るなって。UML書いたとしたら、そのクラスはどういう扱いになるんだと。
「関連」をクラスにしたり、「このクラスに本来あるべきだけど、面倒だから外に出した」ものがutilityクラスになったりってのなら、全部同じクラスになるってことはないでしょ。
別に static メソッドが憎い、全部インスタンスメソッドでやれ、とかそういうことじゃなくて。
立ち位置の問題として、全部一緒って修正の影響範囲逆にわかりづらくならねぇか？ って思うのだよなぁ。

引数に重要な意味を与えるpublicメソッド

これ、結構する人いるんだよなぁ。嫌いなんだよなぁ、引数が重要な意味もっちゃうコード。
だって呼び出し元のコードまで確認しなきゃいけないじゃん。

たとえば

void someMethod() {
    someone.drink("ビール");
      :
      :
    someone.drink("焼酎");
}

みたいなコード。
この drink メソッドの中で

void drink(String a) {
    if ("焼酎".equals(a)) {
      :
      :
    } else {
      :
      :

とかやり始めるともう最悪。
結局 someMethod が呼び出すパラメータによって drink メソッドが制御されてしまっている。
drinkメソッドを「呼び出している側」が、何をどう飲むか判断する事になってモデルと乖離が生じていく。

このケース、何が嫌いかって検索性が低いのが嫌い。
drink メソッドを grep する瞬間がかなり嫌い。

本来の意味では、この形だとクラスの継承とかで解決できない構造になりやすい。簡単にいえば「オブジェクト指向っぽくない書き方」だから嫌い。
素直にシンプルに、以下のようにした方がいいんじゃねぇのかと思う。

void someMethod() {
    someone.drinkBeer();
      :
      :
    someone.drinkShochu();
}

こうしておくことで、特定人物クラスの drinkBeer() だけ2杯飲みたいとかいうケースで、 someMethod 側に一切修正が入らないで済む。
最初のコード例で、ある人だけビール2杯飲みたいって言われたときにどういう修正するか考えて見て欲しい。

たぶん、こうなるだろう。

void someMethod() {
    someone.drink("ビール", 1);
    someone2.drink("ビール", 2);
}

クッソださい。そして結局オブジェクト指向的じゃない。

コメントが多い

関数にコメントをたくさんつけるのは、可読性を落とすと思うんだよね、俺は。
だってパッと見でわかるように書いておけば、コメントつける必要ないじゃん。
「あぁ、ここでファイル読み込んでるのね」とか分かるじゃん？ そんなところにコメントごてごて付けたら逆に読みづらい。

削除コードがコメントで残ってる

なんでかっていうと、ソース管理ちゃんとしてんの？ って気にさせるから。
ソース管理ツールも使えない人たちのコード、はたしてマトモなんだろうか……？ って思われても仕方ない。

正直、個人的な意見だけども、大きくハズしてないと思う

これらは、一見してすぐに「クソコードだー！」と大騒ぎするような物ではないかもしれない。
実際意外とよく目にするし。

でもこれらは、おそらく多くの人から「大丈夫なのか、コイツ？」と思われがちなコードだと思う。
損だよね、実際はちゃんと考えて設計して試験も十分にしたのだとしても、ファーストインプレッションで「大丈夫か？」なんて疑われたら。

ちょっと名前に気をつける、ちょっとオブジェクト指向設計に気をつける、それだけの事で疑念の目を向けられなくて済むのだよ。
だから、気をつければいいと思うんだ僕は。
「うーん、イマイチ」なコードが、少なくとも自分の近辺からは無くなって欲しいと望んでやまないよ僕は。

本当のグチの理由

最近自分の会社の奴らが作ったコードをチラッと眺める機会に恵まれたんでどんなレベルなのかなーって興味本位で眺めてみたワケ。
そしたら、もうなんていうかさぁ……。
「え、これ仕事で作ったコードですよね？」
って感想があふれてきて止まらなかった。
コイツらと同類と思われたくは無いという恥ずかしさやら、教えてくれる人はいないのかという憐憫やら、なんか色々籠もったね、感情がさ。

いや、分かるよ、現実には納期というものがあって、設計にこだわってる暇なんて無いんだよってことは。
だから取り敢えず動く形にしてしまおうという気持ちは分かる。
コードレビューで、うーん設計悪いなーと思っても作り直す時間は無いよね、普通はね。

だから「ここはまぁ瞬発力勝負だったんだろう」って推察できるように TODO コメント残すなり、何か本意ではないことをアピールして欲しい。
規模が小さいからいいや、って思ってるならまだマシだけど、個々人の心境まではコードからは分からんからね。何も爪痕が無いと形として「悪いものが出来た」という感想になってしまうよ。

だから規模が小さくても、正しい設計は必ず意識すべきだよね。正しい実装が面倒でショートカットするなら、それなりにコメント残して。
形として出てくるのはコードだからさ。人のその時の感情は、コードには乗らないからさ。
コード納品したら、そのコードを流用して他の会社に渡すかもしれないんだからさぁ。

ホント、頼みますよ。