こんなコードは嫌だ！現場で実際に見たアンチパターン集

はじめに

内容はタイトルの通りなのですが、アンチパターンなコードを書いた人をこき下ろす意図は一切ありません。コードを憎んで人を憎まずという言葉もありますしね。¹

そのようなネガティブな動機ではなく、アンチパターンからできるだけ教訓を汲み取って前に進もうというポジティブな動機で筆を執っています。

全体的に古い話が多いので、最近はこのようなコードや状況が減っていたらいいな、と願っているところです。

環境

• 主にJava 6

内容

Struts1を拡張したオレオレフレームワークの使用

ご存知の方も多いでしょうけど、Struts1はもう何年も前にサポートが切れているので、この時点で嫌な予感しかしないところです。

Strutsをそのまま使うならまだいいんですが、そのプロジェクトでは見出しの通りStrutsを拡張したオレオレフレームワークを使っていました。そのフレームワークでは、1リクエストの処理を書くのに大量のクラスを作成する必要があり、StrutsのActionやFormBean、ビジネスロジックを記述するBLogic、BLogicへの入力のためのBLogicInputBean、出力のためのBLogicOutputBean、JSP、場合によってはDAOやDAOに渡すためのDAOBeanも必要など盛り沢山でした。他にもstruts-config、Strutsのvalidation、Springの定義、iBATISのsqlMapとXMLファイルの修正もてんこもり。

特に、同じようなBeanを何種類も作る必要があるのは謎でしたが、それらのBeanはフレームワークの規約として嫌でも作るしかなく、それに中身はほぼ同じなのに型としては別なので、Beanの詰め替えを手動で書く必要があって時間をとられました。²

どうすれば良かったのか

当時はまだStrutsに代わるような有力なフレームワークがあまりなかったので、Strutsの採用自体はある程度は仕方がなかったのかなと思います。ただ、素のStrutsでは不要な大量のクラスの作成を強制するように魔改造してしまったのは謎です。素のStrutsをそのまま使えばよかったのではと思います。
簡単に調べてみると、そういうフレームワークは生産性を上げることではなく、開発者のスキルが低くてもなんとかなるようにすることが目的なのだとか。³

もしかしたら、そのようなフレームワークを採用した上で単価の安い新人をアサインし、コストを削減していたのかもしれません。（実際、新人が多かったです）
このフレームワークの選定がそういった政治的な理由によるものだとすると、もはやどうしようもないですね。逃げ出すくらいしか対処方法が思いつきません。技術力が高い人なら参考リンク先の方のように技術的に頑張ることもできるかもしれないですが、当時の私は思いつかなかったです。私はまともなプログラマじゃないかもしれません。

とりあえず、今から新規開発するならStrutsを選ぶ理由はないでしょう。やっぱりSpring Bootがいいんでしょうかね。

コピープログラミングの横行

既存の機能と似たような機能を作る時についやってしまうのがコピープログラミングです。既存のコードをコピペして差分だけ修正するというアレです。私もやってしまうことがあるので偉そうなことは言えないし、コピープログラミングが常に悪だと言う気はありませんが、それでもコピープログラミングには欠点があることを指摘せざるをえません。

コピープログラミングを行うと、自分がよく理解していないコードでもあっという間に機能が実装できてしまうため、ある種の快感がありますし、さらに一見すると生産性が高そうに見えますが、メンテナンス時に地獄を見ます。
まず、何か修正があったらコピーした部分の全てに対して同じ修正をしなければなりません。それに、コピーした際の修正漏れで、変なバグを発生させるリスクがあります。コピーによって、内容をよく理解していないのに一気に組み立てられてしまったコードは読むのが大変に億劫なので確認が疎かになりがちで、修正漏れが発生することはよくあります。コードの修正漏れは問題ですが、コメントの修正漏れもまた問題です。コメントの修正が漏れても動作に影響しないため、ある意味ではコードの修正漏れよりもたちが悪いかもしれません。

私が見たプロジェクトではコピープログラミングが横行しており、というかコーディングの大半がコピープログラミングによってなされていたのではと思われます。そのプロジェクトではコメントの修正漏れがとにかく多くて、もはやコメントが全く信用できない状態になっていました。それではコメントがないのと同じです。コメントが全くないコードをメンテナンスすることの辛さは、皆様もよくご存知のことだろうと思います。

どうすれば良かったのか

同じ処理なら、コピーするのではなくメソッド等に抽出して共有すべきでした。それなら修正が発生しても一箇所だけ直せばいいですし、そもそもコピーしないので修正漏れも発生しません。
ただし、逆に言えば一箇所修正するだけで複数箇所に影響が及ぶようになるため、「本当に共有していいのか？」ということはよく検討しなければいけません。⁴

修正内容をコメントアウトして全て残す

諸悪の根源です。皆様も一度は聞いたことがあるであろう、悪名高いやつです。一時的にコメントアウトするくらいならいいですが、ここで言っているのは全ての修正について永続的にコメントアウトしたままにするということです。その上、そのプロジェクトでは修正時のルールとして、修正開始を示すコメントと修正終了を示すコメントも挿入しなければならないこととなっていました。修正をするたびにゴミが増えるため、非常に邪魔です。とても読みづらいですし、検索のノイズにもなります。全ての修正が残るため、コードは汚くなる一方です。修正が多い箇所ではカオス過ぎて読めたものではありません。
言うまでもなく、コードを読むというのは非常に高頻度で行われる作業ですから、これも生産性がガタ落ちする原因の一つでした。百害あって一利なしとはこのことです。

どうすれば良かったのか

SVNでもGitでも何でもいいですが、何らかのバージョン管理システムを使うべきです。⁵

JSPのスクリプトレットに大量のJavaコード

JSPにビジネスロジックが大量に記述されているという場面がよくありました。HTMLとJavaが混在して非常に読みにくかったです。あと、一つのJSPファイルに記述するコード量が多すぎてエラーになる⁶場合があったため、一つの画面でJSPを分割していて、これまた読みにくかったです。適切な単位で分割するのはいいんですが、この場合は巨大なJSPを適当に2つや3つにぶった切ってファイル名に連番を付けるというやり方だったので、適切な単位での分割とは言い難かったです。

それと、条件によって画面の表示が変わるのを実装するために、愚直に条件分岐を使って全部べた書きしていました。これもコード量が膨れ上がって読みづらくなる原因となっていました。

どうすれば良かったのか

ビジネスロジックはモデルに書きましょう。MVCの基本です。
また、条件によって画面の表示が変わるのならサブのJSPを作って読み込ませるなど、一つのファイルが肥大化しないようにするための工夫をすべきです。

ちなみに、今は新規開発でJSPを採用することはあまりないのではと思います。今主流なのはやっぱりThymeleafですかね。

リファクタリングしない

触らぬコードに祟りなしということで、コード上問題があっても、動作上の問題がなければ触ってはならないことになっていました。そのためちょっとした問題は修正されず、どんどん増えていきました。警告が出ていても修正できないので、警告が溜まりに溜まっていました。警告が大量にあると、本当に問題がある警告が埋もれてしまうので、これは非常に良くないです。

また、TODOコメントも大量に放置されていました。長期間TODOのままで放置されていると、どう扱っていいのか地味に困ります。また、個人的に追加したTODOが埋もれてしまうので邪魔でもあります。

どうすれば良かったのか

多少時間を割いてでもリファクタリングを行うべきでした。時間がないからやらないというのは一見正しそうに聞こえますが、そうではありません。やることはたくさんあるので、自然に時間ができるなんてことはまずありません。他の作業を調整して時間を作るという思考でないと時間なんてできません。ないのは時間ではなくやる気です。⁷

また、コードを触るとテストしなくてはならないので嫌という話もあります。それはわかるのですが、テストしてでもリファクタリングすべき場合もあります。また、バグ修正や機能追加によってコードを修正した際に、どうせテストしなければならないので、ついでにリファクタリングも行うなどの工夫も、その気になればできますし、そうすべきだったと思います。
ちょっとした修正であればUT実行で担保できそうなものですが、このプロジェクトではそもそもUTなんて全く書きませんでした。まずはそこからですね…

もっとも、修正内容をコメントアウトして全て残すルールがある限り、リファクタリングもくそもありません。最低でもこのルールだけは何としても破棄しないといけません。

定数病

一般的に、何度も出現する固定値を定数で扱うようにするのは良いこととされていますが、その意図がわからずに意味不明な定数が作られてしまうことがあります。

public static final int INT_0 = 0;
public static final int INT_1 = 1;
// 以下同じようなのがずらずらと続く

これではリテラルを使うのと何も変わりませんね。謎ですが、これを書いた人は誤った教育によってリテラルを直接使うのは悪であると刷り込まれてしまったのかもしれません。
定数病という言葉は自分で考えたつもりだったのですが、既に同じことを考える人もいたようです。まさにこんな感じです。
【ソース編】「DIV病」とは、何事もやり過ぎると危険であることの代名詞である

どうすれば良かったのか

あまり解説するまでもないような気もしますが、定数とは本来は将来変更されるかもしれない値に対して使うものだと思います。定数を定義して、その値が必要な箇所では定数を参照するようにしておくことで、何か変更があっても定数の初期化の部分だけ修正すれば済みます。
また、将来変更するかもしれないという性質から、定数名はどのような意味の値なのかということを元に命名すべきで、具体的にどのような値が入っているのかということを元に命名するのはナンセンスです。
簡単な例を示します。

// ダメな例
public static final String LF = "\n";

// 良い例
public static final String LINE_SEPARATOR = "\n";

ダメな例のほうだと、将来変更されることになった際に困りますよね。下の例のように、値が変わっても違和感がない命名にしましょう。

あと、将来変更される可能性があまりない値であれば、無理に定数を使わずにそのまま使えばいいと思います。こちらも例を示します。

public static boolean isEven(int num) {
    return num % 2 == 0;
}

ご覧の通り、数値が偶数かどうかを判定するコードです。「2」や「0」といった数値リテラルをそのまま使っていますが、この例であればこれらの数値が将来変更されるとは考えづらいです。そのため、リテラルをそのまま使っても特に問題はないと思います。

拡張for文を使わない

拡張for文を使える箇所で、普通のfor文で頑張っているコードもよく見かけました。ループカウンタが必要ないのであれば、拡張for文を使いましょう。そのほうが書きやすく、バグが入り込む余地が少なく、読みやすくなります。使える場面で使わない理由があまり思いつかないのですが、おそらく存在を知らないでしょう。
そんな人いる！？と思われたかもしれませんが、普通にいると思います。でも最初に書いたように、そういった方をこき下ろす意図は全くありません。知らないのであればこれから知っていけばいいだけですからね。
また、Java8以降であればStream APIの使用も検討しましょう。

クラス名に機能IDが使われている

クラスにはクラスの責務がわかるような名前をつけるのが一般的ですが、あるプロジェクトでは全てのクラスが機能IDで命名されていました。適当に考えた例ですが、たとえば「AP060.java」「AP060BLogic.java」「AP060Bean.java」とかそんな感じです。これでは各クラスが何なのかサッパリわかりません。まあ、どの機能IDが何の機能なのかを覚えてしまえば意外と大丈夫だったりするので、人間ってすごいなーと思ったのですが、一般的には異常な命名規則であると認識しておく必要があります。

どうすれば良かったのか

これも書くまでもないでしょうけど、そのクラスが何なのかわかるような名前を付けましょう。名前重要。

if文の中のコードが長すぎる

if文の中の行数が長すぎて、何がどこに書かれているのかよくわからないというコードもよく見ました。if文の中に何百行と処理が書かれていて、終わったと思ったら今度はelseの中に似たようなコードが延々と続くみたいな。
分岐の条件は、新規登録の場合と更新の場合など、大半は同じ処理だけど一部だけ値が異なるみたいな感じのが多かったです。差分に対してだけif文を使えよと…

どうすれば良かったのか

ちょっと書いちゃいましたが同じ処理ならif文の外に出して、差分だけ条件分岐すべきです。また、意味をなす条件分岐だとしても長いのはやはり辛いので、そのような場合はif文の中の処理をメソッドに切り出してわかりやすい名前を付けるなど、ちょっとした工夫で読みやすさを追求したいところです。場合によっては、ポリモーフィズムを活用して条件分岐そのものを消せないかどうか検討してもいいでしょう。

スコープが無駄に広い

ローカル変数をメソッドの先頭で定義しているが、その変数を実際に使うのはずっと下みたいなコードをよく見かけました。C言語（の古いバージョン）であればそうしないとコンパイルエラーになりますが、Javaにはそのような制約はないので、何の意味もありません。ただ変数の定義部分と使用する部分が離れてわかりづらくなるだけです。
また、ローカル変数で事足りるのにわざわざフィールドとして定義している変数も散見されました。フィールドだと他のメソッドから読み書きされている可能性もあるので、値の変遷を追うのが辛くなります。

どうすれば良かったのか

ローカル変数は必要になってから定義しましょう。また、1つのメソッド内でしか使わない変数はフィールドではなくローカル変数として定義しましょう。まあ、この辺りはIDEが警告を出してくれるかもしれませんが。

変数の使い回しによるリソース解放漏れ

疑似コードですが、こんな感じのコードをよく見かけました。何がダメなのかおわかりでしょうか？

ResultSet rs = null;

try {
    for(HogeBean bean : beanList) {
        // なんかbeanを使った処理...

        rs = stmt.executeQuery();

        // なんかrsを使った処理...
    }
} finally {
    if(rs != null) {
        rs.close();
    }
}

まあ見出しで察しがつくかもしれませんが、リソース解放がきちんと実行されていません。ループの中で変数rsを使い回しているので、close()がちゃんと呼ばれるのは最後の1つだけです。

どうすれば良かったのか

try-finallyをfor文の中に入れるなどして、全てに対してclose()が呼ばれるようにしましょう。加えて、Java7以降ならtry-with-resourcesを使いましょう。まあ、これは静的解析ツールで検出されるとは思いますが。

インデントが適当

インデントは重要です。インデントが適当なソースは本当に読むのに苦労します。if文やfor文などがネストしている場合は特に。
何もないところでインデントしていたり、インデントが必要な箇所なのにしていなかったり、酷いコードをよく見かけました。前述のif文の中のコードが長すぎるのと相まって本当に読みづらかったです。おそらく、書いた人自身も混乱してしまっているのでしょう。コードが何百行とあるのが元凶ですね…
しかし、それだけではありません。修正内容をコメントアウトして全て残すという問題があります。ほとんどのメンバーは先頭行に「// 」を付ける形式でコメントアウトしていたのですが、これをやるとコメントアウトされたコードが少し後ろにズレます。これも混乱の元になりました。さらに、インデントくらい勝手に直したいものですが、コメントアウトで残すルールのせいでなんとなく修正しづらいと感じてしまいました。
なんかもう、複数の原因が絡まり合って酷いことになっていてため息しか出ませんでした。

どうすれば良かったのか

根が深い問題ですが、コメントアウトルールの廃止と、何百行とあるコードの削減がまず必要です。それができれば、インデントも自然に直るのではと思います。

無駄にStringBuffer

別にスレッドセーフが求められるわけでもない箇所で、文字列の連結にStringBuilderではなくStringBufferを使っているコードもよく見かけました。これも拡張for文を使わないと同じで、StringBuilderの存在を知らないのでしょう。まあ、それで性能問題が発生したわけでもないので、どうでもいいのかもしれませんが。

ラッパークラス同士を == で比較している

これは厄介なバグに繋がりかねない問題です。ラッパークラス同士だと参照を比較するため、基本的には別のインスタンスの場合は同値であっても false が返ってきます。ここまではString同士を == で比較してはいけないのと同じです。
しかし、厄介なのは値によっては true が返ってくることもあるということです。具体的には-128〜127の値だと、同値であれば別インスタンスであっても true が返ります。小さい値でテストした時は問題なかったからといって == で比較したコードをリリースしたりすると痛い目にあうかもしれません。要注意です。

どうすれば良かったのか

Stringと同様に equals を使って比較しましょう。ちなみに、この問題は静的解析ツールで検出されると思います。

ロジック上、絶対にthrowされないチェック例外がthrows宣言されている

見たことがあります。メソッドの定義上はthrowされるように見えるけど、ロジック上は絶対に投げられない例外がthrows宣言されていることにより、例外処理のコードが爆発的に増殖していく様を…
それはあちこちから呼ばれる重要なメソッドだったので、それはもう大変に増殖していました。その例外は絶対に投げられないのに。絶対に投げられない例外を処理するためのコードなんてゴミとしか言いようがありません。こうしてまたコードが無駄に肥大化するのでした。
むしゃくしゃして無断で消してまわりました。とても楽しかったです。

どうすればよかったのか

throws節の定義は慎重に設計しましょう。その例外は本当に投げられることがあるのか、よく確認しましょう。Javaのチェック例外は、呼び出し元に例外処理を強制するという強い力を持っています。その例外の処理を呼び出し元に強制させるのは本当に適切なのか、よくよく考えましょう。

その他（コード以外）

iBATISの使用

iBATISは便利なORマッパーなのですが、とっくの昔にサポート切れしており、Raw型のListを返すなど仕様も古いです。後続のMyBatisに移行すべきでしょう。

Excelを使った設計書で、修正前の内容をグループ化して残す

Excelでは特定の行や列をグループ化することができ、グループ化した箇所はワンクリックで表示・非表示を切り替えることができます。この機能を使って、修正前の内容を非表示状態で残すということです。
こいつも百害あって一利くらいしかない代物です。まず、修正内容を全て残すためファイルが重くなり、表示に時間がかかって生産性が落ちます。また、普段は非表示とはいえ存在はしているため検索ノイズになりますし、オートフィル機能で連番などを作る時に、途中にそのグループがあると、その中身にも連番が作成されてしまって非常に邪魔です。
なんか、修正内容をコメントアウトして全て残すのと似ていますね。基本的に、修正履歴をそのファイル自体に持たせるのは悪いアイデアなのかもしれません。
なお、一利くらいはあると書きましたが、ワンクリックで修正前の内容を閲覧できるのは一応便利です。でもメリットはそれくらいしか思いつかず、デメリットのほうが多いです。

どうすれば良かったのか

設計書をSVNなどでバージョン管理すればいいのではと思います。もちろんExcelファイルはバイナリファイルなので、そのままバージョン管理してもあまりうれしくありません。なので、設計書をExcelではなくMarkdownやAsciiDocで作成すればいいと思います。それならテキストファイルなのでバージョン管理しやすいですし、どうしてもExcelがいいなら、MarkdownやAsciiDocをExcelファイルに変換すればいいです。（ちゃんと調べてないですが、たぶんできると思います）

使い勝手の悪いオレオレカバレッジ計測ツール

プロジェクトで使用するように指示されたカバレッジ計測ツールが、元のコードを全コピーしてカバレッジ計測用のソースコードを埋め込むというなかなか大胆な方法をとるツールでした。（カバレッジ計測ツールには詳しくないのですが、バイトコードレベルで見るのが一般的のような気が…）
テスト（UTではなく手動のテスト）を実施する際に、そのツールが生成したソースコードをビルドしてテストを実行するというルールとなっていたのですが、そのツールがコピーして配置してくれるのはJavaのファイルだけなので、その他のXMLやJSPなどは全部手動コピーしなければならず、テストが実施できるようになるまでに時間がかかって非常に不便でした。
それに、元のソースとツールが生成したソースの2つがあるので、2つのファイルの同期がとれているように気を使わないといけません。ソースを少し修正したあとに、その修正を計測用ソースのほうに反映させるのを忘れて、テストをやり直すハメになるといった事故が何度か起こりました。
さらに、カバレッジを計測するには謎のSwingアプリケーションを起動させておかなければならず、重くて邪魔でした。
さらにさらに、そのSwingアプリケーションを立ち上げてなくても正常に動作してしまう（動作はするがカバレッジは計測されない）ため、ここでも後で気づいてテストやり直しという事故が起こりました。
このように使い勝手の悪いツールの使用を強制され、また生産性が下がるのでした。

どうすれば良かったのか

正直なところカバレッジ計測ツールに詳しくないのですが、自社でしか使われていないようなオレオレツールではなく、JaCoCoとか有名なツールを使えば良かったのではと思います。今の私の職場で使っているカバレッジ計測ツールはたしかJaCoCoなんですが、ソースコードレベルで計測コードを埋め込むなどという事故を誘発するような処理はなかったと思います。
というか、カバレッジ計測って手動テスト時ではなくてUT実行時に計測するのが一般的ですよね。そもそも、UTを全く書いていないのが問題なのかもしれません。⁸

SVNで1ファイルずつコミットする人

これも謎なんですがいらっしゃったのですよね、そういうお方。一つの修正なのにコミットが複数（それも2つや3つではなくたくさん）に分散するので、修正履歴を見るのが面倒です。おそらくバージョン管理システムの意義がよくわかっておらず、単にみんなでソースを共有できる仕組みくらいにしか思っていないのでしょう。
それだけならまだいいほうで、中には全部コミットし終わってないのに離席しちゃうような猛者もいます。その間にチェックアウトしてしまうと、もちろん中途半端な状態で取得されてしまって、消えないコンパイルエラーに悩まされることになります。

どうすれば良かったのか

バージョン管理システムはどのようなもので、上記のような振る舞いがどういう影響をもたらすか、言って聞かせるしかないでしょうかね。他社の人だったり、自社の先輩だったりするから面倒ですが…（※今の職場ではありません）

終わりに

コードと言いつつコード以外の内容も含まれていてごった煮のようになってしまいましたが、何か一つでも参考になった項目があれば幸いです。

1. ググってみたら本当にありました。 ↩

2. 後に、似てるけど型が違うBeanの内容をコピーしてくれるライブラリがあることを知ったので、その点は多少楽になったのですが。
   しかも、これらを「1リクエストごと」に作成するので、表示のリクエスト、新規登録や更新のリクエスト、画面の一部項目を更新するためのリクエストなど、それぞれに対して上記を作成する必要がありました。とにかく修正量が多いので非常に生産性が低かったです。 ↩

3. 参考: 侵略的なフレームワーク ↩

4. 参考: 共有は慎重に ↩

5. まあ、何か特殊な事情がない限りはGitを選んだほうがいいでしょうけど。
   バージョン管理システムがあればこのようなことは一切する必要はありません。今時、バージョン管理システムを使わずに開発しているプロジェクトなどないでしょう。…ない…ですよね………？
   もっとも、そのプロジェクトではSVNでバージョン管理していたにもかかわらず上記のルールだったので、全くもって意味不明だったのですが… ↩

6. 参考: javaの制限である１メソッド内でコンパイル結果が65535バイトを越すと、実行できない問題を即時解決してほしい。 ↩

7. 時期にかかわらず全員が毎日22時くらいまで働いていて、たまに休出もあるという状況だったので、時間がないというのは本当でした。ただ、全体的に非効率的な開発手法だったり、体調不良で頻繁に休む人がいたり、自席を立ってから30分くらい戻ってこない人がいたり等、諸々合算されて時間がなくなっていたので、その気になれば時間の捻出は可能だったと思います。 ↩

8. UTがないのは間違いなく問題ですが、カバレッジの計測という観点から見ても同様、ということです。 ↩