miso_soup3 Blog

主に ASP.NET 関連について書いています。

(2) The Standard of Code Review を読んでのメモ

GoogleHow 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を放置しないこと。