コードレビューの指摘の書き方と受け方:例文と返信テンプレート
指摘は事実・理由・提案で書き、返信は直した場所まで書く。
コードレビューの目的と、指摘が伝わらない理由
コードレビューは、変更をマージする前に別の人がコードを読み、不具合や読みにくさ、設計上の問題を見つける作業です。目的はコードベース全体の健全さを保つことで、完璧なコードを求めることでも、書いた人を評価することでもありません。Google が公開しているレビューの指針でも、変更がコードベース全体を良くするものであれば、完璧でなくても承認してよいという考え方が示されています。
それでも指摘がうまく伝わらないことはよくあります。原因の多くは、指摘の中身ではなく書き方にあります。「ここ、もう少し何とかなりませんか」のように問題も直し方も書かれていない指摘は、受けた側が意図を推測するしかありません。「なんでこう書いたんですか」は、質問のつもりでも責められていると受け取られることがあります。文字だけのやりとりでは、口頭よりも冷たく伝わりやすい点も意識が必要です。
指摘を書く前に、見る順番を決めておく
細かい書き方の指摘から読み始めると、時間を使い切って重大な問題を見落とすことがあります。レビューでは、影響の大きい観点から順に見ると効率がよくなります。一般的には次の順番がよく使われます。
- 変更の目的:PR の説明と差分が一致しているか、仕様どおりの動きになっているか
- 壊れ方:本番データを壊す操作、例外の握りつぶし、認証や入力値チェックの漏れがないか
- 設計と性能:責務の分け方、N+1 クエリ、不要な全件取得がないか
- テスト:境界値や失敗時の分岐がテストされているか
- 読みやすさ:命名、関数の長さ、コメント、書式
指摘の書き方:事実・理由・提案
伝わる指摘は、次の 3 つの要素でできています。事実は、コードのどこで何が起きているか。理由は、それがなぜ問題か(不具合になる、仕様と違う、読みにくいなど)。提案は、どう直すとよいか、またはどの方向で考えてほしいかです。3 つすべてを長く書く必要はありませんが、事実と理由が抜けると相手は判断できず、提案が抜けると相手が直し方を探す時間がかかります。
ここ、もう少し何とかなりませんか。なんでこう書いたんですか?これはダメです。[Must] 12 行目は user.age > 18 になっていますが、仕様は「20 歳以上なら許可」です。このままだと 19 歳でも購入できてしまいます。user.age >= 20 にしてはどうでしょう。[Must] users のループの中で毎回 orders を取得しているので、ユーザー数だけクエリが発行されます(N+1)。user_id の IN でまとめて取得するか、JOIN で 1 回にできると思います。nit: 変数名 tmp だと中身が分かりにくいので、activeUsers など内容が分かる名前だと読みやすいです。Must と nit を書き分ける
指摘には重さの違いがあります。仕様と違う、不具合になる、セキュリティ上の穴がある、データを壊す可能性がある、といった指摘はマージ前に直す必要があります。一方、変数名の好みや改行の位置など、直さなくても動作や保守に大きな影響がないものもあります。後者には「nit:」(nitpick の略、細かい指摘の意味)のような印を付けると、受けた側は優先順位を判断できます。チームによっては [Must]、[Should]、[Imo]、[Question] などのラベルを使います。どのラベルを使うかより、チーム内で意味がそろっていることが大切です。
| 種類 | 例 | 扱い |
|---|---|---|
| 必須(Must) | 仕様と違う条件、SQL インジェクション、例外の握りつぶし、本番データを壊す操作 | マージ前に直す |
| 推奨(Should) | N+1 クエリ、重複したロジック、テストが足りない分岐 | 原則直す。理由があれば相談 |
| 軽微(nit) | 変数名の好み、コメントの言い回し、空行の位置 | 直すかどうかは書いた人が決めてよい |
| 質問(Question) | この分岐が必要な理由、仕様の確認 | 回答をもらってから判断する |
人ではなくコードを指す
「あなたはいつもエラー処理を忘れる」ではなく「この catch では例外を握りつぶしているので、失敗したときに気づけません」と書きます。主語を人からコードに変えるだけで、指摘は評価ではなく情報になります。「〜すべき」と断定するより「〜にしてはどうでしょう」「〜だと思います」と提案の形にすると、相手が別の案を出しやすくなります。ただし、必須の指摘まで遠回しにすると重要さが伝わらないので、ラベルで重さを明示したうえで言い回しを柔らかくします。良い点を見つけたら、それも書いてかまいません。
論点を PR の範囲に絞る
「ボタン文言の誤字修正」という PR に対して、画面全体の設計の見直しを求めるのは論点がずれています。気づいた別の問題は、この PR の指摘とは分けて、別の Issue やチケットとして提案します。好みの書き方や、すでにあるコードの問題をこの PR で直させると、変更が膨らみレビューも長引きます。
指摘の受け方:返し方の型
指摘を受けた側の返信も、レビューの一部です。黙って修正を push するだけだと、レビュアーはどの指摘にどう対応したのかを差分から探さなければなりません。返信は短くてよいので、対応の結果が分かるように書きます。
直したら、内容と場所を返す
「修正しました」だけでなく、何をどう直したかと、そのコミットや行を添えます。複数の指摘をまとめて直したときは、それぞれのスレッドに返信するか、まとめのコメントで指摘ごとの対応を一覧にします。スレッドの「解決(Resolve)」を誰が押すかはチームの決まりに従いますが、レビュアーが確認する前に自分で閉じると見落としの原因になることがあります。
修正しました。WHERE の後にスペースを入れています(a1b2c3d、L24)。ご指摘ありがとうございます。条件を user.age >= 20 に直し、19 歳と 20 歳の境界のテストを追加しました(e4f5g6h)。テストが落ちていた件、原因は日付の固定漏れでした。テスト内で現在時刻を固定するように直し、CI が通ることを確認しました。分からなければ質問する
指摘の意図が分からないまま推測で直すと、的外れな修正になり往復が増えます。「どの点が気になったか教えてください。読みにくさでしょうか、それとも性能でしょうか」のように、自分の理解を添えて質問すると、相手も答えやすくなります。
同意できなければ根拠を添えて返す
指摘がいつも正しいとは限りません。たとえば「このループは map に書き換えてください」という指摘でも、途中で処理を抜ける必要があるなら for のほうが適しています。「SQLite では == でないと比較にならない」のように事実と違う指摘もありえます。その場合は、仕様やドキュメント、実際に試した結果などの根拠を添えて「このままにしたい」と返します。根拠のある反論はレビューの質を上げるもので、失礼にはあたりません。意見が平行線になったら、チャットや口頭で話すほうが早く決まることも多くあります。
「この関数は 200 行あって読みにくい」という指摘は、コードについての情報です。分割の方針を相談し、この PR で直すか、別の PR に分けるかを決めます。範囲外の大きな依頼(モジュール全体の命名の見直しなど)は、別チケットにすることを提案してかまいません。
大きすぎる PR は分ける
変更が数千行におよぶ PR は、レビュアーが全体を把握できず、重要な問題を見落としやすくなります。レビューにかかる時間も延び、指摘と修正の往復も重くなります。1 つの PR には 1 つの目的を持たせ、リファクタリングと機能追加、整形だけの変更と中身の変更は別の PR に分けると、それぞれの差分が読みやすくなります。分けられない事情があるときは、PR の説明に読む順番や重点的に見てほしい箇所を書いておくと、レビュアーの負担が減ります。
指摘を送る前・返信を送る前のチェックリスト
- 指摘に、どこで何が起きているか(事実)が書かれているか
- なぜ問題か(理由)と、どう直すか(提案)が分かるか
- 必須の指摘と nit が区別できるラベルや書き方になっているか
- 主語が人ではなくコードになっているか
- PR の目的から外れた指摘を混ぜていないか(外れるなら別チケットに)
- 返信に、直した内容と場所(コミットや行)が書かれているか
- 意図が分からない指摘を推測で直していないか
- 反論するときに、仕様・ドキュメント・試した結果などの根拠を添えているか
問題で確かめる
この記事の内容を、制限時間つきの問題で確かめられます。
- 次の指摘のうち、マージ前に必ず直すべきものは?指摘・レビュー / 4 択
- 好みや軽微な点の指摘には「nit:」などの印を付け、必須の修正と区別できるようにするとよい。指摘・レビュー / ○×
- 指摘された箇所に実際は問題が無いと確かめられたら、根拠を添えて「このままにしたい」と返してよい。指摘・レビュー / ○×
- PR「ログイン画面のボタン文言の誤字修正」への指摘で、論点がずれているのは?指摘・レビュー / 4 択
- 指摘「WHERE の後にスペースが抜けています」への返信として適切なのは?指摘・レビュー / 4 択
- 指摘「ここ、もう少し何とかなりませんか」の意図が分からない。返信として適切なのは?指摘・レビュー / 4 択
- 指摘「このループは map に書き換えてください」。途中で抜ける必要がある。返信として適切なのは?指摘・レビュー / 4 択
- 指摘を受けて直したら、返信せずに push だけするのが、相手の時間を取らない最もよい対応である。指摘・レビュー / ○×
- 5 件の指摘をまとめて直した。返信の仕方として適切なのは?指摘・レビュー / 4 択
- 誤った指摘「SQLite では = でなく == を使わないと比較になりません」への返信は?指摘・レビュー / 4 択
- コメントの書き方として適切なのは?実務シチュエーション / 4 択
- 自分の好みと違う書き方は、修正必須として差し戻すべきである。実務シチュエーション / ○×
- 完璧なコードでなくても、残っているのが Nit の提案だけなら、提案を添えて Approve してよい。実務シチュエーション / ○×