そのクソコード、Intellij IDEAでチェックできるよ

1月 14, 2014

そのクソコード、Intellij IDEAでチェックできるよ

愛知県でシステムエンジニアとして働く友人のMは、プロジェクトメンバの書くJavaのクソコードに苦しめられているそうです。Mはリードプログラマとして、プロジェクトメンバがあげてくる成果物（ドキュメントとコード）のレビューをする立場にあるらしく、提出されてくる数々のクソコードをTwitterでつぶやいていました。

Mを救うことはできるのでしょうか？　もし、クソコードをすばやく見つけることができたら救えるのであれば、救える見込みはあるかもしれません。

コードの問題を見つける静的解析ツール

クソコードとは、おおむね次のような問題のあるコードをさすようです。

潜在的バグ: バグの可能性があるコード。
重複: 機能追加やバグ修正を困難にしがちなコードの重複。
設計上の問題: クラスやパッケージ間の依存関係、多すぎるメソッド引数など。
慣習違反: プログラミング言語やライブラリの慣習、コーディング規約などに違反したコード。

多かれ少なかれ良識と技能のあるプログラマが、こういった問題のあるコードを渡されたとき、怒りを込めてクソコードと言ってしまうのでしょう。

そのようなソースコードに含まれる問題をすばやく見つけてくれるツールが、いくつかあります。ソースコードなどから作られたプログラムは動かさずに、ソースコード（またはコンパイルして得られたバイトコード）だけを検査することから、静的検査ツールとか静的コード分析ツールなどと呼ばれます。Javaプログラミングで代表的なものをあげると、

Checkstyle: コーディングスタイルからの逸脱を中心に検査します。多様なIDEやエディタで使えます。
FindBugs: 既知のバグのパターンにもとづいて潜在的バグを検査します。多様なIDEAやエディタ、JenkinsなどのCIツールで使えます。
Intellij IDEAのCode Quality Features: Intellij IDEA専用ですが、非常に強力なので取り上げます。

静的解析ツール vs 実世界のクソコード

静的解析ツールは、果たして、どれくらいMを救うことができるのでしょうか？

広すぎる変数のスコープ

まあ、もういまさら些細な事なんだけど、メソッドの先頭で使う変数を全部宣言するのやめようぜ。
— みずぴー (@mzp) 2014/1/4

そのとおりですね。Javaプログラミングでは、変数は、使う直前で＆必要なスコープで宣言するのが良いです。主な理由は、

変数の宣言位置と使われる位置が近づくことで、コードが読みやすくなる。
変数が使われるブロックを抜けたら、その変数を意識しなくてよくなる。
もし変数が宣言されるブロックに到達しなければ、その変数を宣言するコードは実行されくなるので、実行速度が向上する。

メソッドの冒頭で変数を宣言してしまう（スコープを広くしすぎる）のは、変数のブロックスコープがない言語をやっていた人がJava言語をやると、よくやってしまうようです。せいぜい数行~十数行であれば気にならないかもしれませんが、数十行~数百行もあるメソッドで、十数個もの変数にこれをやられると、どの変数がどこで使われるのか、わけが分からなくなります。

さて、この問題は、静的検査ツールでは検知してくれるのでしょうか？

IntelliJ IDEA 13.0.1	Checkstyle 5.7	FindBugs 2.0.3 with fb-contrib plugin 5.0
Data flow issues > Scope of variable is too broad データフローの問題 > 変数のスコープが広すぎる。	なし	なし

IDEAでのみ、検知する設定を見つけることができました（見つけられていないだけで、他の2つにもあるかもしれません。以下同様）。

またIDEAでは、Quick Fixという修正までやってくれる機能もあります。これを使えば、修正も非常に容易です。

無意味な初期化

Foo foo = new Foo(); foo = getFoo(); #何がしたいんだ
— みずぴー (@mzp) 2013/12/28

これもありがちですね。最初に作られたFooインスタンスは、初期化のための計算リソースとメモリを消費だけして消えていきます。まったくムダです。

また次のようなバリエーションもあります。

String foo = null;
foo = "foo";

int bar;
bar = 1;

このようなコードは、どうやら、変数の宣言と代入を分けないといけない言語をやっていた人が、よく書いてしまうようです。宣言と代入を同時にできるJavaでは、基本的には、同時にやるべきです。

これも、IDEAなら検知することができます。

IntelliJ IDEA 13.0.1	Checkstyle 5.7	FindBugs 2.0.3 with fb-contrib plugin 5.0
Probable bugs > Unused assignment 潜在バグ > 使われていない代入	なし	なし

メソッドの命名が慣習に違反している

ret = obj.someMethod(); if(ret.isErr().equals("1")) { // エラー処理 }
— みずぴー (@mzp) 2013/12/1

これには、いくつもの問題が混在しています。こういう、どこをとっても問題だらけというのも、クソコードの特徴のひとつかもしれません。

まず1つ目の問題は、エラーかどうかの判定に結果コードを返していることです。基本的にJavaでは、結果コードでエラーかどうかを判定するのはあまり望ましくありません。代わりに、Javaが持つ例外機構を使います。

Javaの例外の仕組みをよく知らない人が設計すると、結果コードを使ってしまうのかもしれません。

次のようなケースであれば、結果コード的なものはOKかもしれません。

HTTPレスポンスのステータスコードのように、外部に既にあるコードを使う場合。
Set#add(Object)が返すboolean（追加した要素が含まれていなければtrue、既に含まれていたらfalse）のように、どちらが成功でエラーかという区別をしない場合。

このクソコードには、もうひとつ「命名が慣習に違反している」という問題があります。

Javaでは普通、booleanを返すメソッドはisXXX、canXXX、hasXXX、containsXXXなどというように、疑問詞（Yes/Noで答えられる動詞や助動詞）のように命名します。逆に、booleanメソッドでないメソッドでそのような名前をつけるのは、混乱を招くため、避けるべきです。

ご覧のとおり、isErrというメソッド名でありながら、文字列で結果コード（おそらく"0"が成功で、"1"や"2"などがエラー）を返しているようです。紛らわしいので、すべきではありません。さらに細かいことを言うと、Javaではクラスやメソッドやパッケージの名前などでは、Errなどと略さずにErrorなどど命名すること慣習です。また、結果コードが1などと数値でありながら、文字列を使っているのも問題です。

なお万が一の可能性ですが、isErrメソッドがbooleanのラッパーオブジェクトであるBooleanインスタンスを返しており（それゆえequalsメソッドを持ち）、equalsでStringの"1"と比較してしまっている可能性もゼロではありません。もしそうだとしたら、さらにクソです。

まず、あるメソッドが結果コードを返すように作られているかどうかは、残念ながら、静的検査ツールで見つけだすことは難しいようです。

もうひとつの問題、メソッドの名前がおかしいかどうかであれば、検査することはできます。

IntelliJ IDEA 13.0.1	Checkstyle 5.7	FindBugs 2.0.3 with fb-contrib plugin 5.0
Naming convention > Boolean method name must start with question word 命名慣習 > booleanメソッドの名前は、疑問詞で始まらなければならない。 Naming convention > Non-Boolean method name must not start with question word 命名慣習 > booleanメソッドでないメソッドの名前は、疑問詞で始まってはならない。	なし	なし

IntelliJ IDEA 13.0.1

Checkstyle 5.7

FindBugs 2.0.3 with fb-contrib plugin 5.0

Naming convention > Boolean method name must start with question word
命名慣習 > booleanメソッドの名前は、疑問詞で始まらなければならない。

Naming convention > Non-Boolean method name must not start with question word
命名慣習 > booleanメソッドでないメソッドの名前は、疑問詞で始まってはならない。

なし

IDEAの標準では、次の動詞または助動詞が疑問詞としてデフォルトで登録されています。

is
can
has
should
could
will
shall
check
contains
equals
add
put
remove
startsWith
endsWith

広すぎる例外

throws Exceptionとかかくなーーーーー
— みずぴー (@mzp) 2013/12/27

おそらく、次のようなコードをいっているのでしょう。

public void foo() throws Exception {
    // いろんな検査例外が投げられる
}

このクソコードの問題は、メソッドが一体どのような例外を投げる可能性があるのか、具体的なことが分からなくなってしまうことです。各例外ごとにcatch節を分け、それぞれに適した例外処理をすることが難しくなります。

次のように、投げられる具体的な検査例外をひとつひとつ明示するよう書きなおすべきです。

public void foo() throws IOException, SQLException {
    // 省略
}

実際に投げられる例外とthrows句を比較するので、このような問題は静的検査ツールの得意とするところです。

IntelliJ IDEA 13.0.1	Checkstyle 5.7	FindBugs 2.0.3 with fb-contrib plugin 5.0
Error Handling > Overly broad 'throws' clause 例外処理 > あまりに広いthrows句	Coding Problems > Illegal Throws コーディングの問題 > 不正な例外スロー	なし

広い例外が許されるときもあるでしょう。例えば、インターフェースや抽象クラスのメソッドを定義する際、何らかの例外を投げる可能性がある（が実装するまで分からない）ときには、throws Exceptionとしても許されることはあるでしょう。java.util.concurrent.Callableクラスのcallメソッドが、その例です。

ただ、ライブラリやフレームワークではそういうこともあるでしょうが、普通のアプリケーションでは、よくないと思います。

例外の無視

} catch(Exception e){ throw new MyException(); } というコードを見て呆然としている。書いたやつを殴っても許されるのではないか。
— みずぴー (@mzp) 2014/1/4

殴るのはともかく、非常に困ったコードですね。発生した例外を捨て、新しい例外を投げています。これだと、キャッチされた例外が持っていたスタックトレースが失われてしまい、例外の分析が困難になります。絶対にやってはいけません。

「キャッチした例外を無視してはいけない」とはよく言われますが、これを「catch節が空ではいけない」と解釈し、「MyExceptionを投げているからOKだろう」と思っているのかもしれません。

} catch (Exception e) {
    // do nothing
}

IntelliJ IDEA 13.0.1	Checkstyle 5.7	FindBugs 2.0.3 with fb-contrib plugin 5.0
Error Handling > 'throw' inside 'catch' block which ignores the caught exception 例外処理 > catch内のthrowが、捕まえた例外を無視している	なし	fb-contrib > LostExceptionStackTrace

例外クラスを定義するときは、次のように、発生元となった例外を引数にするようなコンストラクタを定義するとよいでしょう。

public class MyException extends RuntimeException {
    public MyException(String message, Exception cause) {
        super(message, e);
    }

    public MyException(Exception cause) {
        super(cause);
    }

    public MyException(String message) {
        super(message);
    }

    public MyException() {
        super("default message");
    }
}

課題

残念ながら、静的解析ツールといえど、万能ではありません。

実際、Mが出くわしたいくつかのクソコードは、残念ながら紹介したツールでは、いまはまだ検知できないようです。ツールのプラグインを書くことでできるようになるかもしれませんので、将来の課題としておきましょう。

一応、どういったものがあるか見ておきましょう。

setterメソッドがセット以上のことをしている

新しく値を作って返すsetXXXというメソッドを読みながら静かに泣いている
— みずぴー (@mzp) 2013/12/4

おそらく次のように、setterメソッドが値を返しているのでしょう。

public String setFoo(String arg) {
    this.someField = arg;
    return "foo" + arg;
}

JavaBeansの仕様1.01では、setterメソッドの戻り値はvoidとすることが標準とされています。慣習に違反しているので、値を返すことをやめるか、メソッド名をもっと適切な名前に変えるべきでしょう。

さすがに、次のような、setterメソッドですらない、なんてことは……ないですよね？

public String setFoo() {
    return "foo" + someField;
}

変数名が変数の型にふさわしくない

String isEditable = "true" #やめてくれ
— みずぴー (@mzp) 2013/12/15

止めて（辞めて？）欲しいですね。

isEditableという変数名からは、普通、その変数の型がbooleanであると期待します。ところが実際はStringです。そうかと思うと、文字列として"true"を渡しています。一体、何がしたいのでしょうか…？ひょっとすると、booleanを知らないでプログラミングしているのかもしれません。Codeacademyなどで勉強するといいのではないでしょうか。

残念ながら、変数名から予想される変数の型が、実際の変数の型と合わないことを検知するルールは、見つかりませんでした。

その代わりに、変数に入れる値を次のようにメソッドにしている場合には、最初の方で紹介した「メソッドの命名が慣習に違反している」ことをチェックするルールを使うことができそうです。

private void caller() {
    String editable = isEditable();
    System.out.println(editable);
}

// メソッド名からbooleanを返すべきだが返していないことをチェックできる
private String isEditable() {
    if (someCondition()) {
        return "true";
    } else {
        return "false";
    }
}

まとめ

Intellij IDEAを使えば、Mが出くわしたような、実に様々なタイプのクソコードを検知できそうです。さらに、ここで紹介したチェックルールは、IDEAが提供するもののごく一部に過ぎません。実に多様なクソコードを検知するルールが用意されれています。また、各ルールは、様々なオプションで細かく制御することができます。

CheckstylesやFindBugsも、IDEAほどには強力ではないですが、なかなか捨てたものはありません。ここで紹介していないルールが役立つこともあることでしょう。それに、FindBugsは自分でルールを追加できるようですから、自分が出くわしたバグパターンやクソコードをプラグイン化として作ってみることで、より一層使いやすいものになることでしょう。

ということで、Mくん、職場のコードレビューにはIntellij IDEAを使ってみたら、捗るんじゃないでしょうかね！　え、転職するって？

このブログを検索

I am programmer and proud

そのクソコード、Intellij IDEAでチェックできるよ

コードの問題を見つける静的解析ツール

静的解析ツール vs 実世界のクソコード

広すぎる変数のスコープ

無意味な初期化

メソッドの命名が慣習に違反している

広すぎる例外

例外の無視

課題

setterメソッドがセット以上のことをしている

変数名が変数の型にふさわしくない

まとめ