コードレビューで見ている9つの観点と、Must / Should / Mayの付け方
この記事で扱う問題
コードレビューが、インデントや命名の指摘だけで終わってしまうことがあります。スタイルは静的解析と自動整形に任せられます。人が時間を使うべきなのは、ツールでは見つからない、本番で壊れる問題です。
一方で、指摘がすべて同じ重さで並ぶと、作者はどれを直すべきか判断できず、レビューが長引きます。
判断
私はレビューで、おおよそ次の順に確認します。
| # | 観点 | 確認すること |
|---|---|---|
| 1 | 要件 | チケットの要件を本当に満たしているか。仕様の解釈がずれていないか |
| 2 | 責務の配置 | そのロジックは、その層・そのクラスに置くべきものか |
| 3 | 境界条件 | 0件、上限、空文字、日付の境目、権限の境目 |
| 4 | 例外系 | 失敗したときに、途中まで処理された状態が残らないか |
| 5 | データ整合性 | 複数のテーブル・外部リソースを更新するとき、片方だけ成功しないか |
| 6 | 冪等性 | リトライや二重送信で、同じ処理が2回走っても問題ないか |
| 7 | 性能 | N+1、ループ内のクエリ、インデックスのない検索、全件読み込み |
| 8 | 後方互換性 | 既存のクライアントやデータを壊さないか。デプロイ順序に依存しないか |
| 9 | 障害時の復旧 | 失敗したときに原因を追えるログがあるか。やり直せるか |
指摘には優先度を付けます。
- Must：直さないとマージできない。要件漏れ、データ破壊、セキュリティ、互換性の破壊
- Should：直すべきだが、理由があれば別チケットでもよい。責務の置き場所、テスト不足
- May：好みや提案。直すかどうかは作者が決めてよい
なぜそう判断したか
上の9つは、本番で起きたときの影響が大きく、後から見つけるほど直すコストが上がる順に近い並びです。スタイルの問題は後からでも一括で直せますが、データ整合性の問題はデータそのものが壊れてから気づくことになります。
優先度を付けるのは、レビューを「作者との交渉」にしないためです。Must と May が同じ重さで並ぶと、作者は全部に対応するか、全部に反論するかの二択になります。優先度があれば、Must だけ確実に直してマージする、という判断ができます。
どこから設計過剰になるか
- 小さな修正に、9つの観点すべてを当てはめる必要はありません。変更の種類によって、効く観点は数個に絞れます
- May を大量に付けると、それだけでレビューが重くなります。本当に伝えたいものだけにします
- 観点のチェックリストをそのまま機械的に適用すると、要件（1）の確認がおろそかになりがちです。一番大事なのは1です
まとめ
- スタイルはツールに任せ、人は本番で壊れる問題を探す
- 要件・責務・境界条件・例外系・整合性・冪等性・性能・後方互換性・復旧の順に見る
- 指摘は Must / Should / May で優先度を付け、レビューを交渉にしない