はじめに
Google の Code Review Developer Guide は、Google におけるコードレビューのプロセスをまとめたサイトです。
- How To Do A Code Review: A detailed guide for code reviewers.
- The CL Author’s Guide: A detailed guide for developers whose CLs are going through review.
日本語訳
ここには、レビュアー（レビュー担当者）向けのガイドと、レビュイー（レビューを受ける側の開発者）向けのガイドが載っています。
一方、株式会社 ACCESS から 2022 年 1 月に技術同人誌として出版された「アイのムチ よくないレビューの例とレビューで折れないメンタルづくり」という本にも、同様に 2 章立てのガイドが載っています。
そして私はアイのムチの筆者ですが、実は執筆当時 Google のガイドは読んでいませんでした。
17ページでアメさんに「例えばGoogle 社の公開する Code Review Developer Guide をみんなで読み合わせたりとか」と喋らせてる割には、筆者は読んでない（笑）
でも、執筆後に開発チーム内の輪読会で Google ガイドを読む機会がありました。そして、アイのムチには Google ガイドと同じことをどれくらい書けたのだろうか、という疑問が出てきて、今回確かめてみよう！と思い立ちました。
今回はレビュアー編なので、Google の How To Do A Code Review の私の解釈を意訳でまとめながら、アイのムチの「よくないレビューの例」との違いを見ていきます。レビュイー編はこちらです。
The Standard of Code Review
概要
レビュアーは CL（Change List。Pull Requestと読み替えてもよい） が完全でなくても、改善が前進する内容なら Approve をしてよい。
「完璧なコード」は存在せず、「より良いコード」しかない。
すべての小さな部分を直すべきではなく、重要な修正と継続的な改善のバランスを取る。
ただし、明らかに悪化するものは拒否してよい。
Mentoring
レビューはレビュイーに何かを教える機会でもある。知識の共有は改善プロセスの一部。
その場で重要でないコメントはNit：のような接頭辞を付ける。
Principles （原則）
スタイルガイドのような原則には必ず従い、そこにないものはある程度レビュイーの意向を受け入れるか、既存のコードに合わせるよう依頼する。
Resolving Conflicts （競合解決）
意見が分かれた場合、このガイドの内容に基づいて決めるか、コメントのやり取りではなくミーティングを開催して、議論した結果を CL に記録する。
解決しなければ、第三者に入ってもらう。
アイのムチの場合
「1.9 よいレビューとは」に、
- 1つめは、建設的なフィードバック、健全で活発な議論があることです。コメントを書く人は、自分の考えを明確にしつつ、自分が正しいとは主張しないようにします。状況に応じていろんな代替案や回避策が存在することを認め、修正の強要を控え、どう思う？などと問いかけます。もし実装者から反対意見が出た場合は、本やインターネット、先人の知恵を見て、2人の間でよい着地点を探しましょう。それが難しそうなら、第三者に客観的な意見を求めるのがよいでしょう。
- 2つめは、レビュアーが実装者に共感していることです。いかなる方法でも、実装者が実装にかけた時間や労力を称賛し、強い口調ではなく親切で控えめな口調で接することが大切です。たくさんコメントした場合は、PR以外の場所、たとえば対面やオンライン会議、チャットやメールなどで積極的に連絡を取り、誤解が生じることを防ぎます。そうすればきっとつらい思いをする人は減り、レビューの雰囲気や会話の方向性がとてもポジティブなものとなるでしょう。
- 3つめは、すべての人が対等であることです。職種や職歴、スキルレベル、入社時期、サラリーなどに影響されず、全員が同じ基準、同じ目線で、かつお互い足りないものを埋める場所としてレビューを活用します。本書で取り上げたような、スキルテストや評価の場として利用することは不適切です。
と、おおむね書いてる方向性は同じですが、スタイルガイドのような原則が必要であることの言及は抜けていました。
改版計画が立てばそのことを入れたいです。
What to look for in a code review
概要
コードレビューは次のことを確認する。
- コードがよく設計されていること
- 機能がユーザーと将来の開発者の両方にとって優れていること
- UIの変更が賢く見栄えがよいこと
- 並列プログラミングが安全に行われること
- コードが必要以上に複雑でないこと
- 将来必要になるが今必要かどうかわからないものまで実装していないか
- 適切な単体テストがあるか
- テストがよく設計されているか
- 何であれ明確な名前が使われているか
- コメントが明確で有用、コードの説明ではなく必要な理由を説明していること
- ドキュメント化されていること
- スタイルガイドに準拠していること
レビューを依頼されたコードのすべての行を確認し、環境を確認し、コードの状態が改善されていることを確認し、レビュイーが行っている優れた点を称賛する。
Design （設計）
レビューで見る最重要点は、変更はすべて意味があるか、ベースコードとライブラリのどちらに属するか、うまく結合されているか、今すべき変更か、といった CL の全体設計。
Functionality （機能性）
開発者が意図したとおり動くこと、ユーザーや将来の開発者のために良いこと、バグがないこと、並列プログラミングがあるならデッドロックや競合みたいなリスクがないかを見る。
Complexity （複雑さ）
レビュアーが CL をすぐに理解できなかったり機能の検証が難しい場合、おそらく複雑なコードになっている。
過剰な設計が入っていないか、今解決すべき問題にだけ取り組んでいるかを見る。
Tests
テストが CL に含まれており、正しく有用であることを見る。
テストはレビュー対象外としない。
Naming
読みづらい長さでなく、何であるかを示す、または、何をするかを完全に伝えるような良い命名をしているかを見る。
Comments
わかりやすく明確なコメントが書かれてるかを見る。
コメントはそのコードが必要な理由を説明するためのもので、コード自体の説明に書くならコード自体をわかりやすくすること。
Style
スタイルガイドに従っているかを見る。
スタイルガイドにないが改善したいことはNit:をつけて対応必須ではないコメントとする。
また、他の変更を同じ CL に入れない。
例えば機能変更中に再フォーマットしたい場合、機能変更と再フォーマットを別々の CL とする。
Consistency （一貫性）
スタイルガイドになく新しいルールでもないものは、既存のコードとの一貫性を維持しているかを見る。
Documentation
ビルドやテスト、リリースに関するものを追加/更新/削除したら、READMEなどドキュメントも追加/更新/削除しているかを見る。
Every Line （すべての行）
一部のコードを注意深く精査しつつ、全てのコードを理解して見る。
理解が難しい場合、他のレビュアーも理解が難しい可能性があるので、レビュイーにコードの明確化をお願いする。
理解はできるが自分では承認判断できない場合、判断できるレビュアーを見つける。
Exceptions （例外）
複数のレビュアーがおり、自分が一部だけ見る場合、レビューした部分や他の人への期待をコメントで伝える。
Context （環境）
レビューツールの非表示部分（ファイル全体）も含めて確認し、変更が意味をなしているか、小さなメソッドに分割する必要はないか、これを入れることでシステムがより複雑にならないかを見る。
Good Things
CL でよかったこと、素晴らしいことはレビュイーに伝える。
メンタリングの観点で、間違いだけに焦点を当てず、励ましや感謝も伝える。
アイのムチの場合
Google ガイドほど詳細なレビュー基準は書きませんでしたが、「1.5 一貫性がない」の
- 複数のレビュアーがおり、人それぞれレビューポイントや評価基準が大きく違う
- 同じレビュアーでも一貫性がない。たとえば、グローバル変数をある日は禁止し、別の日は許可する
- 前と同じ問題が見つかっても、前と同じ指摘が行われない。言うのを躊躇う.同じ指摘は2度と行われない
は Style や Consistency の章に通ずる内容だったり、「1.1 攻撃的」でアメさんがコメントした
- ご対応ありがとうございます。実装箇所は合ってますが、いくつかお願いしたいことを Must/Want で伝えますね。
Must: if-elseを仕様通りに直す
Want: 類似実装の共通化、誤りを未然に防ぐための単体テスト作成
は、間違いだけでなく合っている箇所を認めて励ます Good Things と同じ意図が含まれています。
また、「1.4 持久戦」の
- やたらと長く、予定を超過する
- 最初はやる気があっても、時間が経つと疲れる
- 実装者もレビュアーも、時間が経つにつれ不注意になるが、切り上げられない
- 体力を吸われ、レビューの精度も低くなる
- 充実感を伴って終わり、本質的な問題に気づけない
は解釈を変えると Google ガイドの本章全体のように合理的に本質的な問題に注力すれば、レビューが長引いたり疲れたりしにくくなるということです。
正直、もし本を書く前にこの章を読んでいたら、取り入れたいエッセンスがいくつもありました。筆者としては同意見というか、新しい気づきもいくつもありました。
Navigating a CL in review
概要
複数のファイルにまたがるような大きなレビューで最も効率的な方法は、よい説明が書かれていること。
最重要部分に注目し、全体的にうまく設計されているか見てから、 CL の残りの部分を順序立てて見る。
Step One: Take a broad view of the change （広く見る）
Rejectしたい CL なら、その理由をすぐにコメントする。拒否だけでなく、代わりに何をすべきか提案する。
例えば「グッジョブです、ありがとう！でも、実はFooWidgetは廃止予定なので、かわりに新しいBarWidgetを変更しましょう」
丁寧な言葉づかいで礼儀を示すのは大事。意見が合わない場合でも、開発者同士お互いを尊重する。
不要な変更が複数作られるような状況なら、何かがまずいので、コミュニケーションやレビュープロセス改善で未然に防ぐことも重要。
Step Two: Examine the main parts of the CL （主要部分を見る）
論理的な変更の数がいちばん多いファイルがおそらく主要な部分なので、最初にそこを見るのが効率的。
どこから見ればいいかわからなければ、レビュイーに聞くか、複数の CL に分割する提案をする。
主要部分を見て設計の問題に気づいたら、レビュー途中でもコメントを送信したほうがよい。主幹設計が変更されると、残りの部分のレビューは無駄になるかもしれないし、レビュイーがレビューの合間に進めている次の CL も手戻りになるかもしれない。
主幹設計の変更には時間がかかり、大抵期限があるので、できるだけ早く着手したほうがよい。
Step Three: Look through the rest of the CL in an appropriate sequence （残りを順序立てて見る）
設計の問題がなければ、呼び出し関係など論理的な順序を理解して残りを見る。
最初にテストを読むと変更で何が起きるのかがわかりやすい。
アイのムチの場合
「1.3 突き放し」にて、よいレビューの例としてアメさんが
doTask()の設計から実装までありがとうございます。今回もコメントを Must/Want で伝えますね。
Must: この判定をビジネスロジックのクラスに移す。
Must: doTask()に2つ以上の責務があるので、ダウンロードと開く処理とに分ける。
Want: doTask()を4つに分ける
とコメントしていますが、これがまさに Google ガイドの Step One で述べられた内容と同じで、礼儀を伴いながら設計レベルでの変更を提案しています。
レビュー途中でコメントを送信すること、設計に問題がなければ順序立てて残りを見ることまでは言及していませんが、筆者としては、これらも同意見です。
Speed of Code Reviews
Why Should Code Reviews Be Fast?
コードレビューが遅いと、スケジュールは遅れ、苛立ちがレビュアーに向かったり、よいコーディングを妥協する傾向がある。
レビューが早いと苛立ちは少なくなり、よいコーディングや改善の余裕も生まれるので、プロセスの高速化でレビューを早くしよう。
How Fast Should Code Reviews Be?
通常であれば、レビューを依頼された直後に見るのがいちばんよい。遅くても1営業日。つまりレビュー依頼から1日以内に最低1回〜複数回のレビューが行われる。
Speed vs. Interruption
とはいえコーディングなどの集中作業を中断してレビュー対応すると、スムーズな作業に戻るには時間がかかるという調査結果がある。レビューを待ってもらったほうがコストが少ない場合もある。
集中作業の中断地点でレビューするとバランスがよい。例えば、ひと段落したとき、昼食後、会議から戻ったとき、休憩から戻ったとき。
Fast Responses
全体を見ていなくても、途中まででコメントを送ったり、いつ見れるかを事前に知らせたりすると、レビュイーがレビューが遅いと感じる苛立ちが大幅に緩和される。
個々の応答は高速でも、レビュー自体には十分に時間をかけて「LGTM」が「このコードが当社の基準を満たしている」を意味するレビューを行う。
Cross-Time-Zone Reviews
タイムゾーンが異なり、返信が必要なコメントをする場合、実装者が仕事を終える前、さらに応答する余裕がある時間帯にコメントする。
実装者が仕事を終えている場合、翌日仕事を始める前にできる限りのレビューを終える。
LGTM With Comments
CL に未解決のことがあっても、レビュアーが LGTM したほうがよい場合がある。
レビュイーが残りのコメントすべてにちゃんと対応してくれると確信できる場合や、残りの変更が軽微な場合、先にLGTMしておくと、LGTMを待つためだけに1日遅れるようなことを防げる。
Large CLs
CL が非常に大きい場合、CL の分割を依頼する。レビュイーにとっては面倒でも、レビュアーにとってはとても助かる。
分割できない場合、全体的な設計を見て生じたコメントはすぐ送信する。
レビュイーのブロッキングを無くしたり、レビュイーが迅速に対応できることがレビュアーの目標の1つである。
Code Review Improvements Over Time
これらのガイドに従って厳格にレビューしていれば、やがてレビュープロセスがどんどん速くなる傾向がある。
レビュイーはよいコーディングを学び、最初から優れた CL を送る。これにより、レビュー時間がますます少なくなる。
一方のレビュアーは、迅速なレビューを学ぶ。ただし、早さのためにレビュー基準や品質を妥協しないこと。
Emergencies
とはいえ CL が迅速にレビューされ、品質が妥協されるような緊急事態もある。本番環境のユーザーに影響するバグ、いわゆるシステム障害である。
アイのムチの場合
レビューの速度を上げるという観点では、「1.7 論点がズレている」に書いた
- ソフトウェアの出荷には重要でない、コーディングの細かい部分ばかり言及される
- 自動レビューで賄える箇所を、手動で頑張ってチェックする
みたいなよくないレビューパターンへの言及くらいだと思います。
また、「1.8 一方通行」で、システム障害が発生して高速なレビューをおこなっているシーンが出てきますが、これはベテランのコードがノーレビューで適用されていたり、議論を避けたりといった、Google ガイドに書かれている「厳格にレビューしているか、妥協するか」の2択以前の話でした。
でも起こりうる話なので、いきなり高品質かつ高速なレビューを心がけるのではなく、まず今やれる範囲でレビューする、それを繰り返して徐々に改善していく、という受容と向上心みたいなものが大事だと思っています。Code Review Improvements Over Time と方向性は一緒ですね。
How to write code review comments
概要
- 親切に
- あなたの考えを説明する
- 明確な指示だけでなく、レビュイーに決定させることでバランスを取る
- 複雑さを説明するだけでなく、単純化やコメント追加をレビュイーに促す
Courtesy （礼儀）
コードに対してコメントし、開発者に対してコメントしない。無駄な動揺や論争は避ける。
- 悪い例：「並行性から得られるメリットが明らかでないのに、なぜここでスレッドを使用したのですか？」
- 良い例：「ここでの同時実行は、私が見たところ実パフォーマンスにメリットがなく、システムに複雑さを追加しています。パフォーマンスメリットがないので、このコードはシングルスレッドにするのが最適だと思います」
Explain Why
コメントした意図、考えたベストプラクティス、もしくは提案によってコードの状態がどのように改善されるかをちゃんと伝えると役立つことがある。
Giving Guidance
直接指示ではなく、問題を指摘して開発者に決定を任せることで、開発者の学習や、開発者＝コードの専門家からのより良いソリューションに繋がることがある。
ただし、直接的な指示、提案、コードの提示でも役立つことはある。コードレビューの主な目標は、可能な限り最高の CL を得ること、そして2番目が成長やレビューの削減に繋がること。
CL で気に入ったものを見つけたら、それらにもコメントする。レビュイーはリファクタリングやテストカバレッジの追加を、レビュアーは CL から何かをいつも学ぶ。いいね！を伝えて、レビュイーが良い実装を続けることを後押しする。
Accepting Explanations
理解できないコードの説明をレビュイーに依頼する際、レビュイーがコードをより明確に書き直すか、過度に複雑でなければコードにコメントを追加するように促せるとよい。
CL 上に説明を書くことは、将来のコード読者のためにはならないし大抵読まれない。しかし、自分自身があまり知らないながらレビューしていたり、すでに知ってるはずのことをあらためて説明するときなど、いくつかの状況では役立つ。
アイのムチの場合
「1.3 突き放し」の一文に、Giving Guidance と同じ趣旨を書いています。
答えをそのまま教えればよいとも限りません。キーワードや例のみ示し、実装者が自分で考え、答えはネットや本で見つけるほうが、スキルアップやモチベーションに繋がることがあります。
また、「1.9 よいレビューとは」に、
- 1つめは、建設的なフィードバック、健全で活発な議論があることです。コメントを書く人は、自分の考えを明確にしつつ、自分が正しいとは主張しないようにします。状況に応じていろんな代替案や回避策が存在することを認め、修正の強要を控え、どう思う？などと問いかけます。もし実装者から反対意見が出た場合は、本やインターネット、先人の知恵を見て、2人の間でよい着地点を探しましょう。それが難しそうなら、第三者に客観的な意見を求めるのがよいでしょう。
- 2つめは、レビュアーが実装者に共感していることです。いかなる方法でも、実装者が実装にかけた時間や労力を称賛し、強い口調ではなく親切で控えめな口調で接することが大切です。たくさんコメントした場合は、PR以外の場所、たとえば対面やオンライン会議、チャットやメールなどで積極的に連絡を取り、誤解が生じることを防ぎます。そうすればきっとつらい思いをする人は減り、レビューの雰囲気や会話の方向性がとてもポジティブなものとなるでしょう。
- 3つめは、すべての人が対等であることです。職種や職歴、スキルレベル、入社時期、サラリーなどに影響されず、全員が同じ基準、同じ目線で、かつお互い足りないものを埋める場所としてレビューを活用します。本書で取り上げたような、スキルテストや評価の場として利用することは不適切です。
と、Accepting Explanations 以外については同じようなことを書きました。
今いるメンバーのメンタル面の作用に着目しているので、将来の開発者に役立つレビューを行うという観点はあまりありませんでした。
Handling pushback in code reviews
概要
時々、レビュイーは反論したり、レビュアーの提案に同意しなかったり、厳しすぎると不平を言う。
Who is right?
何が正しいかを検討する。レビュイーはレビュアーよりもコードに近いので、具体的な洞察を持っているかもしれない。レビュイーの正しさは認める。
レビュアーが正しいと考える場合、返信への理解を示しながら、提案が正しい理由、追加情報を示すのが適切。
受け入れられるまで数回やりとりが発生したとしても、常に礼儀正しく、相手への理解を示す。同意しないだけ。
Upsetting Developers
レビュアーが改善を促すと開発者が動揺するのは、そういうものである。後でコード品質向上を感謝される。コメントが丁寧なら動揺は減る。
Cleaning It Up Later
レビュイーはいつも多くの作業を抱えており、クリーンアップを後回しにすると忘れがち。後回しはコード縮退への王道。
緊急の場合を除き、マージ前に CL のクリーンアップを求める。今すぐできない場合、クリーンアップをバグとして報告し、やり忘れないようにレビュイーにアサインしておく。コードにTODOコメントを書き残すのも手。
General Complaints About Strictness （厳しさへの不満）
緩いレビューから厳密なレビューに切り替えると、だいたい不満が出るが、やがてコードレビューの速度が改善すると、たいてい不満は消える。
不満が消えるまでに数ヶ月かかることもあるが、最終的には、レビュイーは厳密なコードレビューの価値を理解する傾向がある。時には、最も騒がしかった者が、あなたの最強の支持者になることさえある。
Resolving Conflicts
上記のすべてに従っても解決できない場合、The Standard of Code Review に立ち帰ろう。
アイのムチの場合
議論や不満への対処に正解を示すことはほぼ不可能なので、ひとつのストーリーを例として書くしかなかったです。
「1.6 独裁の場」から「1.7 論点がズレている」にかけて、アメさんとムチさんがレビュースタイルの相違について議論する場面があります。
この結末は、アメさんが自分の主張を引っ込めて従い始めるという、たぶん Google ガイドであまり想定されてない展開です（笑）。
それはムチさんの伝え方次第で変わったかもしれませんし、アメさんに議論を重ねるほどのこだわりがないのかもしれません。
とはいえ、アメさんもムチさんもレビュアーの観点で話をしているので、レビュイーで反論したり不満を持ったりするキャラクターは出てきません。
というか、新人のエマちゃんがレビュイーなので、不満があっても言い出せないって感じですね。
あとがき
アイのムチには、レビュアーの心掛けに関しては Google ガイドと同じ方向性のことが何割かは盛り込めており、解釈やレベルの違いはあれど、相反する内容はほぼ無かった、という個人結論でした。
双方に書かれてる分析はあくまで一般論で、Google ガイドにも「傾向がある」という書き方が多いわけですし、安易に自分の環境に当てはめての過信は禁物だと思いました。
特に最後の章は、動揺する…不満が出る…不満が消える…、そのへんは人の性格次第で変わります。
不満が出やすくても、レビュアー側でコントロールできることは限られており、レビュイー側で不満を溜め込まないような心がけも同時に必要です。
レビュイー編でそこを掘り下げようとと思います。
余談ですが、タックマンモデルにおいて、チームビルディングには形成期、混乱期、統一期、機能期がそれぞれあり、大抵のチームは混乱期から統一期にかけて、不満が高まる傾向が強いと言われています。
レビューを厳しくしてから効果的に実践できるようになるまでも、たいてい混乱期があります。それをアイのムチのストーリーを通して表現したつもりでしたが、反論するレビュイーを登場させて、レビュアー vs レビュイーの激しい戦いをもっと演出してもよかったですね（笑）
レビュアー編はここまでです。丸1日かけて書きました。読むのも長かったと思いますが、お疲れ様でした。
勢いで書いた部分は多いので、ここはどうなんだろう？みたいなのとか、Google 関係者さまからのご指摘などあればぜひ遠慮なくコメントしてください。