【Java】ソースレビューを依頼する前にセルフチェックできること【初心者】

はじめに

初心者～３年目くらいの子たちのソースレビューをする機会に恵まれたんですが、「毎回、同じこと指摘してるなー」と。
しかも、同じ人に同じことを３回くらい・・・
なので、職場のwikiにまとめたら、それ以降そのwikiがソースレビュー受ける前の「自己レビュー基準書」になりました！
というわけで、案件固有の表現を抜いた、汎用ver.を公開します。

おことわり

あくまでも、私がレビューしたときに多かった指摘事項です。
本質とずれていても、経験年数を考慮して「ひとまず、こういうふうに覚えてね」という説明にしてることも多々あります

あと、サンプルソースは、あくまでも「各指摘事項について」示したものです。
いわゆる「イケてる・イケてない」という議論は、ここではしません。

前提条件

• eclipseを使用していること。
• SVNやgitなど何かしらのバージョン管理ツールを使用していること。

Javaのソースレビューを出す前に確認してほしいこと

コードフォーマッタはかけましたか？

eclipseのショートカットCtrl + Shift + Fを叩いて、ソースのフォーマットを整えましょう。

細かいことですが、インデントに使用するのはスペース or tabといった部分まで規約にまとまっているので・・・。
それと、ショートカットは 自分が修正した箇所を選択してから 実行してください
他人のソースもフォーマッタかけてしまうと、SVNやgitで差分取った時に、真の修正箇所がわかりづらくなるからです。

import文の整理は行いましたか？

eclipseのショートカットCtrl + Shift + Oを叩いて、不要なimport文は削除しておきましょう。

「別に、残ったままでもバグが出るわけじゃないしぃ～」って思うでしょうけど、ただのゴミなので。
ゴミはきちんと削除しましょう。
他人が残した未使用importも、ショートカット一発で消せるので、ついでに消してしまいましょう。

クラスの先頭で定数を定義していませんか？

DBで使っているようなマスタ値や、ON/OFFなどを意味する定数は、すでに誰かが定義済みかもしれません。
Enumだったり、定数専用のクラスを作っていたり、プロパティファイルに書いていたり・・・。
まずはそれらを確認し、定義済みであれば積極的に使いましょう。
必要であれば、新たに定義して下さい。

マジックナンバーは撲滅しましたか？

マジックナンバー（＝ソースの中にべた書きされた固定値）は、メンテナンス性に欠けます。
可能な限り、定義済みのものを使いましょう。

実装中に「とりあえずべた書きしちゃえ」って書いちゃったものが、そのまま残っていることが多々あります。
上記「クラスの先頭で定数を定義していませんか？」と同様に、定義済みのものを使用するようにしてください。

ちなみに、個人的には「思考が邪魔されない、中断しない」という理由で、実装の過程ではあえてべた書きすることもあります。
・・・が、ちゃんと最後にマジックナンバー撲滅してます

JavaDocはちゃんと書きましたか？

最低限、以下の３つを確認しましょう。

• 引数は過不足なく書いてあるか。
• メソッドの説明は適切か。
• 戻り値の説明は正しいか。

引数が増えたり減ったりした時は、つい忘れがちです。引数の説明が正しく書いてあることを確認しましょう。

よくあるのが、「最初にメソッドを作った時に全てきっちり書いたが、最終的には少し書き直して違うものになった」ときです。
引数の数がバラバラになっていたり、引数やメソッド内容の説明が微妙におかしなことになりやすいです。
お願いだから、嘘は書かないで・・・

ところで、JavaDocコメントは、Javaコマンド一発でhtmlドキュメントとして出力できます。
ドキュメントとしてお客さんの手に渡っても問題ないかどうか・・・を意識すると良いと思います。

使ってない変数・引数・メソッドは残っていませんか？

使ってない＝ゴミコードです。ゴミはきちんと削除しましょう
たいてい、すごく後になって「これ使ってるの？」ってガヤガヤした結果「なんだ、ゴミじゃん」ってなるので。

使っていない変数

eclipse上で黄色くwarning が出ていると思います。消してしまいましょう。

使っていない引数

黄色いwarning は出ません。見付けるのは至難の技です。
1つずつカーソルを当てて、他にハイライトされる箇所があるかどうかを確認し、使っていなければ削除しましょう。

使っていないprivateメンバ(フィールド、メソッド)

eclipse上で黄色くwarning が出ているはずなので、ばっさり消してください。

メソッド名・変数名は適切ですか？

メソッド名・変数名は、そのモノを正しく表していますか？
returnで値を返しているメソッドなのに「setXXX」とか、何も数えてないのに「count」という変数とか、間違った意味の命名はやめてください。

既存ソースで同じ処理をやっているメソッドはいませんか？

改修案件の場合は、まず「既存処理」を探してみましょう。

オブジェクトをごっそりCRUDするような処理は、たいてい既存メソッドが存在します。
各種フラグのtrue/false判定処理なども、既存ロジックがどこかにあります。
保守性向上のために、同じ処理を2つ作ってしまわないよう、確認をお願いします。

forループやif-elseの中でしか使わない変数を外側で宣言していませんか？

Javaはスコープ（＝使える範囲）が小さい方がよしとされます。
なので、forループやif-elseの中でしか使わない変数は、その中で宣言しましょう。

// 悪い例
String str = new String();
for (int 0 ; i < 10 ;i++) {
  str = i + "番目"
  System.out.println(str);
}

// よい例
for (int 0 ; i < 10 ;i++) {
  String str = i + "番目"
  System.out.println(str);
}

これも、結構繰り返し指摘してるので、別記事書いてます 【Java】ループ内で使う変数はループの中で宣言しよう【ブロック内の変数】

if文の入れ子は多すぎませんか？

例えば「A かつ Bのとき」は以下のように2つの方法で記述可能です。

// 記述1
if (Aのとき) {
  if (Bのとき) {
    // 処理を行う。
  }
}

// 記述2
if (Aのとき && Bのとき) {
  // 処理を行う。
}

意味が同じであれば、入れ子が少なくなる（＝記述2の）ようにしてください。
もうね、読むの大変なんです
多分３か月後くらいの自分が読んでも記述1のパターンはイラッとすると思います。イラッとしない書き方でお願いします。

Stringのnull／空チェックを自前で実装していませんか？

org.apache.commons.lang.StringUtilsを使いましょう。

たいていの案件では、org.apache.commons.lang.StringUtilsというapacheのライブラリが組み込まれているんじゃないでしょうか？
便利なので、Stringのnull／空チェックは自前で実装せず、ライブラリで用意されているメソッドを使ってください。

用意されているメソッドの一例

• StringUtils.isEmpty()
• StringUtils.isNotEmpty()
• StringUtils.isBlank()
• StringUtils.isNotBlank()
• StringUtils.equals()
• StringUtils.contains()
• StringUtils.isAlpha()
• StringUtils.isNumeric()

Listのnull／空チェックを自前で実装していませんか？

org.apache.commons.collections.CollectionUtilsを使いましょう。

これも、たいていの現場ではorg.apache.commons.collections.CollectionUtilsというapacheのライブラリが組み込まれていると思います。
Listのnull／空チェックは自前で実装せず、ライブラリで用意されているメソッドを使ってください。

用意されているメソッドの一例

• CollectionUtils.isEmpty()
• CollectionUtils.isNotEmpty()
• CollectionUtils.exists()

こちらも参考にどうぞ 【Java】リストの要素がnullもしくは空であることをチェックしたい【CollectionUtils】

コメントはウソついてませんか？

あれこれ修正してると、コメントと実装の内容がズレてくることもあります。
実装内容にあったコメントになっているかどうかを確認してください。
まぁ、たいてい実装だけ直してコメントを放置しているんですけどね・・・。

コピペしたそのソースは本当に正しいですか？

例えば、コピペ元が間違っているのに、そのままコピペっていいですか？バグを量産するだけですよね？
コピペるのは構わないですが、ちゃんと自分の頭で考えたうえで「だから、私はここをコピペした」って言えるようにしておいてください。
「こういう記述をしてる人がいたから」とか「こんなサンプルがあったから」はNGです。

とりあえずの「public」にしていませんか？

publicにすれば、どこからでも参照できます。逆に言うと、不要な場所からも参照できてしまいます。
よくわからなければ、ひとまずprivateにしておいてください。
必要になってからprotected→publicと広げていけばいいのです。

そのソースは「なぜそのように書いたか」理由が説明できますか？

「他のところがこのように書いていたから」はNGです！

既存ソースを真似るのは、プロジェクト内のソースコードの統一性を出し、これからの保守性を高めていくためには必要なこともあります。
しかし、ただ真似てしまうと、未知のバグを拡散させてしまったり、不要な処理を作り込んでしまったりするなど、バグの温床にもなります。

真似するときは、自分なりの理由・根拠を持った上で、真似して下さい。
そして、「真似した処理は何をしているのか」を論理的に説明できるようにしておいてください。

おわりに

なんか、すでにいろんな人が書いていて、この方なんか「まんま、私と一緒だな！」って思いました。
Javaを書くときに意識してほしいこと
つまりは、新人さんは皆同じ過程をたどる・・・と。

だからこそ！
ここに書いてあることを１つでも２つでも実践していってほしいなぁと思います。