アーキテクチャをスマートに。

株式会社ネオジニア代表。ITアーキテクトとしてのお仕事や考えていることなどをたまに綴っています。(記事の内容は個人の見解に基づくものであり、所属組織を代表するものではありません)

ホントにあったクソコードの話(2014年春号)

最近久しぶりに Java の開発プロジェクトに参画したので、実際にそこで見たクソコードをいくつか挙げてみます。
提示しているソースコードは、実際のソースコードをもとに出来るだけニュアンスが伝わるよう書き換えています。

定番のやつ

まずはやっぱり定番のこれですね。

List<String> list1 = new ArrayList<String>();
list1 = getList();

ホント多いです。こういう無駄なやつ。
コードが無駄なだけでなく、実行時にCPUとメモリを浪費するので困ったもんです。

間違い探し

public final void main(String[] args) {
    ...

main が static になってないですよ~。実行できないですよ~。
「これをベースに作っていってくれ」とサンプルコードとして提示されたコードがこんなんでした。。。。先が思いやられる。。。

その発想はなかった

// 前処理
...
    
new Hogehoge();

// 後処理
... 

え〜と、メインロジックは全部コンストラクタの中ですか?
とりあえず、一発殴っていいですか?


車輪の再発明

/**
 * 文字列出力ストリーム.
 */
public class StringOutputStream extends OutputStream {

    /** This buffer will contain the stream. */
    private StringBuffer sb = null;

    ...
}

なになに〜、StringOutputStream だって!?
StringBuffer を内部で持ってて、write するとそのバッファに書き込んでいって、一連のストリーム出力処理が終わると String として取り出せるってわけですね。
いやいやいやいや、java.io.StringWriter があるでしょうよ。
もしかして OutputStream じゃないとダメな理由でもあるのかね?
と思って周りのコード見始めたらもう周りにも無駄なものが多すぎて閉口。。。
結局CSV出力したいだけなのに難しいライブラリ呼んでて、そいつに与えるために OutputStream が必要だから作ったっぽい。

しかも何か英語のコメント入ってるし、どこからパクッてきたんやこれ。。。

たかがCSV出力でこんなの使わされるのは何としても避けたいので、StringBuffer でラッパー作って先にテスト通して「もう自前のクラスで書いちゃいました。皆さんこっち使って下さい」て言ってやった。

System.exit() が効かない?

参考にすべしとして提示されたコードが

// 引数チェック
String returnCode = checkArgs(args, 3);
if (isErr(returnCode)) {
    logger.logout(returnCode, "引数が不正です", null);
    // 返却値:異常
    System.exit(XxxxConst.BATCH_EXIT_ERROR);
}

// メインロジック実行    
...

ってなってて、System.exit() を利用して終了ステータスを返すという仕組みになっていました。
Web系なら System.exit() なんてマズイに決まってるのですが、この例の場合はバッチ処理だったので、特にマズイやり方でもないと思っていました。
さらにテストコードが djUnit で書かれていて、なかなかナウイ(死語?)と思ってたんですが、実際それらを参考に大体同じような構造でコード書いていくと、引数チェックのテストがいくらやっても通らない。んでテストコードをよく見てみると

@Test
public final void testCase01() throws Exception {

    // System.exitメソッドを無効化
    addReturnValue("java.lang.System", "exit");
    
    ...
}

となってて、System.exit() がスルーされてメインロジックが実行されてしまうという罠でした。
どうやら、ホントに exit() してしまうと djUnit が中断されてテストが出来なくなるので、無効化してあるようでした。
仕方ないので System.exit() 呼んでる箇所すべてに return を付け加えた。結果、何とも気持ち悪いコードになった。
そもそも System.exit() じゃなくて素直に return で結果を返すようにしとけばこんな無駄なことに時間を使わなくて済むのに。アホらしい。

あと、ログ出力のメソッド名が「logout」ってのも笑ってしまった。センスなさすぎでしょう。

isErr() とかもやめて欲しい。これは次の話題で。

無用の長物

ロジッククラスのベースクラスの中を見てみると isNormal()、isErr()、isNull()、isNotNull() の4つが定義してあるだけでビックリした。

  • isNormal() は、ある定数と引数が同じかどうか比較してるだけ。
  • isErr() の中身は return !isNormal();
  • isNull()、isNotNull() に関してはお察しの通り引数を null と比較してるだけ。

もうすでに全部不要確定*1なんですが、せっかくなのでそのうちの一つ isNull() のコードを見てみましょう。

/**
 * オブジェクトが未生成(NULL)か判定しかえします.<br>
 *
 * @param obj 未生成かチェックするオブジェクト
 * @return boolean 生成済み:false、Null:true
 */
public static Boolean isNull(Object obj){
    if (obj == null){
        return true;
    }
    return false;
}
  • 全体的な書き口から察するに VB系の人が書いたのかな? ぜひJavaも勉強してほしいです。
  • "オブジェクト"とか"未生成"とかって単語の使い方がちょっと変な感覚ですね。
  • 戻り値が boolean じゃなくて Boolean ってのも謎。オートボクシングこわい。
  • なんで一行で return obj == null; って書かれへんの?
  • ていうかそもそもこんなメソッド何の役に立つねん?

このメソッドだけでこれだけツッコめるとはちょっとスゴイです。大阪人なのでソースコードちょっと眺めるだけでもう反射的にツッコミまくってます。
なんか頭痛くなってきました。

検査例外、お嫌いですか?

このコードは別のプロジェクトから。クソコードとはちょっとニュアンスが違うかも知れませんが、ついでなのでご紹介。

try (PreparedStatement stmt = con.prepareStatement(SQL_FIND)) {
    ResultSet rs = stmt.executeQuery();

    while (rs.next()) {

        ...  // XMLのロードとかして色々頑張る

    }
} catch (SQLException e) {
    throw new RuntimeException(e);
} catch (IOException e) {
    log.error("XMLのロードに失敗しました。");
    throw new RuntimeException(e);
}

まぁ、気持ちはわかりますよ。Javaの検査例外、鬱陶しいですよね。
でも何でもかんでも盲目的に RuntimeException にしてしまうのはやっぱり良くないですね。
え?メンドクサイ?いや、仕事でしょ?

これをすると回復可能な例外であっても回復できず、一緒くたになってコケてしまいます。
SQLの文法エラーとかは設計時に考慮してないし大体バグなので、実行時例外にしてコカしても良いかもしれないけど、例えば一意制約違反とかはちゃんと考慮して回復すべきですよね。
あとXMLのロードエラーはどう考えても回復して呼び出し元にエラー返すべきないんじゃない?

大げさなログ出力

最後はちょっとした力作です。

try {
    
    ...

} catch (SQLException | IOException | CloneNotSupportedException | SAXException
    | ParserConfigurationException e) {
    try {
        if (comDb != null) {
            comDb.rollBack();
        }
    } catch (SQLException e1) {
        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
        e1.printStackTrace(pw);
        LOGGER.error(sw.toString());
    }
    LoggerCommon logCm = new LoggerCommon();
    List<Object> list = new ArrayList<Object>();
    list.add(args[0]);
    list.add(args[1]);
    LOGGER.error(logCm.getParam(new String[] {"接続ID", "処理日付" }, list));
    LOGGER.error(String.format("例外発生 %s", e.getMessage()), e);
    strExitCode = XxxConst.SYSTEM_ERR;
} catch (Throwable e) {
    try {
        if (comDb != null) {
            comDb.rollBack();
        }
    } catch (SQLException e1) {
        StringWriter sw = new StringWriter();
        PrintWriter pw = new PrintWriter(sw);
        e1.printStackTrace(pw);
        LOGGER.error(sw.toString());
    }
    LoggerCommon logCm = new LoggerCommon();
    List<Object> list = new ArrayList<Object>();
    list.add(args[0]);
    list.add(args[1]);
    LOGGER.error(logCm.getParam(new String[] {"接続ID", "処理日付" }, list));
    LOGGER.error(String.format("例外発生 %s", e.getMessage()), e);
    strExitCode = XxxConst.SYSTEM_ERR;
} finally {
    
    // 似たようなログ出力のためのコードが延々と...

}

もう突っ込みどころが多すぎてどこから言えばのやら。。。

  • まず、2つの catch 節の中身はなんと全く同じです。コピペしたのでしょう。catch (Throwable e) の方だけあれば全部キャッチできるんだけどな。。。
  • メンバ変数 LOGGER は log4j の Logger です。スタックトレースをログ出力するためにStringWriter 使って頑張ってますが、Logger.error("エラー", e1) とすれば済む話ですよ。え?知らなかったって?いや最後の LOGGER.error であんたそれ使ってますよ?
  • LoggerCommon#getParam() はその名前からは想像できない挙動をします。なんと Map を作って返します。ツッコミどころは、その引数を (String[], List) よりも (List, String ...) にすればいいのに、、、という話以前に、こんな難しいことしなくても素直に
LOGGER.error(String.format("接続ID=%s 処理日付=%s", args[0], args[1]))

で十分じゃない?

  • 最後の LOGGER.error() も冗長ですね。その上の LOGGER.error() に e を渡してしまえばいいんじゃないの?

つまりこんだけ書けば十分だと思われます。

} catch (Throwable t) {
    if (comDb != null) {
        try {
            comDb.rollBack();
        } catch (SQLException e) {
            LOGGER.error("Rollback Error", e);
        }
    }
    LOGGER.error(String.format("接続ID=%s 処理日付=%s", args[0], args[1]), t);
    strExitCode = XxxConst.SYSTEM_ERR;
} finally {

本当にお疲れ様です。

あとがき

クソコードっていうのは突然1箇所に出現するんじゃなくて、全体的にクソまみれになってる状態がほとんどです。大抵、設計からすでにクソ設計で、さらにヤバイ現場はもうプロジェクト自体がクソプロジェクトなんです。そらもうムチャクチャですわ。

実際にクソプロジェクトによくある実例をだすと、例えば、コーディング規約の内容や意図もわからずに CheckStyle 導入するのとかですね。もうホントやめて欲しい。
穿った見方をすれば、法律の内容や意図もわからずに施行するのと同じですよ。そんなバカなこと有り得ないですよね。

CheckStyle 自体はいいツールだと思います。ただ道具の使い方を間違ってるだけです。

コードスタイルを合わせるために導入するのはいいと思うんですが、プログラミングの自由度を制限するのは賛成できません。
例えば、無名クラスを使おうとすると、CheckStyle が「無名インナークラスの長さが n行あります。最大 0行」と言ってきて頑なに拒絶されたりとか。
で元請けの人に「無名クラスは使用禁止ですか?」って尋ねたら「何それ美味しいの?」的な反応だったので、もう考えるだけ無駄なんだとわかった。

*1:あえて残すなら isNormal() だけユーティリティ系クラスに移動するのが望ましい