engineering-code-review

エンジニアリングのコードレビュー

コードレビューを活用し、価値のある変更を取り込みながら、長期的なコードの健全性を守り、向上させます。基準は完璧さではありません。変更によってシステム全体が改善され、許容できないリスクが持ち込まれないことが基準です。

Google Engineering Practices Documentation、特にコードレビューの概要、レビュアーガイド、レビュー基準、ナビゲーション、スピード、小さな変更、緊急時のガイダンスを参考にしています。出典：https://google.github.io/eng-practices/、https://google.github.io/eng-practices/review/reviewer/standard.html、https://google.github.io/eng-practices/review/reviewer/navigate.html、https://google.github.io/eng-practices/review/reviewer/speed.html、https://google.github.io/eng-practices/review/developer/small-cls.html、https://google.github.io/eng-practices/review/emergencies.html。ライセンス：CC-BY 3.0。

レビュー基準

• 完璧でなくても、変更によって対象システムのコードの健全性が確実に向上するなら、承認を優先します。
• 真の緊急事態であり、後続のクリーンアップが明確に示されている場合を除き、システムを悪化させる変更は承認しません。
• 好みよりも、技術的な事実、データ、既存のプロジェクト標準、長く通用する設計原則を優先します。
• スタイルガイドや自動フォーマッターを基準として扱います。個人的なスタイルの好みは軽微な指摘にとどめます。
• 複数の設計にそれぞれ妥当性がある場合、コードの健全性を損なわない限り、作者の選択を受け入れます。
• セキュリティ、プライバシー、並行処理、アクセシビリティ、国際化、インフラストラクチャ、データ移行、または自分では検証できないドメインロジックなど、専門性の高い領域に変更が及ぶ場合は、別の適任なレビュアーに依頼します。

レビューワークフロー

1. スコープを明確にします。

   • 差分、依頼されたレビューの範囲、対象ブランチ、レビューが全体か部分的かを確認します。
   • 部分的なレビューの場合は、レビューしたファイル、動作、懸念事項を正確に示します。

2. レビュー資料を集めます。

   • GitHub の PR の場合は、コメントする前に、PR の説明、リンクされた課題／設計ドキュメント、変更されたファイル、既存のレビューコメント、CI の状態、関連するコミット履歴を確認します。
   • ローカルでレビューする場合は、git status --short、対象ブランチまたは判明している場合はベース SHA、ローカルの差分を確認します。
   • 差分や PR のコンテキストが確認できない場合は、レビューを作り上げず、提示を依頼します。

3. まず全体像を把握します。

   • 説明、課題、テスト、周辺のコンテキストを読みます。
   • 現時点でこのコードベースにこの変更が必要かを判断します。
   • 進む方向が誤っている場合は、早い段階で伝え、より適切な方法を提案します。
   • 変更が大きすぎて徹底的なレビューが難しい場合は、表面的な全体レビューを試みる前に、変更の分割か、レビュー担当者が承認した大規模変更の計画を求めます。

4. 詳細に入る前に、主要な設計をレビューします。

   • 変更を定義する中核的なファイルや API を特定します。
   • 命名やフォーマットに関するコメントを挙げる前に、アーキテクチャ、責任範囲、データフロー、互換性、ロールバックのリスクをレビューします。
   • 残りのレビューが無意味になる重大な設計上の懸念があれば、すぐに伝えます。

5. 担当範囲内で、人が書いた行をすべてレビューします。

   • 生成ファイル、大きなデータファイル、機械的に生成された出力は、適切であればサンプリングして確認できます。
   • コードの理解が難しすぎる場合は、承認する前に簡素化や、より明確な構成を求めます。

6. 以下のレビュー用チェックリストに照らして変更を確認します。

7. 検証の証拠を確認します。

   • ユーザーからコマンドの実行を求められていない場合は、既存の CI と対象を絞ったテストを優先します。
   • 環境と依頼内容に照らして適切な場合にのみローカルテストを実行し、それ以外の場合は不足しているテストや未検証の項目を明示します。
   • コマンドの出力や CI の結果を確認していない限り、テストに合格したと主張しないでください。

8. 判定を下します。

   • APPROVE：コードの健全性が向上しており、残るコメントはブロッキングではありません。
   • APPROVE_WITH_COMMENTS：残るコメントは軽微または任意のものであるか、追加のレビューラウンドを行わなくても作成者が対応できると信頼できます。
   • REQUEST_CHANGES：正確性、設計、保守性、テスト、ドキュメント、リスクに関する問題が残っています。
   • NEEDS_SPECIALIST：責任を持って評価できない分野を、別のレビュアーが確認する必要があります。

レビューチェックリスト

• 設計：この変更はここに加えるべきですか？現在のシステムに適した抽象化になっていますか？
• 機能：作者の意図どおりに動作しますか？その動作はユーザーや将来の開発者にとって適切ですか？
• エッジケース：失敗、空の状態、再試行、冪等性、順序、時間、権限、並行処理を考慮していますか？
• 複雑さ：すぐに理解できますか？推測に基づく将来の問題ではなく、今日わかっている問題を解決していますか？
• テスト：リスクに応じた単体テスト、統合テスト、エンドツーエンドテストになっていますか？コードが壊れた場合、テストは失敗しますか？
• テストの品質：アサーションは有意義で、焦点が絞られ、決定的で、保守しやすいですか？
• 命名：名前から目的が伝わりますか？曖昧だったり、冗長だったりしませんか？
• コメント：コードを言い換えるのではなく、理由、制約、わかりにくいアルゴリズムを説明していますか？
• ドキュメント：動作や使用方法が変わる場合、README、API ドキュメント、移行ノート、運用手順書、生成ドキュメントを更新していますか？
• スタイルと一貫性：機能変更と無関係な書式変更を混在させず、プロジェクトの規約に従っていますか？
• コンテキスト：この変更は周囲のシステムを改善しますか？それとも、長期的な複雑さを少し増やすだけですか？
• 運用：この変更に対するデプロイ、移行、可観測性、ロールバック、機能フラグ、障害対応は適切ですか？
• セキュリティ：認可、認証、シークレット、インジェクション、依存関係、デシリアライズ、権限境界に関するリスクに対処していますか？
• プライバシー：機密データの不要な収集、ログ記録、公開、保持、境界を越えた移動を避けていますか？
• アクセシビリティ：ユーザーインターフェースの場合、キーボード操作、セマンティクス、フォーカス、コントラスト、ラベル、支援技術での動作を維持していますか？
• 国際化：該当する場合、ロケール、タイムゾーン、エンコーディング、テキストの拡張、複数形、並べ替え、翻訳済み文字列に対応していますか？

セキュリティ、プライバシー、アクセシビリティ、国際化、並行処理、インフラストラクチャ、データ移行、またはドメイン固有の正確性を自信を持って評価できない場合は、専門家にエスカレーションしてください。

コメントの重大度

• BLOCKER：承認前に必ず修正してください。
• IMPORTANT：書面による根拠がない限り、承認前に修正してください。
• QUESTION：最終判断の前に回答が必要です。
• NIT：仕上げのための指摘です。承認を妨げてはいけません。
• OPTIONAL：作者が採用しないことを選べる提案です。
• FYI：学習や将来の検討のための情報であり、変更を求めるものではありません。

出力形式

重大度の高い順に指摘事項から始めてください。可能な場合は、ファイル名と行番号を示してください。

## Findings
- [BLOCKER] path/file.ext:123 - Problem. Why it matters. Required change.
- [IMPORTANT] path/file.ext:45 - Problem. Why it matters. Suggested fix.

## Open Questions
- ...

## Verdict
REQUEST_CHANGES | APPROVE_WITH_COMMENTS | APPROVE | NEEDS_SPECIALIST

## Notes
- Scope reviewed:
- Tests/commands checked:
- Follow-ups:

問題が見つからなかった場合は、その旨を明確に伝え、未検証の範囲や残存リスクについても記載してください。

よくある間違い

• コードの健全性ではなく、好みを理由に承認を妨げる。
• 周囲の関数やモジュールによって変更が危険になる可能性があるのに、変更行だけをレビューする。
• 作者がレビューのスレッドで説明したことを理由に、複雑なコードを受け入れる。
• 変更自体が複雑さを持ち込んでいるのに、「後で整理する」を許容する。
• 設計上の問題を早めに指摘せず、変更の終盤になって大規模な書き直しを求める。
• すばやく包括的な回答をすれば有益な作業を進められるのに、作者を待たせる。