コードレビューチェックリスト

レビュー前

• 大きな変更をして後で破棄や大部分の書き直しを迫られるよりは、できるだけ早くやり直しに着手してもらう

レビュアーは誰がするか

• コードベースの領域を熟知しているか
• もっとふさわしいレビュアーがいる場合はふさわしいレビュアーに渡す
• 変更の主要部分に設計上の重大な問題が見つかれば、すぐにコメントを残す

プルリクエストの単位

• 変更の適切なサイズは単一の自己完結的な変更(CIとリリースフローが確立されているプロジェクトの場合)
• ファイル全体を再フォーマットしたいと思ったら、再フォーマットだけを1つの変更として提出しているかどうか
• 変更をベストなタイミングにする(この機能を追加するタイミングは今がふさわしいか、別の機会がいいのか)
• プロダクトコード基準で小さな変更にする
• 機能変更やバグ修正と、リファクタリングは別の変更にするのがベスト

仕様の確認

• 意図通りに動作するかどうか検証する
• 大きな視点できちんと確認する
• コードの各部分は相互にきちんと連携するかどうか
• ビジネスサイドまたはユーザーからの指摘がありそうなものはないか

コード

• コードベースと一貫性があるかどうか
• 正しくない使い方をしていないか
• その言語のベストプラクティスに従っているかどうか
• 理解しやすいか
• 必要以上に複雑になっていないかどうか
• オーバーエンジニアリングしていないか
• 適切な名前かどうか(何であるか／何をするかを伝えるのに十分に長く、しかし読むのに困難を覚えないほど短いもの)
• スタイルはツールでチェックする
• 開発者はコードを間近で見ているため、ある側面では開発者のほうが優れた洞察を持っていることも
• コードの隅々まで磨きをかけることを要求すべきではない
• 指摘がシステムのコードの健康状態を悪化させたり、指摘によりコードがより複雑になったりしないか

ソースコード内のコメント

• 実際に必要なコメントかどうか
• コメントは決定の背後にある理由といった、コード自体が語ることのできない情報を含んでいるかどうか
• コードがそれ自身を説明するほど明確でなく、コメントが多い場合はコードをもっとシンプルにすべき

バグ

• バグがコードベースに入る前に検知する(早く見つけるほど調査や修正に必要な時間が短くなる)
• 非効率な処理でアクセス数が増えた場合に問題になるコードはないか
• コードを読むだけではわからない不具合がないかを確認する
• 無限ループやデッドロックや競合状態を引き起こす可能性があるかどうか

負債

• 技術的負債を増やさない
• 今後改修しやすいか
• 重複を防ぐ(すでにあるものがいくつも作られることを防ぐ)
• 将来のリファクタリングの際にツールによる修正が可能かどうか
• 将来の開発者が変更をディスクリプションに基づいて検索しやすいようにしておく

テストコードレビュー

• ユースケースやデータ的にあり得るケースがあるかどうか
• 挙動がおかしい場合にちゃんと失敗するか
• 挙動を変えずにリファクタリングしても、テストは失敗しないか
• テストが複雑になっていないか
• privateメソッドはpublicメソッドを通してテストされているかどうか
• 特定のヘルパークラスは使われているクラスを通してテストする

やり取り

• 設計上の以前の決定を蒸し返したり論争したりしない
• レビューは過去の決定を見直す機会ではない
• もし重要でない指摘であれば、細かい指摘であることを明記する
• レビュアーは開発者の選好を受け入れるべきである
• 丁重に理由を説明する
• 問題の指摘に加えて明確な方向性を示すことと、開発者本人に決定を委ねることをバランス良く行う
• まとめてコメントする(フィードバック対応後に無関係なフィードバックをし続けない)
• 開発者の応答に理解を示し、それから変更が必要な理由を付け加えるのが良い説明

ドキュメント

• 関連するドキュメンテーションも更新しているかを確認