【チェックシート】システム開発において、Java11・SpringFrameworkで実装した際のコードレビューでよくある指摘観点をまとめる

はじめに

SAP社のエンタープライズPaaS製品である、SAP Business Technology Platform（BTP） でアプリケーション開発をしている@fussasyです。開発プロジェクトを経験する中で、チームメンバーのコードレビューをする機会が多くありました。やはり、コードレビューでは似たような指摘も発生してしまい、手間に感じている部分もありましたので、まとめてしまいたいと思いました。
当記事ではレビュイーがコードレビューに向かう前の事前チェック資料、もしくはレビュアーがレビューをするための参考資料となるような一覧整理資料を目指しています。アプリケーション開発する際、品質の向上に繋がれば嬉しく思います。なお、この記事については随時、更新すると思います。

チェック観点

Java11 各社ガイドライン

基本的に以下を利用すれば良いと思われる。ただ各プロジェクトにおいて、コーディング規約が指定されている場合は、その規約に従うこと。
・フューチャー株式会社（Future Enterprise Coding Standards）
https://future-architect.github.io/coding-standards/
・フューチャー株式会社（Future Enterprise Coding Standards > Javaコーディング規約）
https://future-architect.github.io/coding-standards/documents/forJava/
・Google Style Guides
https://google.github.io/styleguide/
・Google Style Guides（Java）
https://google.github.io/styleguide/javaguide.html

Java11（Spring Framework）非推奨API
・Oracle社
https://docs.oracle.com/javase/jp/8/docs/api/deprecated-list.html

仕様・規約とのつきあわせ

提供されている「命名規約」と「コーディング規約」に従って記述していること
コーディングの際、仕様が不明瞭なものについては、関係者にQAを実施し回答を刈り取っていること
コーディングの際、関係者にQAを実施し、回答を刈り取ったものについて、証跡を一覧として提示できること
コーディングの際、スケルトンを利用する場合には、バグ発生時における責任の所在が明確になっていること
詳細設計書（基本設計書）に記述された処理をコーディングしていること
詳細設計書（基本設計書）について、レビュアに対してコードへの反映箇所を説明できること
詳細設計書（基本設計書）と突き合わせがしやすいよう、コメント文を記述したこと

単体テスト観点とのつきあわせ

単体テスト仕様書がある場合、各テスト観点については事前にデバッグで検証していること

よくある指摘

※以下、基本的には先述のガイドラインに記載内容となりますが、ヌケモレが多い箇所をピックアップしています！

不要なコード、コメント、メソッド、クラス等は残っていないこと
開発時のデバッグコードは残っていないこと
警告（エラー）箇所は消えているか。消えていない場合、デプロイしても問題ないか確認していること
警告（エラー）は、フロントエンド側かバックエンド側のどちらで出力するか、関係者にQAを実施し回答を刈り取っていること
フィルタやソートについては、フロントエンド側のフレームワーク機能での実装を優先していること
例外処理に関するアンチパターンに該当していないこと
※https://qiita.com/ts7i/items/d7f6c1cd5a14e55943d4
　・例外処理として、エラーを握りつぶしていないこと
　・例外処理として、エラーを握りつぶしていないか、それをスタックトレースやログを申し訳程度に出力していないこと
　・例外処理として、自前でロギングしてから再送出していないこと
　・例外処理として、誤った例外を送出していないこと
　・例外処理として、複数の例外に対して同じ処理を繰り返していないこと
　・例外処理として、nullをreturnしていないこと
　・例外処理として、別の戻り値を返していること
　※アノテーションを用いて例外クラスを指定も可能
例外ログ（「java.util.logging」もしくは「SLF4J + Log4j2」）において、適切なレベルで出力して運用していること
1行1文の記述であること
return文は、「return()」としないこと（「return;」「return 1;」「return value」等）
for文は、初期化・条件・更新を使用する際、コンマ演算子(,)を利用した変数の規約を守っていること
for文は、個別のカウンタを使用しないこと（for文前か、またはforブロックの最後に分けて記述）
for文は、初期化のループカウンタは、基本的にゼロからスタートさせていること
for文は、ループ中にカウンタの値を意図的に変更（値を代入）していないこと
while文は、無限ループを起こしていないこと（break存在チェック）
do-while文は、無限ループを起こしていないこと（break存在チェック）
switch文は、break 文のないcaseの場合には、「/* 次の行も実行*/ 」といったコメントを明記していること
switch文は、defaultを必ず記載すること（「default→break」することで新たにcaseを追加する場合にbreak 付け忘れを抑止したい）
try-catch文は、ループ内で利用は避けて、try-catchの中にループを記載していること（レスポンス向上）
try-catch文は、フロー制御のために利用しないこと（バグ抑止）
ファイルやデータベース接続などのリソースのオープン処理を実行した場合、メモリリークを避けるべくクローズ処理を実施していること
Javadocコメントは規約に沿っていること（日本語、ですます記載、機種依存文字の回避　等）
1クラスは1ソースファイルの定義が基本であること
非推奨APIを利用していないこと
クラスやメソッドのアクセス修飾子の宣言は適切であること（基本privateを使用し、アクセス制御を緩和する場合はprotected～publicを検討）
プリミティブ型と参照型の違いを意識して扱っていること
文字列は、文字リテラルをハードコーディングをしていないこと（毎回インスタンスが生成されるため）
文字列の連結は、java.lang.StringBuilderクラスを利用していること（「String hoge = "京都" + "宗太郎";」とかはダメ、毎回インスタンスが生成されるため）
文字列の比較は、Stringクラスが参照型なので比較する場合、「==」や「!=」を使用せず、「文字列.equals(文字列)」を使用していること
文字列のキャストはClassCastExceptionの発生を防ぐため、型を確認せずにキャストを行うことは避けること
可読性として、無駄な空行をいれていないこと
可読性として、適切に半角スペースを入れていること（「(」の後、メソッド・引数を区切る「.」、演算子、「;」、キャストの後　等）
可読性として、クラス・メソッドには処理内容を示す名称をつけていること
可読性として、行を折り返す際は「コンマ後」、「演算子前」、「hi-level（×low-level）」、「3つめ～は2つめと左端を合わせる」、「半角スペースで調整」等を意識していること
可読性として、数式を分割する際には()で囲まれた部分(hi-level)は外して分割していること
可読性として、if 文での行の折り返しの際には1レベルのインデントだとブロック内部本体の見通しが悪くなるため、2レベルインデントとしていること
ローカル変数の初期化は、宣言と同時に実施していること
変数は可能な限り減らしていること
変数宣言時、変数名は使用箇所や意図が分かる名前になっていること（「id1」「id2」等になっていないこと）
変数宣言時、変数名は似たような名前を多用していないこと（見間違いの原因となる）
変数宣言時、明示的に型を記載していること
変数宣言時、1行1宣言としていること
変数宣言時、出来る限り関連する処理の手前で行うこと（メソッドの先頭でローカル変数を全て宣言するのは見辛い）
再代入や再宣言をしていないこと
定数の宣言にはインスタンス生成を一度に限定するため、必ずstatic修飾子を付与していること
enumクラスの作成は基盤部品、共通部品に限っていること
クラス理解を容易にするために、可視性は高→小の順に宣言していること
メソッドの引数が多すぎる場合は、引数をオブジェクトで置き換えていること
@ Override アノテーションをメソッドの先頭に記載する。オーバーライドされていない時は、コンパイルエラーを出力していること
ユーティリティクラスのpublicメソッドは、引数の正当性を検証すること。引数が不正だった場合は、「lllegaArgumentException」等の例外にしていること
メソッド呼出元で意図しなかったり、String等の不変クラスの場合、新しいインスタンスが作成されメソッド実装者との認識の相違が生まれるため行わないこと
ジェネリッククラスの作成は、基盤部品、共通部品のみに限っていること
Import文の表記として、クラス名を「*」で省略が規約でNGの場合もあるため確認していること
※なお、Importするクラス群は、以下でまとまっていると良い
　・標準JavaAPIに属するクラス群
　・利用するフレームワーク等に属するクラス群
　・サードパーティあるいはオープンソースプロジェクトなどが提供するクラスライブラリに属するクラス群
　・開発プロジェクトで、個別に作成しているクラス群
文字コードは UTF-8 を使用していること
インデントは統一していること（規約も確認すること）
1行あたりの処理の文字数を確認していること（規約も確認すること）
関数の処理の一連の流れが、長すぎる状態となってないこと（規約も確認すること）
配列の添え字を指定した場合、必ず想定のデータがそこに入ってくるか確認していること
条件分岐処理で、例えば複数の要素（AとB）で条件を指定する場合、AとBを並列で条件処理とするか、A→Bの順で条件処理とするか誤っていないこと
条件分岐処理において、条件に使用する比較演算子は可読性向上のため「<」「<=」のみ使用していること
条件分岐処理において、「0」か「1」でしか判断しないのであれば、「TRUE」か「FALSE」にしていること
条件分岐処理において、正常系＝「TRUE」、および異常系＝「FALSE」でチーム内メンバー全員にて、統一していること
条件分岐処理において、ネストは限界まで浅くしていること（深さ＝6以上になると可読性が悪化）
条件分岐処理において、必須チェック後に処理から抜けないことにより、二重エラーを発生させていないこと
条件分岐処理おいて、空文字一致の際には「isEmpty」を利用していること（「.equal(XXX)」等をしない）
比較処理において、定数を前後どちらにするか、チーム内メンバー全員にて、統一していること
暗黙の型変換が発生している箇所はないこと
有効桁数を超えた時のPOST処理は考慮していること
引数と返値に同一の配列を使用していないこと
特殊符号の編集時には、全角・半角を考慮していること
無意味なコメントは入れていないこと
冗長処理になっている箇所はないこと
可能な限り、引数に配列を渡していないこと

おわりに

色々と書きましたが、結局のところJavaでバックエンド側を開発する場合、冒頭にて紹介したフューチャー株式会社様が提供しているコーディングガイドライン（Future Enterprise Coding Standards）を読み込むのが早いと思います。実際、弊社にて命名規約やコーディング規約を作成する際には、当資料を多分に参考にさせてもらいました。また、レビュー以前に各種フォーマッターにこれらの設定を盛り込んでおけば、細かくコードを見る必要もなくなるので開発効率も上がると思います。
そして、技術者人口の最も多いJavaで開発するという点において重要なことは、あらゆる技術レベルの人々によって見られ続ける、ということを意識して書くことだと思います。つまりは、イケてるコードよりも、新卒でも分かるような丁寧なコードの方が現場のニーズが高い、ということですね。