コードレビューチェックリスト
1. 可読性
メソッド/関数の行数が長過ぎないか
- メソッド/関数の行数最大30行以内、20行以下
- 30行以上のメソッド/関数は原則リジェクトし、メソッド/関数の分割を検討
- SQL関連処理やデータフィルタ処理などやむを得ず長くなる場合は都度判断
メソッド/関数に記述されている処理の抽象度は適切か
巨大ループはないか
- 巨大ループは即刻リジェクト
- 処理の観点毎に分割できないか要検討
- ループ内のロジックは5行程度を目処にする
- それ以上多くなる場合は、ループ内のロジックを関数に切り出すべき
- @see: 巨大ループ
ネストが深くなっていないか
- if文やループのネストは最大でも2段まで
- 3段以上はNG（三重ループ、三重IF文）
- ネストが深くなる場合は、ガード節や関数分割を行う
- それでもネストが深くなる場合は別クラス/モジュールへ切り出す
複雑な条件式はないか
- 複雑な条件式は可読性を落とすので対策を検討する
- 検討候補:
- (1) 説明変数に代入する
- (2) 関数化する
- (3) ド・モルガンの法則を用いて簡略化する etc...
- 説明変数の例は↓の通り
# NG
if x == 0 and y > 10 and z != 999:
:
:
# OK
is_x = (x == 0)
is_y = (y > 10)
is_z = (z != 999)
if is_x and is_y and is_z:
:
:
# OK: all（every）でまとめてもよい
if all([is_x, is_y, is_z]):
:
:
- やっている事は全く同じだが、条件が一行ごとに分解されたので条件式の理解が容易になっている
- 変数名をもっと具体的な判定内容に寄せた名前にすると尚良い（ここではサンプルなので変数名は雑になっている）
循環的複雑度は適切に保たれているか
- 循環的複雑度は最大20以内、10以内
- 循環的複雑度が20以上は原則NG
- 目視で計測するのは面倒過ぎるので循環的複雑度を計測するツールを利用すること
変数を上書きしていないか
- 変数の上書きは原則NG
- 上書きしたくなったら別の変数を定義する
name = "Mr A"
...
name = "Mr B" # <= これはNG
name2 = "Mr B" # <= 別の変数に代入するのはOK
- 変数の再代入は状態が変化するのでコードの理解が難しくなる
- ただし、ループの中で変数を上書きするのはOK
2. 命名
適切な命名か
- 機能や処理を適切に表現した命名ができているか
- クラス名はクラスが扱う概念を適切に表現しているか
- 関数/メソッド名は操作を適切に表現しているか
一般的なイディオムに従っているか
- 言語やフレームワークの一般的なイディオムがあればそれに従うこと
3. モジュール設計
モジュールの役割は明確になっているか
- 単一責務原則に従っているか
- 複数の責務を持っていないか
- モジュールの役割を一言で説明できるか
十分にモジュール化されているか
- 特定のコンポーネントの責務が多すぎになっていないか
- ひとつのメソッドの処理は多すぎないか
モジュールの配置は適切か
モジュールの粒度は適切か/大きすぎないか
- 巨大クラスはないか（クラスの行数が100行以上）
- 巨大なクラスは分割できないか検討する
- モジュール配下の関数が多すぎないか（パブリックな関数は7個を目処と考える）
モジュールは粗結合になっているか
4. 関数/メソッド設計
メソッド/関数の引数は多すぎないか
- 原則として3個程度を上限とする
- 関数の機能的にどうしても引数が増える場合はあるので都度判断
プリミティブ型を乱用していないか
- 関数の引数にプリミティブ型が指定されており、その値に意味（例えば、
1は管理者ユーザーであるなど）があるならクラスやEnumによる置き換えを検討するべき
- 特にドメイン層（またはそれに類するレイヤー）ではプリミティブ型ではなく、プリミティブ型をラップしたユーザー定義型が望ましい
エラーを握り潰していないか
クエリ関数が副作用を発生させないか
不要なimportはないか
コメントは適切に記述されているか
- 引数と返り値の情報は記述されているか
- 特殊な挙動をする場合はその旨記述されているか
- 上長なコメントはないか
順番に依存したコードはないか
- DB取得時にソートして一番上のレコード（
record[0]的な）を参照するようなコードはNG
- 仕様変更でレコードの格納順に規則性が無くなることは有り得るので適切な条件でデータを取得すること
5. セキュリティ
セキュリティに問題を生じさせる実装はないか
- SQLインジェクション、XSS、CSRF etc...
プログラムのパーミッションは適切は
プログラムが利用するアカウントの権限は適切か
- RDBMSなどを使用する際、利用するアカウントは無駄に強い権限を持たせていないか
6. パフォーマンス
パフォーマンスに悪影響を与える実装は無いか
- N+1を発生させる実装はないか
- ループの中でDB/ファイル/ネットワークアクセスを行っているコードはないか
7. コメント
嘘コメントはないか
- コメントが実態と乖離している場合は嘘コメントなのでNG
無駄コメントはないか
- コメントに書く必要のない内容は無駄なのでNG
- コードと1:1になっているようなコメントは不要なので除去
8. ロギング
必要なログは出力しているか
不要なログを出力していないか
- 開発用のデバッグログは削除する
- 開発時に必ず必要なのであれば適切なログレベル（DEBUGなど）で出力するようにしておく
9. ユニットテスト
ユニットテストは実装されているか
ユニットテストは素早く完了するか
- あまりにも遅くなるようならテスト内容を再検討する必要がある
- 個別のテストはミリ秒単位で実行されること
- テストが遅い理由は大概DB/ファイル/ネットワークにアクセスすることなので、ユニットテストではそういう部分はモック化されている必要がある
テストは機能をカバーしているか
- 当然だがテストが機能をカバーしていないと意味がない
10. コード品質強化
- 不要になったソースコードはないか
- 重複しているコードはないか
- 早まった最適化をしていないか
- 将来必要になるかもしれないという理由で不要なものを実装していないか（YAGNI原則）
- デバッグ用のコードは残っていないか
- コメントアウトされたコードが残っていないか
- DRYになっているか
- よりよい書き方はないか
- コードはLintのチェックを通過しているか（あるいはコーディング規約に従っているか）
- TODOコメントが残っていないか
参考資料