コードレビューを受ける前に確認しておきたいチェックリストを整理してみた
レビュアーの負荷軽減とレビュー時間の短縮を目的として 、コードレビューを依頼する前に確認しておくべきチェックリストを整理してみた*1
チェックリストは特定の言語・フレームワーク、開発内容（バックエンド・フロントエンド）に依存しない内容になるように心がけてまとめた。
以下チェックリスト
- ☑ 仕様を満たしているか
- ☑ 仕様が伝わるか
- ☑ 影響範囲を記載しているか
- ☑ 不要なコードが残っていないか
- ☑ 明らかなアンチパターンが含まれていないか
- ☑ コードのフォーマットが崩れていないか
- ☑ ユニットテストが全てパスするか
- ☑ 適切なコメントが書けているか
☑ 仕様を満たしているか
- 予め整理されていた仕様通りに実装されているか
- 仕様が抜けている・間違っているとコードレビューをしても仕方がない
- 仕様の不明な箇所を想像で実装していないか
- 非機能要件（実行環境の差異、性能、大量データ等）を考慮・想定しているか
☑ 仕様が伝わるか
- 実装した仕様は明文化されているか
- レビュアーが仕様を理解できないと形式的なコードレビューに終始してしまう恐れがある。必ずドキュメント化するべきというわけではなく、仕様が明らかでレビュアーと互いに合意が取れているのであればチケットで補足する程度でも大丈夫と思う
☑ 影響範囲を記載しているか
- 機能の追加、不具合修正等既存のコードに手を加えている場合はどこに影響するのかを記載すると、レビュアーの負荷を下げられる
- 影響範囲が不明なままコードを修正するのは止めよう
☑ 不要なコードが残っていないか
- 検証用のログ出力や処理、コメントアウトされたまま最終的には使わなかったコード等が残っていないか
- 後から人力で検出するのは辛いので、都度”TODO:後で消す”などのコメントを付与してIDEで管理するのが良い（VSCodeなら Todo Tree - Visual Studio Marketplace）
☑ 明らかなアンチパターンが含まれていないか
- 主にパフォーマンスやセキュリティの観点
☑ コードのフォーマットが崩れていないか
- インデントの崩れ、横幅の超過、不要な空白や改行はないか
- 人力で修正するのも指摘するのも辛いので自動フォーマットの仕組みを導入するべき（prettier等）
☑ ユニットテストが全てパスするか
- 新規に追加したユニットテスト、既存のユニットテストを含めて全てパスするか
- 開発途中までは通っていたはずなのにな...? ということが意外とある
- 全て実行するにはユニットテストの数が多すぎる場合は、開発箇所と関連する部分を選択する。（影響範囲を特定しきれないという場合もあるかもしれないが、それはそもそもコードを書く前に確認して明らかにするべきこと）
☑ 適切なコメントが書けているか
コメントは後からコードを読む・書く人のために書くものだが、良質なコメントはコードレビューにおけるレビューアーとのコミュニケーションを減らし、レビュー時間を短縮することにも繋がる。
クラス
- 何のために、何をするクラスなのか
- 文章化してみると実はクラス設計自体に問題があるということも少なくない
メソッド
園田：一般論として最初に検討するべき候補はあります。「（1）事前条件」「（2）事後条件」「（3）不変条件」「（4）入力の意味」「（5）出力の意味」「（6）副作用」「（7）計算量」です。要するに、利用する側が知っておくべきだけれど、コードの中身を見なければ容易には分からないものです。
Rubyコミッター・Yuguiに学ぶ、コードに書くべき「適切なコメント」と「適切な場所」 - エンジニアHub｜若手Webエンジニアのキャリアを考える！より引用。
事前条件、事後条件、不変条件という言葉に馴染みがない人はオブジェクト指向入門 第2版 原則・コンセプトの第11,12章を読み契約による設計の概念に触れてみよう
最近の自分は所謂Javadoc（関数の説明、引数・返値の内容）＋事前条件、事後条件、（必要なら不変条件）という形に帰着している
テストメソッド
- どういったケースにおける何を検証するテストなのか
- 正常系・異常系のどちらを確認しているのか
制御構文
- 特にif文の条件式の意図が伝わりにくいことが多い。条件式を自然言語に書き直すわけではなく、なぜその条件式が必要なのかを書く
- しかし基本的には、伝わりにくいコードをコメントで補うのではなくコード自体を見直すべき。式が長い場合には説明変数に代入できないか、式が簡潔だが意図が伝わらない場合は要約変数を利用できないか？（リーダブルコードの8章を参照）
その他
- 問題があることを認識しつつも時間がない、今回の修正目的の範囲を超える等の理由で修正しない場合はFIXME、WARNING等のコメントを付与
- 部分的なリファクタリングで、どうしても過去の負債を引き継がないといけない場合はその旨を書く
- コメント内容自体に誤りがないか（実装と合っているか）
*1:コードレビューの定義や仕方はチームや会社によって異なるとは思うが、このチェックリストはそれなりに普遍的な、つまりコードレビューで指摘されずにmasterブランチに含まれると問題があるような内容がほとんどなんじゃないかと思う。
