背景
私は３年目エンジニアです。３年目になると、コードレビューをする側に回ることもあり、
コードレビューをする側とされる側、両方経験して、思うことがあります。
コードレビューをする側
- 体裁面や命名規則などケアレスミスを指摘したり、そこまで細かく全てのコードをレビューすることは工数がかかる
- まだ私自身経験が浅いため、チェックリストのようなものがないと、指摘漏れ、観点もれがある
コードレビューされる側
- ケアレスミスを指摘されると申し訳ない
このような経験があり、
- 実際に私が受けた指摘
- 良いコード／悪いコードで学ぶ設計入門
- ネット上の有益な記事
から得た知見をもとに、自分なりにレビューチェックリストを作成しました。
共通
- 仕様書通りに実装できているか
体裁面
- 警告やエラーが出ていないか
- デバッグ用のコードが紛れていないか
- インデントがずれていないか
- フォーマッターを実行したか
命名規則
- メソッド名
- ファイル名
- 変数名
に対して確認する。
体裁が整っているか、規約に反していないか
- スペルは正しいか
- コーディング規則に従った形か
より良い名前はないか
- 具体的で意味範囲の狭い名前か
- 限定的なスコープ以外でtmp,result などの汎用的な命名をしていないか
- 名前から予想される処理と実際の処理が異なっていないか
- 略語の使用は適切か、必要以上に省略していないか
定数
- マジックナンバーが書かれていないか
- 定数とグローバル変数以外でfinalを使っていないか
クラス
- コンストラクタで初期化されているか
- 不要な値はガード節で取り除いているか
- （ドメイン駆動であれば）データと処理ロジックを同じクラスに記述して高凝集にしているか
- 単一責任原則を守っているか
- 継承と委譲を使い分けているか
- 例外を握りつぶしていないか
- 他のクラスと疎結合になっているか
メソッド
共通
- 関数内で早めに処理が抜けられるものはreturnで適切にガード節が作られているか
- ひとつの関数・メソッドが大きすぎないか、複数のタスクを処理していないか
- 関数名を逸脱した複数のタスクがひとつの関数に書かれていないか
- 変数が適切なスコープで宣言されているか
- 行数が長すぎないか（30行以内）
- すでに実装している処理は、標準APIで実装できない処理か
- 処理件数が多い処理ではパフォーマンスを考慮した実装ができているか
引数
- 引数にnullを渡していないか
- フラグ引数を使っていないか
- プリミティブ型以外を渡すことができないか
- 引数の数が多すぎないか（4つ以下が理想的）
戻り値
- nullを返していないか
- プリミティブ型以外を返すことができないか
条件分岐
- 早期リターンをしているか
- ガード節を有効に使えているか
- 複雑な分岐の場合はinterfaceの利用を考慮しているか
- 三項演算子によってコードが理解しづらいものになっていないか
- switch文やif-elseの連鎖が長すぎないか
コレクション
- 自前のコレクション処理を実装していないか、標準APIで同じ処理ができないか
- 配列以外にもタプル、ハッシュ（ディクショナリ）など、他のデータ型を検討したか
- コレクションの操作は効率的か
実装漏れや二重実装などの観点
- チーム内で横展開されていることが反映されているか
- すでに存在する共通処理を自分で実装していないか
- エッジケースや例外的なシナリオが考慮されているか
ファイル
- 絶対パスが使われていないか
- ファイルはクローズされているか
コメント
- コメントはコードの意図を明確に説明しているか
- 複雑なロジックには適切な説明が付けられているか
- コードから読み取れる事をコメントにそのまま書いていないか
- 定数にはコメントが書かれているか
セキュリティ
- センシティブな情報（パスワード、APIキーなど）がハードコードされていないか
- 入力値のバリデーションが適切に行われているか
- SQLインジェクションやXSSなどを考慮した実装ができているか
- 出力値は正しいエンコード処理がされているか
パフォーマンス
特に一括〇〇など扱うデータ件数が多い場合には計算量を意識した実装を行う
- 不必要なループや処理がないか
- 大量のデータを扱う場合、メモリ使用量は適切か
- データベースクエリは最適化されているか
- N+1を発生させる実装はないか
参考資料
良いコード／悪いコードで学ぶ設計入門 ―保守しやすい 成長し続けるコードの書き方
プログラマー歴20数年の私が考えた、コードレビュー依頼前のセルフチェックリスト - Qiita