教えるコードレビュー、傷つけるコードレビュー — 同じ指摘の結果を分けるもの

はじめに — 同じ指摘、違う結果

レビューコメントが二つあります。一つは「これ、なんでこう書いたんですか」。もう一つは「この関数がバリデーションと保存を一緒にやっているので、あとでバリデーション規則だけ変えるときに保存ロジックまで触ることになりそうです。二つに分けるのはどうでしょう」。指摘している対象はまったく同じです。けれども前者を受けた人は防御の準備を始め、後者を受けた人はコードを見ます。

この違いは人柄や礼儀の問題として説明されがちですが、実際はほとんど形式の問題です。親切な人がよいレビューを書くのではなく、よい形式が普通の人からよいレビューを引き出します。だからこの記事は心構えより形式の話です。承認の基準、コメントの接頭辞、文の順序、PRのサイズ。どれもチームのウィキ1ページで合意できるものばかりです。

レビューの三つの目的、そして多くが取りこぼす二つ

コードレビューは三つのことを同時にやっています。欠陥を見つけ、知識を広げ、コードの所有権を分け合う。ところが多くのチームは最初の一つだけをレビューの定義とみなし、残りの二つを副次効果として扱います。

興味深いのは、データが逆方向を指している点です。アルベルト・バッケリ(Alberto Bacchelli)とクリスチャン・バード(Christian Bird)は、2013年にマイクロソフトで行った研究で、開発者も管理者もレビューの目的として欠陥の発見を真っ先に挙げるが、実際に観測される産出物はそれとは違うと報告しました。実際のコメントの多くは欠陥の指摘ではなく、可読性の改善、代替案の提示、そしてコードがなぜそういう形になっているのかについての理解の共有でした。ケイトリン・サドウスキー(Caitlin Sadowski)の研究チームが2018年に発表したグーグルの事例研究も、似た結論に達しています。グーグル社内でレビューの一次的な動機として認識されていたのは、バグ狩りよりも教育と規範の維持に近いものでした。

二つ目の目的である知識の伝播はこう働きます。レビュアーはコードを読みながら、その領域の地図を手に入れます。半年後にそのコードが午前3時に火を噴いたとき、レビューに参加していた人が一人でもいれば復旧時間が変わります。この観点で見ると、レビュアーを決めるときに「一番よく知っている人」だけを入れるのは最適化ではありません。よく知っている人一人と知らない人一人の組み合わせのほうが、チーム全体としては得になる場合が多いのです。

三つ目、所有権の共有は事故が起きたときに現れます。レビューを経てマージされたコードは、作者個人のものではなくチームの決定になります。これは責任回避の装置ではなく、失敗を個人のミスではなくシステムの問題として扱うための前提です。個人を責めるのをやめる文化がなぜ回復力の条件になるのかは、起きたことを資産に変える方法の回で扱いました。

承認の基準 — 完璧ではなく明確な改善

レビューが地獄になる最も多い経路は、レビュアーが自分に間違った質問をすることです。「これは自分が書いたはずのコードか」と問えば、承認は永遠に来ません。

グーグルが公開しているエンジニアリング慣行のドキュメントは、この点を明示的に整理しています。要点はこうです。レビュアーはコードが完璧になることを要求してはならず、その変更がシステム全体のコード健全性を明確に改善するなら承認すべきである。完璧なコードというものはなく、より良いコードがあるだけだという一文も添えられています。承認の質問は最善かどうかではなく、マージ前より良くなったかどうかです。

この基準がなぜ重要かというと、逆の基準が生むコストが見えにくいからです。完璧を要求するレビュアーの下で、人は二通りの反応をします。PRを細かく分けるのではなく、レビューそのものを避けられるやり方で働き始めるのです。リファクタリングを先送りし、そのレビュアーをレビュアー一覧から外し、急ぎだという理由で承認を飛ばす名分を探します。レビュー品質を極端まで押すと、レビュー自体が迂回されます。

ただしこの基準には例外があります。セキュリティ、データ整合性、戻しにくいマイグレーション、公開APIの契約。こうした領域では「良くなったから通す」は通用しません。ブロックするものと通すものの一覧をチームであらかじめ合意しておけば、その判断がレビュアー個人の性格の問題として誤解されずに済みます。

コメントの文法 — 強度の表示と三段構造

レビューが人を傷つける最も多い理由は、内容ではなく強度が伝わらないことです。レビュアーが「直ったら嬉しいけれど、直さなくてもいい」程度で書いたコメントを、作者は「これを直さないと承認しない」と読みます。コメントが十個つけば、十個すべてが最高強度で読まれます。

解決策は驚くほど単純です。接頭辞をつけること。グーグルのレビュードキュメントにも、細かい指摘には「nit:」をつける慣行が明記されています。チームの事情に合わせて日本語で書いてもかまいませんし、三段階あれば十分です。

接頭辞が生む本当の変化は、レビュアーの側にもあります。強度をつけるにはレビュアーが自分にこれは本当にマージを止める話かと問う必要があり、その問いだけでブロック性のコメントの数が目に見えて減ります。

強度を決めたら、次は文の構造です。観察、影響、提案の順で書くと、評価語が自然に抜け落ちます。「このコード、いまいちですね」は観察がなく影響がなく提案もない、純粋な評価です。一方「この関数が例外を握りつぶしているので(観察)、障害時に原因追跡が難しくなりそうです(影響)。ログを出してから再スローするのはどうでしょう(提案)」は、同じ内容を協業に変えます。この構造はコードレビュー固有のものではなく、一般的なフィードバックの文法でもあります。状況別の文のパターンは、会社でそのまま使える文章集の回にまとめてあります。

一つ付け加えると、うまくいっている部分を指摘するコメントは礼儀ではなく情報です。レビューに否定的な信号しか流れなければ、作者は何を維持すべきかを知りようがありません。「このエラー処理のやり方、他のモジュールにも持っていきたいです」といった一行が、チームの慣行をつくります。

PRをレビュアーのために設計する — サイズと説明

レビュー品質のかなりの部分は、レビュアーではなく作者が決めます。そして作者が握る最大の変数はサイズです。

広く引用される数字があります。一度に200から400行を超えると欠陥発見能力が落ちるというもの。出典は2006年にスマートベアがシスコで行った事例研究です。開発者約50人、コード320万行、レビュー2500件規模のデータを分析し、レビュー規模が大きくなるほど行あたりの欠陥発見数が落ち、毎時500行を超える速度や60分を超える連続レビューでは効果が急減するという結論を出しました。

この根拠の限界も一緒に知っておく必要があります。一つの会社、一つのコードベース、2006年のツール環境から出た単一の事例研究であり、GitHubのプルリクエスト以前の慣行を前提にしています。しかも中心指標である欠陥密度は行数で割った値なので、大きな変更で値が下がるのは部分的に定義上当然の結果でもあります。レビュアーが実際に報告した欠陥だけを数えている点も引っかかります。ですから200から400行は、物理法則ではなく経験則として受け取るほうが正確です。

経験則であっても方向はおおむね合っています。参考までに、グーグルの事例研究で報告された変更の中央値サイズは数十行水準でした。私たちが想像するよりはるかに小さいのです。実務的にはこう進めれば十分です。リファクタリングと機能変更を一つのPRに混ぜないこと、自動フォーマットと実際のロジック変更を分けてコミットすること、大規模な変更はインターフェースを先にマージして実装を後ろにつけること。

サイズを減らしたら、残り半分は説明です。よいPR説明は三つを含みます。なぜこれをやるのか(背景とリンク)、どんな選択肢を検討してなぜこれを選んだのか(代替案)、どこを集中して見てほしいのか(レビュー依頼ポイント)。特に三つ目が、非同期レビューの往復を決定的に減らします。3番目のファイルのロック処理だけ特に見てください、残りは機械的な変更ですという一行が、レビュアーの2時間と1日分の往復を節約します。

レビューを受ける側の技術

レビュー文化はレビュアーだけが頑張ってできるものではありません。受ける側にも技術があります。

第一に、最初の反応を遅らせること。コメントを読んですぐ反論文を書くのが最もよくある失敗です。特に時間をかけて書いたコードほど、指摘が人格攻撃のように読まれます。30分後に読み返すと、同じ文が違って見えることが多いものです。

第二に、同意できないときは根拠を求めること。「そうすると性能が悪くなりそうですが」より「この方式を選んだ理由はAだったのですが、ご提案の方式はその部分でどんな利点がありますか」のほうが、はるかに早く結論に届きます。議論が長引くなら形式を変えるのも手です。往復が三回を超えたら、15分の通話がスレッド20件より安くつきます。

第三に、反映しないと決めたことにも返事をすること。沈黙で流されたコメントは、レビュアーには無視と読まれます。「これは次のPRで対応します、チケットを作っておきました」や「これは意図した動作なのでコメントを追加しました」の一行で十分です。反映率より応答率が信頼をつくります。

第四に、学んだことを明示的に言うこと。「このパターン知らなかったので勉強になりました」の一行はお世辞ではなく、レビュアーへのフィードバックです。レビューはおおむね報われにくい労働であり、その一行が次のレビューの丁寧さを変えます。

レビューが権力ゲームになるとき

ここまではうまく機能している場合の話です。そうでない場合もあります。

サインはおおむね似ています。特定のレビュアーのPRだけがやけに長くかかる。指摘の内容がレビューごとに変わって合わせようがない。アーキテクチャの問題はそのまま通るのに、変数名と空白にはコメントが二十個つく。最後の項目は、C・ノースコート・パーキンソン(C. Northcote Parkinson)が1957年に指摘した自転車置き場の議論とまったく同じ構造です。原子炉の設計には誰も意見を出せませんが、自転車置き場の色にはみんなが意見を出します。細かい指摘が集まる場所は、たいてい誰も難しい部分を読んでいないというサインです。

レビューが完全に中立な技術行為ではないという実証研究もあります。ジョシュ・テレル(Josh Terrell)の研究チームが2017年に発表したGitHubプルリクエストの分析は、全体としては女性のPR受理率のほうが高かったが、プロフィールから性別が識別できる外部貢献者の場合はその優位が消えると報告しました。この研究は性別の推定方法と観測データの因果解釈をめぐって相当な反論を受けており、結論が確定した事案ではありません。ただし確定していないことが、レビューは純粋に技術的だという主張の根拠になるわけでもありません。

対応は個人技より制度のほうが効きます。スタイル論争は人ではなくフォーマッタとリンタに判定させること。強度の接頭辞をチームのルールとして明文化し、ブロック権限を明示すること。承認までの時間と往復回数を、人別ではなくチーム指標として見ること。そして特定の人にだけ繰り返し違う基準が適用されるなら、それはレビュースレッドで解く問題ではなく、上司との1on1で扱う問題です。

おわりに — レビューはコードではなく人を通ります

よいレビュー文化の指標は、欠陥の発見数ではありません。半年後にそのコードを直せる人が何人増えたかです。欠陥はテストとモニタリングでも捕まりますが、設計の文脈がチームに広がる通り道は、ほぼレビューだけです。

今日すぐ変えられるのは二つだけです。次のコメントに強度の接頭辞をつけること、そして次のPR説明にどこを見てほしいかの一行を入れること。文化は合意文書ではなく、こうした一行が積み上がってできあがります。