(2) The Standard of Code Review を読んでのメモ
Google が How to do a code review のドキュメントを公開しています。いくつかのセクションのうち、レビュアーのための1つ The Standard of Code Review | eng-practices を読んで、読み取ったものをメモします(訳ではない)。前回:Speed of Code Reviews を読んでのメモ - miso_soup3 Blog
※ 日本語に翻訳されたプロジェクトがあります: Google エンジニアリング・プラクティス ドキュメント | eng-practices
The Standard of Code Review
コードレビューの主な目的は、全体的なコードの健全性を時間ともに改善していくことを確かにすること。すべてのコードレビューのツールとプロセスはこのために設計される。
これを達成するには、一連のトレードオフのバランスを取らねばならない。
一方で、レビュアーの義務は、それぞれのCL(Change list)が、時間の経過とともにコードの全体的な健全性が低下しないような品質であることを確認すること。これは手間を要することだ。なぜならしばしば、コードベースは時間とともに小さな健全性の低下を通して劣化してく。特に、顕著な時間的制約下において、ゴール達成のためは短絡的な手段をとる必要がある。
また、レビュアーは、レビューするコードのオーナーシップと責任をもつ。彼らは、コードの一貫性、メンテナンス性、What to look for in a code review で述べている全てのことを確実にしたい。
したがって、コードレビューで我々が期待する標準として、次のようなルールが得られる:
In general, reviewers should favor approving a CL once it is in a state where it definitely improves the overall code health of the system being worked on, even if the CL isn’t perfect.
一般的に、レビュアーは、稼働中のシステムの健全性を確実に改善するという状態まで来たら、CLを承認することを好んだ方がよい。たとえそのCLが完璧でなくても。
これが、すべてのコードレビューのガイドラインの上位原則だ。
もちろん、これには制限がある。例えば、もしCLがレビュアーがシステム内にあってほしくないと思う機能を追加するようなCLであるとき、それが適切に設計されていたコードであっても、レビュアーは確実に承認を拒否できる。
ここで重要なことは、”完璧”なコードなどというものは存在しないこと、あるのはより良いコードだけ。レビュアーは、CLの著者に対して、こまごまと小さな欠片まですべて磨くまで承認を与えないというようなことを求めない方がいい。
むしろ、レビュアーは、前に進む必要性と、彼らが提案する変更の重要性を比較してバランスをとろう。 レビュアーが完璧を求める代わりに、継続的な改善が求められる。
CLは、全体として、システムのmaintainability・readability・understandabilityを改善する。「完璧ではない」ということを理由にして数日とか数週という単位で遅れるべきではないだろう。
レビュアーは、より良くなるためのコメントを残すことを気軽にいつも行うべきだが、もしそれがあまり重要ではない場合、”Nit(ちょっとしたことだけど)”などの接頭辞をつけ、単なる磨きのポイントに過ぎず、彼らが無視を選択できることだよと伝える。
Mentoring
コードレビューは、開発者に、言語・フレームワーク・一般的なソフトウェアデザイン設計について新しいことを教える重要な働きをもたらす。
開発者が新しいことを学ぶ助けになるコメントを残すことは、常に良いことだ。知識を共有することは、時間の経過とともにシステムのコードの健全性を改善させることの一部だ。
もしあなたのコメントが純粋に教育的(educational)であっても、このドキュメントで説明される基準に満たすために重要でない場合は、”Nit:”をつけるか、このCLで必ずしも解決すべきでないことを伝えることに、常に気をつけること。
Principles
- 技術的な事実とデータは、意見や個人的な好みを覆す
- スタイルの問題は、style guideが絶対的な権限である。スタイルガイドにない純粋なポイント(whitespaceなど)は、個人的な好みの問題だ。スタイルは、そこにあるものと一貫している必要がある。以前のスタイルがない場合は著者のものを受け入れる。
- ソフトウェアの設計の側面は、純粋なスタイルの問題でも個人的な好みの問題でもない。それらは基本的な原則にに基づき、個人的な意見によってではなく、それらの設計に重点を置くべき。ときどき、いくつかの有効な意見もある。もし、著者が(データを通じて、あるいは堅実なエンジニアリング原則に基づいて)、いくつかのアプローチが同等に有効であることを実証できる場合、レビュアーは著者の好みを受け入れるべきだ。それ以外の場合、選択は、ソフトウェアの設計の標準原則によって決められる。
- もし他のルールが適用されない場合、レビュアーは、現在のコードと一貫するように著者に求めることができる。それがシステム全体のコードの健全性を悪化させない限り。
Resolving Conflicts
コードレビューで衝突(メモ:gitの競合ではなくレビュー上の対立と思われる)が発生する場合は、開発者とレビュアーは、このドキュメントの内容と、The CL Author’s Guide and this Reviewer Guide、他のドキュメントに基づいて、常にコンセンサスを得ようとする必要がある。
コンセンサスを得ることが特に難しくなるときは、コードレビューのコメントを通して衝突を解決しようとする代わりに、直接対面で話すか、レビュアーと著者の間のVC(voice chat ?)を設けることで解決の助けとなり得る。(ただし、もしあなたがこれを行う場合、この後読む人のためにディスカッションの結果をCLのコメントに記録すること)
もしそれでも解決しない場合、もっとも一般的な解決方法はエスカレーションだ。多くの場合、エスカレーションのパスは、より広い範囲のチームのディスカッション、TL(Tech Lead ?)に渡すこと、コードのメンテナーの意見を訪ねること、Eng Manager(?) の助けを求めることだ。著者とレビュアーが合意に達することができないからといって、CLを放置しないこと。
Speed of Code Reviews を読んでのメモ
Google が How to do a code review のドキュメントを公開しました。いくつかのセクションのうち、レビュアーのための1つ Speed of Code Reviews を読んで、読み取ったものをメモします。
※ 日本語に翻訳されたプロジェクトがあります: Google エンジニアリング・プラクティス ドキュメント | eng-practices
なぜコードレビューは早くするべきか?
Googleは、”個人”の開発者のコードの書く早さを最適化するのではなく、開発者の”チーム”が一緒にプロダクトを作る早さを最適化する。個人の早さは重要だが、チーム全体のベロシティほど重要ではない。
レビューが遅いとき、次のことが発生する:
- チーム全体の速度が低下する
- レビューにすぐに反応しない人は個人として別の仕事を完了できる。ただし、他のチームメンバーの機能開発やバグフィックスは、レビューと再レビューを待ち、数日、数週間あるいは数ヶ月遅れる。
- レビューのプロセスに不服を唱える
- レビュアーが数日毎にレスポンスしたとき、そしてその度に大きな変更を要求するとき、開発者はイライラして苦しくなる。これは、レビュアーの厳格さに関する苦情となる。もし同じような変更を要求しても、それが素早いレスポンスであれば、その苦情は消える傾向がある。コードレビューのプロセスの苦情のほとんどは、素早い対応で解決される。
- コードの健全性に影響を与える可能性がある
- 遅いレビューは、コードのクリーンアップ、リファクタリング、改善を妨げる
どれくらい早くするべきか?
集中しているタスクの途中でない場合は、レビューがきたらすぐに行う。
最大、1営業日までに応答する。つまり、翌朝の最初に行う。
これは、典型的なコード変更は、1日以内に複数回のレビューを受ける必要があることを意味する。
早さ vs. 割り込み
個人のベロシティを考慮することが、チームのベロシティに勝る場合がある。もし、集中しているタスクの途中の場合は、自身をコードレビューで中断しないこと。
コーディング中に中断することは、別の開発者にコードレビューを待ってもらうことよりも、実際にはチームにとってコストがかかる。
代わりにブレイクポイントを活用する。例えば、コーディングが完了したとき、昼食の後、キッチンから戻ってきたときなど。
早いレスポンス
ここでの早さとは、レビューが通過するまでの時間の早さではなく、個々のレビューに応答しているときの時間早さを言う。
全体のプロセスも高速である必要があるが、それよりも、個々の応答が迅速になることが重要。
忙しくてレビューをできない場合:
- 開発者にいつできるかを知らせるクイックなレスポンスを送信
- より迅速に対応できる他のレビュアーを提案
- 最初に大雑把なコメントを提示する provide some initial broad comments
コメント付きのLGTM
コードレビューを高速化するために、まだ未解決なコメントを残していたとしても、LGTM/Approval を与えるべき特定の状況がある。これは以下のいずれか:
- レビュアーは、開発者が残りのコメントに適切に対処すると確信しているとき
- 残りの変更はマイナーで、その開発者によって行われる必要はないとき
レビュアーは、上記のいずれであるかを明記しよう。
コメント付きのLGTMは、例えば、レビュアーと開発者が違うタイムゾーンにいるとき特に一考価値がある。そうでなければApprovalを得るために丸1日待つことになってしまう。
コードの変更は小さく
レビューをする時間があるかどうか分からないほど誰かが大きなコードレビューを送ったとき、あなたの典型的な応答は、開発者に小さなコードの変更に分割することを求めること。
もし、コードの変更を小さく分割できなかったり、あなたが全体をすぐにレビューする時間がないときは、少なくとも、全体的な設計に関するコメントを書いて、開発者に改善のために送り返すこと。
レビュアーのゴールの1つは、開発者をブロックしないこと。開発者がコードの健全性を犠牲にすることなく、追加のアクションを迅速に実行できるようにすること。
長期にわたるコードレビューの改善
これらのガイドラインに従って厳格にレビューしたとき、コードレビューのプロセス全体が時間とともに早くなる傾向があることに気づくはず。
開発者は、健全なコードに必要なものを学び、最初からすぐれたコード変更を送信し、レビュー時間を短縮する。
レビュアーは、遅延せずに迅速に対応することを学ぶ。
ただし、ベロシティを早くするために、code review standards や品質に妥協してはいけない。それは長期的にみれば、実際には何も実現していない。