Java初心者だった新入社員の頃、先輩にコードレビューで指摘された事を思い出してまとめてみた。

追記：本記事に関しては賛否含め、多くの有益なコメントを頂いています。本記事をお読みになる際は、是非コメント欄も併せてご覧下さい。

2018/04/26
コメントを参考に「何でも定数にしようとする」の見出し・本文を修正しました。@kagilinn さん、ありがとうございました。
2018/04/30
サンプルコードの判定バグってたので修正しました。@y_miz さん、ご指摘ありがとうございました。
コメントの誤記、用語誤りを修正しました。@scivola さん、編集リクエストありがとうございました。

不要なインスタンス変数を作ってしまう

インスタンス変数は状態が保持されるので、バグを作り込みやすい。
「これローカル変数でよくない？」ってよく指摘された。

インスタンス変数を作る前に、ローカル変数で実現できないか検討する。

メソッドの先頭で全ての変数を宣言しようとする

C言語から入った人によくある習慣らしい。
変数のスコープが不必要に広がる為、トレースが非常に面倒。

悪い例

public String function() {
    int count;
    int max;
    int min;
    List fooList;
    String hogeCode;
    String fugaCode;
    String ret;
    
    // 処理いろいろ

    return ret;
}

変数の宣言はスコープを意識し、必要になったタイミングで行う。

説明不足な変数名をつける

初心者あるある。ベテランでも意外と適当な人がいたりする。
初心者でやりがちなのは、型名を省略しただけのものや、書いた本人しかわからない連番など。

悪い例

String str;  // 文字列であることしかわからない

String code; // 何のコードかわからない

int a;       // 酷いけど稀に見かける

File file1;  // 謎の連番
File file2;

static final String MSGID_E0001 = "E0001"; // 定数名に値が入っている

良い例

String userName;

String messageCode;

int age;

File userListFile;
File companyListFile;

static final String MSGID_FILE_NOT_FOUND = "E0001";

ただし、何でもかんでも修飾たっぷりの変数名を付ければ良いというわけではなくて、
for文のカウンタやcatchした例外など、短い名前で良い場合もある。

変数名をどこまで詳しくするかは、スコープの長さによって決めると良い。
インスタンス変数や長いメソッドで使われるローカル変数には、詳しい名前を付けたほうが良い。逆に、変数のスコープが数行のブロックに限る場合は、短い変数名でも問題はない。

重要なのは、コードの読み手(数ヶ月後の自分を含む)の立場に立った際、「その変数に何が格納されるのか」が理解できるかどうかである。

変数の命名は簡単なようでとても奥深い世界で、
名著「リーダブルコード」でも、変数名についてかなりのページ数を使っている。

booleanの変数名にxxxflgを使う

指摘されるまで違和感を全く持っていなかったやつ。
xxxFlgだと、trueの時にどうなるのかが分かり辛い。

悪い例

private boolean writeFlg = false; // どういう場合にtrue/falseとなるのかが不明瞭

良い例

private boolean writable = false;

booleanの変数名は命題にする。
変数名 == trueと書いた時に、文として理解できるものが望ましい。

メソッド名の例

public boolean isWritable() {
    return writable;
}

上記のようなメソッド名とすることで、呼び出しの際にインスタンスが主語となり、英文として意味が通じやすい。

if (note.isWritable()) { /* ... */ }

よく使われるメソッド名には次のようなものがある。

is + 形容詞
can + 動詞原形
has + 過去分詞
三単現動詞 (+ 名詞)

【参考】booleanの命名について、英文法の観点でまとめられている記事
http://kusamakura.hatenablog.com/entry/2016/03/03/boolean_値を返却するメソッド名、変数名の付け方

意味を与えずに定数化する

「リテラルのべた書きはダメだ」という指摘を受けたので、色々定数化してしまった例。
定数が意味を持っておらず、ただ文字を置き換えただけになっている。

private static final String HANKAKU_SPACE = " ";
private static final String BLANK = "";
private static final String COMMA = ",";
private static final String SLASH = "/";
private static final int ZERO = 0;

必ずメソッドの最後でreturnしようとする

最初に指摘された時は、何が悪いのか理解できなかったが、
自分が読む立場になってみるとよくわかった。

悪い例

boolean isPrimeNumber(int num) {
    boolean ret;
    if (num < 2) {
        ret = false; // 2未満は素数でない
    } else if (num == 2) {
        ret = true;  // 2は素数
    } else if (num % 2 == 0) {
        ret = false; // 2以外の偶数は素数でない
    } else {
        ret = true; // 割り切れなかったら素数
        double sqrtNum = Math.sqrt(num);
        for (int i = 3; i <= sqrtNum; i+=2) {
            if (num % i == 0) {
                ret = false;   // 割り切れたら素数でない
                break;
            }
        }
    }
    return ret; 
}

良い例

boolean isPrimeNumber(int num) {
    if (num < 2) {
        return false; // 2未満は素数でない
    }
    if (num == 2) {
        return true;  // 2は素数
    }
    if (num % 2 == 0) {
        return false; // 2以外の偶数は素数でない
    }
    double sqrtNum = Math.sqrt(num);
    for (int i = 3; i <= sqrtNum; i+=2) {
        if(num % i == 0) {
            return false;   // 割り切れたら素数でない
        }
    }
    return true; // 割り切れなかったら素数
}

第三者が、numに1が入ってきた場合をトレースする場合を考えてみる。

「悪い例」では、どこでretが書き換えられているかわからない為、読み手はretの状態を記憶したまま、最後のreturnまで読み進めなければいけない。

一方、「良い例」では、各判定でその場でreturnしている。
num=1のケースをトレースする際は、3行目のreturnまでを読めば良い。

この例は10数行のメソッドだが、50行100行のメソッドを「悪い例」のように実装すると、読み手に掛かる負担は計り知れない。

メソッドは、戻り値が確定した時点でreturnするのが望ましい

また、早くreturnすることで、ネストが深くなりづらいというメリットもある。

不必要にpublicを付けようとする

初心者の頃は、privateで済む変数やメソッドをpublicにしがちである。
まずはprivateで作り、必要ならprotected, publicと広げていくのが良い。

安易にxxxUtil, xxxHelper, xxxManagerなどの神クラスを創造する

役割がはっきりしない名称のクラスは危険だ。
作った時点では良いが、5年10年とメンテナンスされていくうちに機能を~~押し付けられ~~追加されて手がつけられなくなっていく。

xxxの部分が抽象的であればあるほどまずい。最悪なのがCommonUtilである。
10年以上前に作られたCommonUtilを見たことがあるが、4000行を越えていた。

まずはFactory, Builder, Writer, Readerなど、役割が限定されるような名称に変えられないかを検討すると良い。

コメントを全く書かない

実装することに必死だったので、コメントを全く書かずにコードレビューに持っていったら怒られた。

コメントはちゃんと書こう。

特に以下のようなものは、コメントで補足をしておくべきだ。

難解な処理や複雑な条件分岐
コードから読み取れない特殊な経緯や意図

自明な情報をコメントとして書く

「コメントを書きなさい」という指摘を受けた後、一行一行コメントを書いたらこれも指摘された。

コードをただ日本語訳したようなコメントを一行一行書いてしまうと、逆に雑音となってしまう。画面内に収まる有効行が少なくなる為、見通しも悪くなる。

悪い例

// ユーザIDを取得する
String userId = user.getId();

// ユーザIDをリストに追加する
userIdList.add(userId);

ループ0回の考慮をしていない

for文やwhile文などのループ文を実装する際、ループが1回も回らないケースを想定しておらず、ユニットテストの0件パターンでバグが発覚するという事が何度かあった。

Foo foo = null;
for (int i=0; i < hogeList.size(); i++) {
    if (i == 0) {
        foo = new Foo();
    }
    // 処理
}
foo.function(); // ループ0回の場合はNullPointerExceptionが発生

ループ実装の際には、0件、1件、複数件のパターンを想定する

メソッドの戻り値・例外の設計を疎かにする

publicな共通系のメソッドにも関わらず、
引数や戻り値の仕様が曖昧だった。

例えば次のようなことは、漏れなく設計してJavadocに記載した方が良い。

引数でnullや負の数を与えられたら何を返すか
どの場合に何の例外をthrowするか
戻り値としてnullを返すのはどのような場合か

Javadocの模範例を知りたい場合は、OracleのJavadocを見てみると良いと思う。
馴染みのあるStringクラスのメソッドなどがわかりやすいだろう。

さいごに

新人プログラマが良いコードを書けるようになる為には、本人の努力はもちろんだが、
優秀なプログラマからコードレビューを受けられるかどうかが重要だと思う。
レビュアからの「自分ならこう書く」というアドバイスを受け、「ああ、そういうのもあるのか」という気付きを得る。その積み重ねを経てノウハウを吸収していき、より良いプログラムが書けるようになっていくからだ。

コードレビューを教育の場とすることには賛否あるが、
私個人の意見としては、コードレビューは品質向上の取り組みであると同時に、勉強会であってもよいと思っている。
自分の書いたコードに対し、他のプログラマから指摘やアドバイスを貰い、時には議論する。そのような機会はコードレビュー以外ではそうそう無いだろう。バグ指摘だけで終わらせるにはあまりに勿体無いのだ。