Google eng-practices とアイのムチの比較（レビュイー編）

はじめに

レビュアー編の続き、後半です。

前回に引き続き Google の Code Review Developer Guide と、アイのムチ よくないレビューの例とレビューで折れないメンタルづくりの内容比較です。
Google ガイドの方を私の解釈を意訳でまとめながら、アイのムチの「レビューで折れないメンタルづくり」との違いを見ていきます。

Writing good CL descriptions

概要

CL（Change List。Pull Requestと読み替えてもよい） の説明は、レビュー後もずっと残り続ける。
将来のコード検索者が読むことを想定して書こう。

First Line

最初の行は、内容を短く、焦点を絞り、要点を命令形式で書く。
（例：Deleting the FizzBuzz RPC and replacing it with the new system.ではなくDelete the FizzBuzz RPC and replace it with the new system.）
その後に空白行を入れ、最初の行は独立させる。
将来のコード検索者が説明全体を読まなくても、最初の行で何をした CL かがわかり、素早く把握できるようにするため。

Body is Informative （体は有益）

残りは CL の理解に必要な補足情報を書く。解決した問題の説明と、これが最善のアプローチである理由から成る。
アプローチの短所もあれば言及する。
必要に応じて、バグ番号、ベンチマーク結果、設計書へのリンクなども含める。
将来リンク切れする可能性は考慮すること。

Bad CL Descriptions

Fix bugだと、どんなバグでどうやって修正したのかがわからない。他のよくない例は、

• Fix build.
• Add patch.
• Moving code from A to B.
• Phase 1.
• Add convenience functions.
• kill weird URLs.

など。短いのはよいが、有用な情報を十分に提供していない。

Good CL Descriptions

Functionality change （機能変更）

rpc: remove size limit on RPC server message freelist.

Servers like FizzBuzz have very large messages and would benefit from reuse.
Make the freelist larger, and add a goroutine that frees the freelist entries 
slowly over time, so that idle servers eventually release all freelist entries.

rpc： RPCサーバーのメッセージフリーリストのサイズ制限を削除。

FizzBu​​zzのようなサーバーは非常に大きなメッセージを持っており、再利用の恩恵を受けるであろう。
フリーリストを大きくし、時間の経過とともにフリーリストエントリーをゆっくりと解放するゴルーチンを追加して、
アイドル状態のサーバーが最終的にすべてのフリーリストエントリーを解放するようにする。

最初の行で、CL が実際に行うことを説明する。
残りで、解決する問題、この方法がいい理由、特定の部分に関する補足を書く。

Refactoring

Construct a Task with a TimeKeeper to use its TimeStr and Now methods.

Add a Now method to Task, so the borglet() getter method can be removed 
(which was only used by OOMCandidate to call borglet’s Now method). 
This replaces the methods on Borglet that delegate to a TimeKeeper.

Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. 
Eventually, collaborators that depend on getting Now from the Task should be changed 
to use a TimeKeeper directly, but this has been an accommodation to refactoring in 
small steps.

Continuing the long-range goal of refactoring the Borglet Hierarchy.

TimeStrとNowのメソッドを使用するために、TimeKeeperを使用してタスクを作成。

NowメソッドをTaskに追加して、borglet（） getterメソッドを削除できるようにする。
（これは、OOMCandidateがborgletのNowメソッドを呼び出すためにのみ使用されていた）
これは、TimeKeeperに委任するBorgletのメソッドを置き換える。

TasksにNowの供給を許可することは、Borgletへの依存を排除​​するためのステップである。
最終的には、タスクからNowを取得することに依存しているコラボレーターは、TimeKeeperを直接使用するように
変更する必要があるが、これは小さなステップでのリファクタリング対応である。

Borglet Hierarchyをリファクタリングするという長期的な目標を継続する。

最初の行で、CL が行うことと、書き換えである旨を書く。
残りで、まだ理想的じゃないが将来の計画があることと、今回変更する理由を説明する。

Small CL that needs some context （コンテキストが必要な小さな CL）

Create a Python3 build rule for status.py.

This allows consumers who are already using this as in Python3 to depend on a rule 
that is next to the original status build rule instead of somewhere in their own tree. 
It encourages new consumers to use Python3 if they can, instead of Python2, and 
significantly simplifies some automated build file refactoring tools being worked on 
currently.

status.pyのPython3ビルドルールを作成します。

これにより、Python3のように既にこれを使用しているconsumerは、独自のツリーのどこかではなく、
元のステータスビルドルールに基づくルールに依存できます。
これにより、新しいconsumerは、可能な限りPython2に代わってPython3を使用することとなり、
現在作業中の自動ビルドファイルのリファクタリングツールが大幅に簡素化されます。

最初の行で、CL が行うことを書く。
残りで、変更の理由やその前後関係を見せる。

Generated CL descriptions （生成された CL の説明）

ツールが CL を生成することもあるが、それも可能な限り、このガイドに従う。

Review the description before submitting the CL （CL を適用する前に内容をレビューしよう）

CL に書くべきことはレビュー中に大幅に変わることがあるので、CL 投稿前、つまりマージ前に説明をアップデートする。

アイのムチの場合

テーマが「レビューで折れないメンタルづくり」なので、わかりやすい CL を書こう！という話はテーマ外だから取り上げませんでした。

個人としては、Google ガイドに全く異論なくて、とても良い指針だと思います。

Small CLs

Why Write Small CLs?

小さくて単純な CL は、

• より迅速にレビューできる
  • レビュアーは30分×1回よりも5分×数回のほうがありがたい
    ＊ より徹底的にレビューできる
  • 大きな変更は重要なポイントが見落とされがち
• バグの可能性が低くなる
  • 変更が少なければバグの確認も簡単になる
• 無駄になった場合、無駄が少なくなる
  • 巨大な CL を作成後に全部無駄だとわかると目も当てられない
• マージが簡単
  • 巨大な CL はマージするときに多くの競合が発生する
• 簡単にうまく設計できる
  • 大きな変更の全詳細を洗練するより、小さな変更の設計とコードの健全性を磨く方がはるかに簡単
• レビューのブロッキングが少なくなる
  • 現在の　CL　のレビューを待つ間、次の CL のコーディングをできる
• ロールバックが簡単
  • 大きな CL はピンポイントでロールバックできない

レビュアーは、大きな CL の NG や、分割を要求してくることがある。
後で分割するより、最初から小さな CL を書くほうが簡単。

What is Small?

CL の適切なサイズは、1つの自己完結型の変更になるくらい。具体的には、

• CL は、1つのことだけに対する最小限の変更をする
  • 通常、機能の一部だけ。わからなければ、レビュアーと協力して、適度なサイズを決める
• CL は、関連するテストコードを全て含む
• レビュアーが CL について理解するための全てを含む
• CL 適用後も、システムはユーザーと開発者にとって引き続き正常に機能する
• 新しい API を追加する場合は、同じ CL が API の使用法を含み、レビュアーが API をよく理解できるようになっている
  • CL が小さくないと、API　の意味を理解するのも困難
  • 未使用の API の混入も防止できる

サイズの厳格なルールはないが、一般的に 1000 行は大きすぎるし、50 のファイルにまたがるのも大きすぎる。

コードに深く関わってきたレビュイーにとっては許容できるサイズでも、前後関係を知らないレビュアーにとっては大きすぎるかもしれない。
疑わしい場合は、自分の感覚より小さい CL を作成する。
レビュアーが CL が小さいと不満を言うことは滅多にない。

When are Large CLs Okay?

大きな変更が悪くない状況はある。

• ファイル全体の削除
  • ファイルの削除は1行の変更として数えてよい
• 信頼できる自動リファクタリングツールによる大きな CL
  • レビュアーの仕事は、変更が本当に必要かどうかの確認のみ

Splitting by Files （ファイルによる分割）

CL を分割する方法の1つが、ファイルごとの CL 。
例えば、プロトコルバッファの変更 CL と、それを使用するコードの CL を分けると、マージの順序は気をつけなければならないが、別々のレビュアーに見せたり、前後関係を把握しやすくできる。

Separate Out Refactorings （リファクタリングを分離）

リファクタリングは機能変更やバグ修正とは別の CL で行うほうが明らかにレビューしやすい。
ただし、ローカル変数名の修正などの小さなものは、機能変更やバグ修正に入れてよい。

Keep related test code in the same CL （関連するテストコードを同じ CL に含める）

CL には、関連するテストコードを含める必要がある。
テストが存在しなければ、追加する必要がある。
以下の場合、テストの変更を別の CL に入れることもできる。

• 新しいテストで、既存のコードを検証する
  • リファクタリング CL の前に追加テスト CL をマージしておくと、リファクタリングで動作が変わらない証明になる
• テストコードのリファクタリング
• 異なるテストフレームワークの導入

Don’t Break the Build

CL が送信された後もビルドが通ること。そうしないと、次の CL まですべての開発者のビルドに影響する。

Can’t Make it Small Enough

CL を小さくできるのに大きいほうが良いケースはほとんどない。
ここでのガイドがすべて失敗した場合（非常にまれだが）、事前にレビュアーに同意を得て大きな CL をレビューに回す。
この場合、レビューに時間がかかるのを受け入れ、バグを導入しないように注意し、テストの作成には特に注意を払う。

アイのムチの場合

CL はできるだけ小さくしましょうという話も、これもアイのムチで全く書かなかった観点です。
確かに、よいレビューをするにはレビュアーにとってレビューしやすい状態で出されてることも大事ですよね。

とはいえ、やはりテーマが「レビューで折れないメンタルづくり」なので、それを書くとテーマ外になっちゃうのでした。
色々盛り込みたかった反面、メンタル面の本だという希少性をサブタイトルで感じて手に取ってもらうのも大事だと考えたので…。

How to handle reviewer comments （レビュアーからのコメントの処理方法）

来ました！ここがアイのムチ2章で取り上げたメインテーマ相当です。

Don’t Take it Personally （個人宛だととらえない）

レビューの目的は、コードベースと製品の品質を維持することで、個人の能力を攻撃したり評価したりすることではない。

レビュアーが不満をコメントで表現することは、よくないレビューだが、レビュイーはこれに備えておく必要がある。
「レビュアーが私に伝えようとした建設的なことは何か？」と自問し、それに従って行動する。
どうしても怒りで反応しそうなら、しばらくPCから離れるか、丁寧に返信できるようになるまで別の作業をする。

レビュアーからのフィードバックが建設的で礼儀正しいものじゃないと感じた場合は、レビュアーに直接またはビデオチャットで（難しければテキストで）、そのやり方が嫌いなことや、違うやり方でやってほしいことを礼儀正しく説明する。
それにさえ非建設的に応答してくる場合、または効果がなさそうな場合は、必要に応じて上司にエスカレーションする。

Fix the Code

レビュアーがコードの特定箇所を理解していないと言った場合、将来のコード読者も理解できない可能性があるので、レビュイーが最初にやることは、コード自体を理解可能なものに直すことである。
それが難しい場合は、コードの存在理由を説明するコードコメントを追加する。
コメントを書いても無意味に感じる場合に限り、コードレビューツール上で説明する。

Think Collaboratively （一緒に考える）

頑張って CL を作ったのに、同意されなくて変更を求めるコメントを受け取った場合は、レビュイーはイライラするものである。
Don’t Take it Personallyに従った上で、レビュアーからのコメントを理解できても同意できない場合は、戦ったり保身するのではなく、協力して考える。

悪い例：「いいえ、そうはしません。」

良い例：「[これらの長所/短所]と[これらのトレードオフ]のためにXを使用しました。私の理解では、[これらの理由]のためにYを使用するのはよくないと思っています。◯◯さんは、Yが元のトレードオフをうまく処理できる、トレードオフを別の方法で評価する必要がある、あるいは他の意見がありますか？」

礼儀と敬意は常に最優先事項。
レビュアーに同意できない場合は、共同作業として説明を求め、賛否両論について話し合い、コードベース、ユーザー、またはGoogleにとってこのやり方がなぜ優れているのかを明らかにする。

もしかすると、レビュアーがこちらの知らないユーザー、コードベース、または CL について何か知っているかもしれない。
それがあれば、必要に応じてコードを修正し、レビュアーにより多くの前後背景を伝えてディスカッションに参加させる。
そうすれば通常は、技術的な事実に基づいて、レビュイーとレビュアーの間で合意に達するはず。

アイのムチの場合

アイのムチの2章では、レビュイーの心構えとして、次の3本柱を提起しました。

• 心理的安全性を高め、無知/無能/邪魔/ネガティブだと思われる不安を減らす
  • 失敗が起こることを受け入れる
  • 人や過去を責めず、プロセスやこれから成功する方法を考える
  • うまくいかないのは学びや成長の機会だととらえる
  • 指摘は一定レベルを超えてるから受けるもので、土台には好評価と承認があるととらえる
  • 相手に好奇心を持ち、質問や意見を積極的にする
  • 予想と異なることに好奇心や関心を持つ
• 完璧や正解を求めすぎない
  • 全員が全部やりすぎると時間を取られすぎたり心身を壊したり、成果が出ないチームになるので、予算/時間/品質/スコープなどのトレードオフスライダーをチーム内で前もって作っておく
• 戦いをせず協力を求める
  • 言葉の隅々を受け止めず、「イケてない」とか一言多い部分はスルーし、コメントのどこが改善や成長につながるかを読み取る
  • 必要以上に自分の正当性を主張しない。戦いではなく協力作業、レビュアーは味方であると常に意識する
  • とはいえ、人は感情に左右されるものなので、儀式をして気持ちを切り替える

Google ガイドと比較すると、Don’t Take it Personally や Think Collaboratively は概ね同じことを書いてますが、こちらからも礼儀正しく受け答えようとか、スルーできなかったら席を立ったり別の作業をしようとか、アイのムチでは思いつかなかった Tips もあって確かに…！と思わされました。

また、Fix the Code は「必要以上に自分の正当性を主張しない」の一文で伝えたかったことが近いのかなと思いましたが、より掘り下げると Google ガイドに近い内容になりますね。
開発者にかかっているバイアスを抜くのがレビューの目的の1つなのだから、議論や反論があってもこれは協力作業の状態なので、自分にできる限りのことは悩まずにパパッとやっちゃいましょう、ってことだと思います。

あとがき

大体レビュアー編のあとがきに書いちゃったので、今回何を書くか迷うところですが…笑

Google ガイド側がとても充実していたので、ここはこうじゃない？とか言うことは前回同様ほとんどなくて。
でも、アイのムチはストーリー形式だからこそ、経験の浅いレビュイーにも伝わりやすかったんじゃないかと思います。

アイのムチという本を通して伝えたかったポイントは、

• チームにはいろんな性格や価値観の人がいて、それぞれが異なる強みや課題をもってて、みんな最終的には自分の行動を自分で選んでいる
• レビューや組織運営には全員が全部を完璧にできる、あるいは全員の居心地がよくなるような解決策はない
• 日常の対話やふりかえりなどを通じて、各々が「聞きたい」「話したい」「やりたい」と感じられる状態になれば、チームはうまくまわるはず
• レビューはソフトウェアの品質を高める作業であると同時に、相互理解、改善、良いチームづくりのきっかけの1つでもある

みたいなことなんですが、それをバイブルのように伝えたいわけじゃなくて、例えばムチさんみたいな人いるからこんなふうに接してみようかな〜とか、こういうチームづくりいいな〜とか、逆に俺たちならもっとうまくやれるわ！とか、何か1つでも感想を持っていただければもう十分嬉しいです。

いつかまた同じテーマを書くことがあれば、より Google ガイドっぽいガチな内容に近づけても良いかなと思いつつ、今回の比較企画を終わります。