インパス！あのこれダメッス！

～Javaコードレビューの指摘ポイント10選～

JJUG ナイト・セミナー2017/08/23

自己紹介

• 株式会社ジャストシステム　福嶋 航• Twitter @fukushiw• Java歴約20年、JavaでWebサービス作っています• #Java100 本ノックの人https://github.com/JustSystems/java-100practices

インパス！あのこれダメッス！

インパス！

あのこれダメッス！

1.instanceof2.parseInt / parseLong / toString3.substring4.unchecked parameters on public methods5.non-private fields6.code convention ignored7.resources without try-with-resources8.Date / Calendar / SimpleDateFormat9.methods over 30 lines / classes over 1000 lines10.throws RuntimeException / throws Exception

コードレビュー指摘頻出ポイント10

1.instanceof2.parseInt / parseLong / toString3.substring4.unchecked parameters on public methods5.non-private fields6.code convention ignored7.resources without try-with-resources8.Date / Calendar / SimpleDateFormat9.methods over 30 lines / classes over 1000 lines10.throws RuntimeException / throws Exception

コードレビュー指摘頻出ポイント10

インパス！あのこれダメッス！

1. instanceof

【匂い】これを使うということはポリモーフィズムをうまく使えていない可能性がある。

【確認ポイント】ドメインオブジェクトにあるべきロジックがコントローラーやサービスクラスにないか？

2. parseInt/parseLong/toString

【匂い】これを使うということは値オブジェクトを使わずに何でも文字列で処理しようとしている可能性がある。

【確認ポイント】型や形式の変換処理があちこちにないか？本当に文字列で扱う必要があるのか？

3. substring

【匂い】これを使うということは日付や数値の変換処理などコアAPIにあるものを自前で実装しようとしている可能性がある。

【確認ポイント】型や形式の変換処理があちこちにないか？本当に文字列で扱う必要があるのか？

4. unchecked parameterson public methods

【匂い】メソッドを呼ぶ順序など前提に暗黙のルールがある可能性がある。

【確認ポイント】APIドキュメントに詳細な説明があるか？NullPointerExceptionやIndexOutOfBoundsExceptionが起きないか？

5. non-private fields【匂い】定数以外でprivateではないフィールドがある場合、そのクラスのカプセル化が不十分で変更に弱いコードの可能性がある。

【確認ポイント】本来そのクラスにあるべきロジックが他のクラスに存在していないか？クラスの役割が不明確になっていないか？

6. code convention ignored

【匂い】クセのあるコードが生産されメンテナンスできなくなる可能性がある。

【確認ポイント】守られない理由は何か？ （ツール、環境、…）不条理なルールはないか？

7. resources withouttry-with-resources

【匂い】リソースの解放漏れによりリソースリーク（メモリ、ファイルディスクリプタ、…）の可能性がある。

【確認ポイント】リソースの生存期間が明確か？ (通常1メソッド内)Java 7 以降にアップデートできないか？

8. Date / Calendar /SimpleDateFormat

【匂い】マルチスレッド動作時や防御的プログラムでない場合にバグが出現する可能性がある。

【確認ポイント】Mutable考慮済/シングルスレッド前提か？Java 8 以降にアップデートできないか？

9. methods over 30 lines /classes over 1000 lines

【匂い】密連携されたメンテナンスが困難なコードの可能性がある。

【確認ポイント】メソッドやクラスの役割が大きすぎないか？業務とコードが対応できているか？

10. throws RuntimeException /throws Exception

【匂い】例外設計をサボっている可能性がある。

【確認ポイント】メソッド内で消化すべき例外をスローしていないか？例外をそもそも考慮しているのか？

1.instanceof2.parseInt / parseLong / toString3.substring4.unchecked parameters on public methods5.non-private fields6.code convention ignored7.resources without try-with-resources8.Date / Calendar / SimpleDateFormat9.methods over 30 lines / classes over 1000 lines10.throws RuntimeException / throws Exception

コードレビュー指摘頻出ポイント10

インパス！あのこれダメッス！