#Google エンジニアリングプラクティスドキュメント

2019 年 9 月に Google が自社用のエンジニアリングプラクティスのコードレビューを Github に公開しました。役に立ちそうなので、日本語に訳しながらメモりました。

Google には、すべての言語とすべてのプロジェクト。これらのドキュメントは、さまざまな私たちが時間をかけて開発したベストプラクティス。そのオープンソースが可能ですプロジェクトや他の組織はこの知識の恩恵を受けるため、可能な場合は一般公開します。

#コードレビューアガイド

#前書き

コードレビューは、コードの作成者以外の誰かがそのコードを調べるプロセスです。

Google では、コードレビューを使用して、コードと製品の品質を維持しています。

このドキュメントは、Google のコードレビュープロセスとポリシーの標準的な説明です。

このページは、コードレビュープロセスの概要です。このガイドの一部である他の 2 つの大きなドキュメントがあります。

コードレビューの実行方法:コードレビュー担当者向けの詳細ガイド。
CL Author's Guide:CL のレビューを行っている開発者向けの詳細ガイド。

#コードレビューアは何を求めますか?

コードレビューでは次を確認する必要があります。

  • 設計:コードは適切に設計され、システムに適していますか?
  • 機能:コードは作成者が意図したとおりに動作しますか?ユーザーにとってコードの動作は良いですか?
  • 複雑さ:コードをより単純にできますか?別の開発者が将来このコードに遭遇したときに、このコードを簡単に理解して使用できるでしょうか?
  • テスト:コードには、適切で適切に設計された自動テストがありますか?
  • 命名:開発者は変数、クラス、メソッドなどの明確な名前を選択しましたか?
  • コメント:コメントは明確で有用ですか?
  • スタイル:コードはスタイルガイドに従っていますか?
  • ドキュメント:開発者は関連ドキュメントも更新しましたか?

#最高のレビュアーを選ぶ

一般に、妥当な期間内にレビューに返信できる最高のレビュアーを見つけたいと思います。

最高のレビュアーは、あなたが書いているコードの部分について最も徹底的かつ正確なレビューをあなたに与えることができる人です。これは通常、コードの所有者を意味します。所有者は、OWNERS ファイルの人である場合とそうでない場合があります。これは時々、CL の異なる部分をレビューするように異なる人々に依頼することを意味します。

理想的なレビュアーを見つけても利用できない場合は、少なくとも変更について CC する必要があります。

#パーソンレビュー

適切なコードレビューを行う資格のある人とコードをペアプログラミングした場合、そのコードはレビュー済みと見なされます。

また、レビュアーが質問し、変更の開発者が話されたときにのみ発言する、対面のコードレビューを行うこともできます。

#コードレビューを行う方法

このセクションのページには、長年の経験に基づいて、コードレビューを行う最適な方法に関する推奨事項が含まれています。 これらはすべて 1 つの完全なドキュメントを表し、多くの個別のセクションに分かれています。 すべてを読む必要はありませんが、多くの人々は、セット全体を読むことは自分自身とチームにとって非常に役立つと感じています。

#コードレビューの基準

コードレビューの主な目的は、Google のコードベースの全体的なコードの健全性が時間とともに改善されることを確認することです。コードレビューのすべてのツールとプロセスは、この目的のために設計されています。

これを達成するには、一連のトレードオフのバランスをとる必要があります。

まず、開発者は自分のタスクを進めることができなければなりません。コードベースの改善を送信しない場合、コードベースは改善されません。また、レビュー担当者が何らかの変更を加えることを非常に困難にした場合、開発者は将来改善する意欲を失います。

一方、各 CL が、時間の経過とともにコードベースの全体的なコードの健全性が低下しないような品質であることを確認するのは、レビューアーの義務です。多くの場合、コードベースは時間の経過とともにコードの正常性がわずかに低下することにより低下することが多く、特にチームがかなりの時間的制約下にあり、目標を達成するためにショートカットを取る必要があると感じている場合、これは難しい場合があります。

また、レビュー担当者は、レビューするコードの所有権と責任を負います。彼らは、コードベースの一貫性、維持可能性、および「コードレビューで探すべきもの」で言及した他のすべてのことを確実にしたいのです。

したがって、コードレビューで期待される標準として次のルールを取得します。

一般に、レビュー担当者は、CL が完全ではない場合でも、作業中のシステムの全体的なコードの健全性が確実に改善される状態になったら、CL の承認を支持する必要があります。

これが、すべてのコードレビューガイドラインの上位原則です。

もちろん、これには制限があります。たとえば、CL がシステムにレビュアーが必要としない機能を追加した場合、コードが適切に設計されていても、レビュアーは確実に承認を拒否できます。

ここで重要なことは、「完璧な」コードなどというものは存在しないということです。より良いコードしかありません。レビューアは、承認を許可する前に、著者に CL のすべての小さな断片を磨くことを要求すべきではありません。むしろ、レビューアは、彼らが提案している変更の重要性と比較して、前進する必要性のバランスをとるべきです。レビュアーが完璧を求める代わりに、継続的な改善が求められます。全体として、システムの保守性、読みやすさ、および理解可能性を向上させる CL は、「完全」ではないため、数日または数週間遅らせるべきではありません。

レビュー担当者は、何かがより良いものになる可能性があることを示すコメントをいつでも気軽に投稿する必要がありますが、あまり重要でない場合は、「Nit:」などの接頭辞を付けて、無視することを選択できるのは単なる洗練点であることを著者に知らせます。

注:このドキュメントには、システムのコード全体の健全性を確実に悪化させる CL のチェックを正当化するものはありません。それを行うのは、緊急時のみです。

#メンタリング Mentoring

コードレビューは、開発者に言語、フレームワーク、または一般的なソフトウェア設計原則について新しいことを教える重要な機能を持つことができます。開発者が何か新しいことを学ぶのに役立つコメントを残すことは常に問題ありません。知識を共有することは、時間の経過とともにシステムのコードの健全性を向上させることの一部です。あなたのコメントが純粋に教育的であるが、このドキュメントで説明されている基準を満たすために重要ではない場合、「Nit:」をプレフィックスとして付けるか、著者がこの CL でそれを解決することが必須ではないことを示すことに留意してください。

#原則 Principles

  • 技術的事実とデータは意見や個人的な好みを覆します。

  • スタイルについては、スタイルガイドが絶対的な権限です。スタイルガイドにない純粋なスタイルポイント(空白など)は、個人的な好みの問題です。スタイルはそこにあるものと一致している必要があります。以前のスタイルがない場合は、著者のものを受け入れます。

  • ソフトウェア設計の側面は、純粋なスタイルの問題でも個人的な好みでもありません。それらは基本的な原則に基づいており、単に個人的な意見によってではなく、それらの原則に重点を置くべきです。有効なオプションがいくつかある場合があります。著者が(データを通じて、または堅実な工学原理に基づいて)いくつかのアプローチが同等に有効であることを実証できる場合、査読者は著者の選好を受け入れるべきです。それ以外の場合、選択はソフトウェア設計の標準原則によって決まります。

  • 他のルールが適用されない場合、レビュアーは、システムのコード全体の健全性を悪化させない限り、著者に現在のコードベースにあるものとの一貫性を求めることができます。

#競合の解決 Conflicts

コードレビューの競合が発生した場合、開発者とレビューアは、このドキュメントの内容と 『CL Author's Guide』およびこのレビューアガイドの他のドキュメントに基づいて、常にコンセンサスを得ようとする必要があります。

コンセンサスに達することが特に難しくなった場合、コードレビューコメントを通じて競合を解決しようとするのではなく、レビュアーと著者の間で対面ミーティングまたは VC を開催することができます。 (ただし、これを行う場合は、ディスカッションの結果を必ず記録してください

#コードレビューで何を探すべきか

#設計 Design

レビューで取り上げる最も重要なことは、CL の全体的な設計です。 CL 内のさまざまなコードの相互作用は意味がありますか?この変更はコードベースまたはライブラリに属しますか?システムの他の部分とうまく統合されていますか?今、この機能を追加する良い時期ですか?

#機能性 Functionality

この CL は開発者が意図したことを行いますか?このコードのユーザーにとって、開発者が意図したものは良いですか? 「ユーザー」は通常、エンドユーザー(変更の影響を受ける場合)と開発者(将来このコードを「使用」する必要があるユーザー)の両方です。

ほとんどの場合、開発者はコードレビューを行うまでに CL が正しく機能することを十分にテストする必要があります。ただし、レビュー担当者としては、エッジケースについて検討し、同時実行の問題を探し、ユーザーのように考えて、コードを読むだけでバグが表示されないようにする必要があります。

必要に応じて CL を検証できます。レビュアーが CL の動作を確認することが最も重要なのは、UI の変更など、ユーザーに影響を与える場合です。コードを読んでいるときに、一部の変更がユーザーにどのような影響を与えるかを理解するのは困難です。そのような変更については、CL にパッチを適用して自分で試してみるのが不便な場合、開発者に機能のデモを提供してもらうことができます。

コードレビュー中に機能について考えることが特に重要なのは、CL で理論的にデッドロックまたは競合状態を引き起こす可能性のある並列プログラミングが行われている場合です。これらの種類の問題は、コードを実行するだけでは検出が非常に難しく、通常、問題が発生しないことを確認するために誰か(開発者とレビュアーの両方)を慎重に検討する必要があります。 (これは、競合状態やデッドロックが発生する可能性がある並行性モデルを使用しない理由でもあります。コードのレビューやコードの理解が非常に複雑になる可能性があります。)

#複雑 Complexity

CL は本来よりも複雑ですか? CL のすべてのレベルでこれを確認します。個々の行は複雑すぎますか?機能は複雑すぎますか?クラスは複雑すぎますか? 「複雑すぎる」とは、通常「コードリーダーがすぐに理解できない」ことを意味します。また、「開発者がこのコードを呼び出したり、変更しようとすると、バグが発生する可能性が高い」ことも意味します。

特定の種類の複雑さは、開発者がコードを必要以上に汎用的にしたり、システムが現在必要としない機能を追加したりするオーバーエンジニアリングです。レビュー担当者は、過剰なエンジニアリングに特に注意する必要があります。開発者が将来解決する必要があると推測する問題ではなく、今すぐ解決する必要があるとわかっている問題を解決するように開発者に奨励します。将来の問題は到着したら解決する必要があり、実際の形状と要件を物理的な宇宙で見ることができます。

#テスト Tests

変更に適した単体テスト、統合テスト、またはエンドツーエンドテストを依頼します。一般に、CL が緊急事態を処理していない限り、テストは製品コードと同じ CL に追加する必要があります。

CL のテストが正しく、適切で、有用であることを確認してください。テストはそれ自体をテストせず、テスト用のテストを作成することはほとんどありません。テストが有効であることを人間が確認する必要があります。

コードが壊れると、テストは実際に失敗しますか?コードがそれらの下で変更された場合、それらは誤検知を生成し始めますか?各テストは単純で有用なアサーションを作成しますか?テストは異なるテスト方法間で適切に分離されていますか?

テストも維持する必要があるコードであることに注意してください。テストがメインバイナリの一部ではないという理由だけで、テストの複雑さを受け入れないでください。

#ネーミング Naming

開発者はすべてに良い名前を付けましたか?良い名前は、読みにくくなるほど長くなくても、アイテムが何であるか、何をするかを完全に伝えるのに十分な長さです。

#コメント Comments

開発者はわかりやすい英語で明確なコメントを書きましたか?コメントはすべて実際に必要ですか?通常、コメントは、一部のコードが存在する理由を説明するときに役立ち、一部のコードが何をしているのかを説明すべきではありません。コードがそれ自体を説明するほど明確でない場合は、コードをよりシンプルにする必要があります。いくつかの例外があります(正規表現や複雑なアルゴリズムは、多くの場合、実行していることを説明するコメントから大きな恩恵を受けます)が、ほとんどのコメントは、決定の背後にある理由のように、コード自体には含まれない情報のためのものです

また、この CL の前にあったコメントを見ることも役立ちます。すぐに削除できる TODO や、この変更に対するアドバイスなどがあります。

コメントはクラス、モジュール、または関数のドキュメントとは異なることに注意してください。代わりに、コードの目的、使用方法、使用時の動作を表現する必要があります。

#スタイル Style

Google には、すべての主要言語、およびほとんどのマイナー言語のスタイルガイドがあります。 CL が適切なスタイルガイドに従っていることを確認してください。

スタイルガイドにないスタイルポイントを改善したい場合は、コメントの前に「Nit:」を付けて、開発者にコードが改善されると思うが、必須ではないことを確認してください。個人のスタイル設定のみに基づいて、CL の送信をブロックしないでください。

CL の作成者は、主要なスタイルの変更を他の変更と組み合わせて含めるべきではありません。 CL で何が変更されているのかがわかりにくくなり、マージとロールバックがより複雑になり、他の問題が発生します。たとえば、作成者がファイル全体を再フォーマットする場合は、再フォーマットを 1 つの CL として送信し、その後、機能変更を行った別の CL を送信するように依頼します。

#ドキュメンテーション Documentation

CL がユーザーによるコードの作成、テスト、操作、またはリリースの方法を変更する場合は、README、g3doc ページ、および生成された参照ドキュメントなどの関連ドキュメントも更新することを確認します。 CL がコードを削除または廃止する場合は、ドキュメントも削除する必要があるかどうかを検討してください。ドキュメントが見つからない場合は、それを求めます。

#すべての行 EveryLine

レビューに割り当てられたコードのすべての行を見てください。データファイル、生成されたコード、または時々スキャンできる大きなデータ構造などがありますが、人が書いたクラス、関数、またはコードブロックをスキャンせず、その中に問題がないと想定します。明らかに、一部のコードは他のコードよりも綿密な精査に値します-それはあなたがしなければならない判断の呼び出しです-しかし、少なくともすべてのコードが何をしているのかを確実に理解する必要があります。

コードを読むのが難しすぎてレビューが遅くなる場合は、開発者にそれを知らせて、レビューを試みる前に明確にするのを待ってください。 Google では、優秀なソフトウェアエンジニアを雇用しており、あなたもその 1 人です。コードを理解できない場合、他の開発者も理解できない可能性が高くなります。したがって、開発者にこのコードを明確にするよう依頼することで、将来の開発者がこのコードを理解するのを支援することにもなります。

コードを理解しているが、レビューの一部を行う資格がないと感じる場合は、特にセキュリティ、並行性、アクセシビリティ、国際化などの複雑な問題について、資格のあるレビュアーが CL にいることを確認してください。

#コンテキスト Context

多くの場合、CL を広いコンテキストで見ると役立ちます。通常、コードレビューツールは、変更されている部分の数行のコードのみを表示します。変更が実際に理にかなっていることを確認するために、ファイル全体を確認する必要がある場合があります。たとえば、新しい行が 4 行しか追加されていないように見える場合がありますが、ファイル全体を見ると、これらの 4 行は 50 行のメソッドであり、実際にはより小さなメソッドに分割する必要があります。

システム全体のコンテキストで CL について考えることも役立ちます。この CL は、システムのコードの健全性を改善していますか、それともシステム全体をより複雑にしたり、テストを減らしたりしていますか?システムのコードの健全性を低下させる CL を受け入れないでください。ほとんどのシステムは、多くの小さな変更が追加されると複雑になります。そのため、新しい変更では小さな複雑さでも防止することが重要です。

#良いもの GoodThings

CL で何か良いものを見つけたら、特にコメントの 1 つをすばらしい方法で扱ったときに、開発者に伝えてください。コードレビューは多くの場合、間違いに焦点を合わせていますが、グッドプラクティスに対する励ましと感謝も提供する必要があります。メンタリングの観点からすると、デベロッパーに、自分が間違ったことを伝えるよりも、自分が正しいことを伝える方がさらに価値がある場合があります。

#概要 Summary

コードレビューを行う際には、次のことを確認する必要があります。

  • コードは適切に設計されています。
  • この機能は、コードのユーザーに適しています。
  • UI の変更はすべて賢明で見栄えが良いです。
  • 並列プログラミングはすべて安全に行われます。
  • コードは必要以上に複雑ではありません。
  • 開発者は、将来必要になるかもしれないものを実装していませんが、現在必要なことを知りません。
  • コードには適切な単体テストがあります。
  • テストは適切に設計されています。
  • 開発者はすべてに明確な名前を使用しました。
  • コメントは明確で有用であり、主に何ではなく理由を説明します。
  • コードは適切に文書化されています(通常 g3doc にあります)。
  • コードはスタイルガイドに準拠しています。

確認するように求められたコードのすべての行を確認し、コンテキストを確認し、コードの状態を改善していることを確認し、開発者が行っている良いことを称賛してください。

#レビュー中の CL のナビゲート

概要 Summary

何を探すべきかがわかったので、複数のファイルにまたがるレビューを管理する最も効率的な方法は何ですか?

  • 変更は意味がありますか?良い説明はありますか?
  • 最初に変更の最も重要な部分を見てください。全体的にうまく設計されていますか?
  • 適切な順序で CL の残りの部分を見てください。

#ステップ 1:変更の概要を見る

CL の説明と、CL の一般的な動作を確認してください。この変更は理にかなっていますか?そもそもこの変更が発生してはならない場合は、変更が発生してはならない理由の説明をすぐに返信してください。このような変更を拒否する場合、開発者に代わりにすべきことを提案することも良い考えです。

たとえば、「これに何か良い仕事をしたように見えます、ありがとう!しかし、実際にここで変更している FooWidget システムを削除する方向に進んでいるので、作成したくない現在、新しい変更があります。代わりに、新しい BarWidget クラスをリファクタリングしてください。」

レビュアーが現在の CL を拒否し、別の提案を提供しただけでなく、丁寧にそれを行ったことに注意してください。この種の礼儀は重要です。同意しない場合でも、お互いを開発者として尊重することを示したいからです。

行いたくない変更を表す CL をいくつか取得した場合は、CL を作成する前にコミュニケーションができるように、チームの開発プロセスまたは外部貢献者向けのポストプロセスをやり直すことを検討する必要があります。大量の仕事をする前に、人々に「いいえ」と言ったほうが、今では捨てるか、大幅に書き直さなければなりません。

#ステップ 2:CL の主要部分を調べる

この CL の「メイン」部分であるファイルを見つけます。多くの場合、論理的な変更の数が最も多いファイルが 1 つあり、それが CL の主要な部分です。これらの主要な部分を最初に見てください。これにより、CL のすべての小さな部分にコンテキストが与えられ、一般的にコードレビューの実行が加速されます。 CL が大きすぎてどの部分が主要な部分であるかを判断できない場合は、開発者に最初に何を見るべきかを尋ねるか、CL を複数の CL に分割するよう依頼してください。

CL のこの部分でいくつかの大きな設計上の問題を見つけた場合は、CL の残りの部分を今すぐレビューする時間がなくても、それらのコメントをすぐに送信する必要があります。実際、設計上の問題が重大である場合、レビュー中の他のコードの多くは消えてしまい、いずれにせよ、CL の残りの部分をレビューすることは時間の浪費かもしれません。

これらの主要な設計コメントをすぐに送信することが非常に重要な 2 つの主な理由があります。

開発者はしばしば CL を郵送し、レビューを待っている間にその CL に基づいてすぐに新しい作業を開始します。検討している CL に重大な設計上の問題がある場合は、後の CL を作り直す必要があります。問題のあるデザインに加えて余分な作業を行う前に、それらをキャッチしたいのです。
主要な設計変更は、小さな変更よりも時間がかかります。開発者のほぼ全員に期限があります。これらの期限を設定し、コードベースに質の高いコードを保持するために、開発者はできるだけ早く CL の主要な再作業を開始する必要があります。

#ステップ 3:適切なシーケンスで CL の残りの部分を調べる

CL 全体に大きな設計上の問題がないことを確認したら、ファイルを確認するための論理的な順序を把握し、ファイルのレビューを見逃さないようにします。通常、主要なファイルを調べた後、コードレビューツールが表示する順序で各ファイルを調べるのが最も簡単です。メインコードを読む前に最初にテストを読むことも役立つ場合があります。これは、変更が何を行うべきかを把握しているためです。

#コードレビューの速度

#Why コードレビューの速度が大事なのか

  • チーム全体の速度が低下する可能性がある
  • 開発者はコードレビュープロセスに抗議する可能性がある
  • コードの状態が影響を受ける可能性がある

#コードレビューはどれくらい速くすべきですか?

焦点を絞ったタスクの途中にない場合は、入ってすぐにコードレビューを行う必要があります。

1 営業日は、コードレビューリクエストへの応答に要する最大時間です(つまり、翌朝に最初に行うこと)。

これらのガイドラインに従うことは、典型的な CL が 1 日以内に(必要に応じて)複数回のレビューを受ける必要があることを意味します。

#速度と中断

個人の速度を考慮することがチームの速度に勝る場合があります。コードの作成など、集中的なタスクの途中にいる場合は、コードのレビューを行うために自分自身を中断しないでください。開発者が中断された後、開発のスムーズな流れに戻るには長い時間がかかることが研究により示されています。したがって、コーディング中に自分自身を中断することは、別の開発者にコードのレビューを少し待たせるよりも、実際にはチームにとって費用がかかります。

代わりに、レビューのリクエストに応答する前に、作業のブレークポイントを待ってください。これは、現在のコーディングタスクが完了したとき、昼食の後、会議から戻ったとき、マイクロキッチンから戻ってきたときなどです。

#高速応答

コードレビューの速度について話すとき、CL がレビュー全体を通過して送信されるまでにかかる時間とは対照的に、関心のある応答時間です。プロセス全体も理想的には高速である必要がありますが、プロセス全体が迅速に行われるよりも、個々の応答が迅速に来ることがさらに重要です。

レビュープロセス全体を完了するのに時間がかかる場合もありますが、プロセス全体を通してレビュー担当者から迅速な応答があれば、開発者が「遅い」コードレビューで感じる不満を大幅に軽減できます。

忙しすぎて CL の完全なレビューを行うことができない場合でも、開発者に連絡するタイミングを知らせるクイックレスポンスを送信したり、より迅速に対応できる他のレビューアを提案したり、初期の幅広いコメントを提供します。 (注:このいずれも、このような応答を送信する場合でもコーディングを中断する必要があることを意味します。作業中の妥当なブレークポイントで応答を送信します。)

レビュー担当者は、「LGTM」が「このコードは当社の基準を満たしている」ことを意味することを確認するのに十分な時間をレビューに費やすことが重要です。ただし、個々の応答は理想的には高速である必要があります。

#クロスタイムゾーンのレビュー

時間帯の違いに対処する場合は、まだオフィスにいるときに著者に連絡を取ってください。すでに家に帰っている場合は、翌日にオフィスに戻る前に、レビューが完了していることを確認してください。

#コメント付き LGTM

コードレビューを高速化するために、CL に未解決のコメントを残している場合でも、レビューアーが LGTM /承認を与える必要がある特定の状況があります。これは、次のいずれかの場合に行われます。

レビュー担当者は、開発者がレビュー担当者の残りのコメントすべてに適切に対処すると確信しています。
残りの変更はマイナーであり、開発者が行う必要はありません。
レビューアは、明確でない場合、これらのオプションのどれを意図するかを指定する必要があります。

LGTM With Comments は、開発者とレビューアが異なるタイムゾーンにいる場合や、開発者が「LGTM、承認」を取得するために丸 1 日待つ場合に特に考慮する価値があります。

#大きな CL

誰かがあなたにそれをレビューする時間があるかわからないほど大きいコードレビューを送信する場合、あなたの典型的な応答は、CL を互いに構築するいくつかの小さな CL に分割するように開発者に依頼することです、一度にすべてのレビューが必要な 1 つの大きな CL の代わりに。これは通常、開発者の追加作業が必要な場合でも、レビュー担当者にとって非常に役立ちます。

CL をより小さな CL に分割できず、全体をすばやくレビューする時間がない場合は、少なくとも CL の全体的な設計に関するコメントを作成し、改善のために開発者に送り返します。レビュー担当者としての目標の 1 つは、開発者のブロックを常に解除するか、コードの健全性を犠牲にすることなく、何らかの追加アクションを迅速に実行できるようにすることです。

#長期にわたるコードレビューの改善

これらのガイドラインに従い、コードレビューに厳しい場合は、コードレビュープロセス全体が時間とともに速くなる傾向があることに気付くはずです。開発者は、健全なコードに必要なものを学習し、最初から優れた CL を送信し、必要なレビュー時間を短縮します。レビュー担当者は、レビュープロセスに不要な遅延を追加せずに、迅速に対応することを学びます。ただし、コードレビューの標準や速度を妥協しないでください。速度の向上を想像できます。実際には、長期的には、より迅速に何かを実行することはできません。

#緊急事態

また、CL がレビュープロセス全体を非常に迅速に通過する必要があり、品質ガイドラインが緩和される緊急事態もあります。ただし、緊急事態とはをご覧ください。実際に緊急事態とみなされる状況とそうではない状況の説明。

#コードレビューコメントの書き方

概要
親切にしてください。
あなたの推論を説明してください。
問題を指摘して開発者に決定させるだけで、明示的な指示を与えるバランスをとります。
開発者に、複雑さを単に説明するのではなく、コードを簡素化するか、コードコメントを追加するように促してください。

#礼儀

一般に、コードをレビューしている開発者にとって非常に明確で役立つと同時に、丁寧で敬意を払うことが重要です。これを行う 1 つの方法は、コードについて常にコメントを作成し、開発者についてはコメントしないようにすることです。常にこの慣行に従う必要はありませんが、動揺したり論争を呼んだりするかもしれないことを言うときは、必ずそれを使うべきです。例えば:

悪い点:「なぜ並行性から得られるメリットがないのに、ここでスレッドを使用したのですか?」

良い:「ここでの同時実行モデルは、実際のパフォーマンス上の利点なしにシステムに複雑さを追加しています。パフォーマンス上の利点がないため、このコードは複数のスレッドを使用するのではなくシングルスレッドにするのが最適です。」

#理由を説明

上記の「良い」例で気付くのは、コメントをする理由を開発者が理解するのに役立つことです。レビューのコメントにこの情報を常に含める必要はありませんが、意図、フォローしているベストプラクティス、または提案がコードの状態を改善する方法について、もう少し説明することが適切な場合があります。

#ガイダンスを与える

一般に、レビュー担当者ではなく CL を修正するのは開発者の責任です。ソリューションを詳細に設計したり、開発者向けのコードを書いたりする必要はありません。

ただし、これはレビュアーが役に立たないという意味ではありません。一般に、問題を指摘することと直接的なガイダンスを提供することの間で適切なバランスを取る必要があります。多くの場合、問題を指摘し、開発者に決定を任せると、開発者が学習しやすくなり、コードのレビューが容易になります。また、開発者はレビュー担当者よりもコードに近いため、より良いソリューションが得られます。

ただし、直接的な指示、提案、またはコードでさえもより役立つ場合があります。コードレビューの主な目標は、可能な限り最高の CL を取得することです。 2 番目の目標は、開発者のスキルを向上させ、時間の経過とともにレビューの必要性を減らすことです。

#説明を受け入れる

開発者に理解できないコードを説明してもらうと、通常はコードをより明確に書き直すことになります。過度に複雑なコードを説明するだけではない限り、コードにコメントを追加することも適切な応答です。

コードレビューツールでのみ記述された説明は、将来のコードリーダーには役立ちません。あまり知られていない領域をレビューしているときに、開発者がコードの通常の読者がすでに知っていることを説明するなど、いくつかの状況でのみ受け入れられます。

#コードレビューでのプッシュバックの処理

開発者がコードレビューを差し戻す場合があります。彼らはあなたの提案に反対するか、あなたが一般的に厳しすぎると文句を言うでしょう。

#誰が正しい

開発者があなたの提案に同意しない場合、まずそれらが正しいかどうかを検討してください。多くの場合、彼らはあなたよりもコードに近いため、特定の側面についてより良い洞察を持っているかもしれません。彼らの議論は理にかなっていますか?コードの健全性の観点から意味がありますか?もしそうなら、彼らが正しいことを彼らに知らせて、問題を落としてください。

ただし、開発者は常に正しいとは限りません。この場合、レビューアは、提案が正しいと考える理由をさらに説明する必要があります。適切な説明は、開発者の返信の理解と、変更が要求されている理由に関する追加情報の両方を示しています。

特に、レビュアーが提案によってコードの健全性が改善されると考えている場合、結果として生じるコード品質の改善が追加の要求された作業を正当化すると考える場合、変更を支持し続ける必要があります。コードの状態の改善は、小さなステップで行われる傾向があります。

時々、提案を説明するのに数ラウンドかかることがあります。ただ丁寧に滞在し、開発者に彼らが言っていることを聞いていることを知らせてください、あなたは同意しません。

#動揺する開発者

レビュー担当者は、レビュー担当者が改善を主張した場合、開発者が動揺すると信じることがあります。開発者が動揺することもありますが、通常は短時間であり、コードの品質を改善するのを手伝ってくれたことに感謝します。通常、あなたがあなたのコメントに丁寧であるならば、開発者は実際に全く動揺することはありません、そして心配はただレビュアーの心にあります。動揺は通常、レビュー担当者のコード品質に対する主張よりも、コメントの記述方法に関するものです。

#後できれいにする

プッシュバックの一般的な原因は、開発者が(当然のことながら)物事を成し遂げたいということです。この CL を取得するためだけに別のラウンドのレビューを行いたくないので、後の CL で何かをクリーンアップすると言うので、この CL を LGTM する必要があります。一部の開発者はこれについて非常に優れており、問題を修正するフォローアップ CL をすぐに作成します。ただし、経験上、開発者が元の CL を作成してから時間が経過するにつれて、このクリーンアップが行われる可能性は低くなります。実際、通常、開発者が現在の CL の直後にクリーンアップしない限り、クリーンアップは発生しません。これは、開発者が無責任であるためではなく、多くの作業が必要であり、クリーンアップが他の作業のプレスで失われたり忘れられたりするためです。したがって、通常は、コードがコードベース内にあり「完了」する前に、開発者が今すぐ CL をクリーンアップすることを主張するのが最善です。コードベースを退化させる一般的な方法は、人々に「後で物事をきれいにする」ことです。

CL が新しい複雑さを導入する場合、緊急でない限り、提出前にクリーンアップする必要があります。 CL が周囲の問題を明らかにし、それらに今すぐ対処できない場合、開発者はクリーンアップのバグを報告し、紛失しないように自分で割り当てる必要があります。オプションで、提出されたバグを参照するコードに TODO コメントを書き込むこともできます。

#厳格性に関する一般的な苦情

以前にかなり緩いコードレビューがあり、厳密なレビューに切り替えた場合、一部の開発者は非常に大声で文句を言うでしょう。通常、コードレビューの速度を向上させると、こうした苦情が消えてしまいます。

これらの苦情が消えるまでに数か月かかることもありますが、最終的に開発者は、優れたコードの生成を支援するために、厳密なコードレビューの価値を見出す傾向があります。騒々しい抗議者は、厳しくすることであなたが追加している価値を本当に見る何かが起こると、最強の支持者になることさえあります。

#競合の解決

上記のすべてを実行しても、解決できない開発者との間に競合が発生する場合は、競合の解決に役立つガイドラインと原則について、コードレビューの標準を参照してください。

#コードレビューを完了するための CL 作成者ガイド

#適切な CL 記述の作成

CL の説明は、どのような変更が行われているのか、なぜ変更されたのかを示す公的な記録です。これはバージョン管理履歴の永続的な一部となり、長年にわたってあなたのレビュー担当者以外の何百人もの人々に読まれることになるでしょう。

将来の開発者は、記述に基づいて CL を検索します。将来の誰かが、その関連性のかすかな記憶のために、あなたの変化を探しているかもしれませんが、詳細は手元にありません。すべての重要な情報が説明ではなくコード内にある場合、CL を見つけるのは非常に困難になります。

#最初の行

行われていることの短い要約。
まるで注文のように書かれた完全な文。
空行が続きます。
CL の説明の最初の行は、CL によって行われていることの具体的な短い要約であり、その後に空白行が続きます。これは、ほとんどのコード検索者がコードのバージョン管理履歴を閲覧しているときに表示されるものです。したがって、この最初の行は、一般的な情報を取得するためだけに CL あなたの CL が実際に何をしたかのアイデア。

伝統的に、CL 記述の最初の行は完全な文であり、あたかも順序(命令文)であるかのように書かれています。たとえば、「FizzBu​​zz RPC を削除して、新しいシステムに置き換えます」と言います。 「FizzBu​​zz RPC を削除して新しいシステムに置き換える」代わりに。ただし、説明の残りの部分を命令文として記述する必要はありません。

#ボディーは有益です

説明の残りの部分は参考になるはずです。解決されている問題の簡単な説明と、これが最良のアプローチである理由が含まれる場合があります。アプローチに欠点がある場合は、それらに言及する必要があります。関連する場合は、バグ番号、ベンチマーク結果、設計ドキュメントへのリンクなどの背景情報を含めます。

外部リソースへのリンクを含める場合は、保持ポリシーのアクセス制限により、将来の読者には表示されない可能性があることを考慮してください。可能な場合は、校閲者と将来の読者が CL を理解するのに十分なコンテキストを含めます。

小さな CL であっても、細部に少し注意を払うに値します。 CL をコンテキストに配置します。

#悪い CL の説明

「バグの修正」は、CL の説明が不十分です。どんなバグ?あなたはそれを修正するために何をしましたか?他の同様に悪い説明は次のとおりです。

  • 「ビルドを修正」
  • 「パッチを追加」
  • 「A から B へのコードの移動」
  • 「フェーズ 1」
  • 「便利な機能を追加」
  • 「奇妙な URL を消す」

それらのいくつかは、実際の CL 記述です。著者は、有用な情報を提供していると信じているかもしれませんが、CL 記述の目的を果たしていません。

#良い CL の説明

良い説明の例をいくつか示します。

機能変更
例:

rpc:RPC サーバーメッセージの空きリストのサイズ制限を削除しました。

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

最初の数語は、CL が実際に行うことを説明しています。残りの説明では、解決される問題、これが良い解決策である理由、および特定の実装に関するもう少しの情報について説明します。

#リファクタリング

例:

TimeStreep および Now メソッドを使用するには、TimeKeeper でタスクを作成します。

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

タスクに Now を提供できるようにすることは、Borglet への依存を解消するためのステップです。最終的に、タスクから Now を取得することに依存しているコラボレーターは、TimeKeeper を直接使用するように変更する必要がありますが、これは小さな手順でリファクタリングするための手段です。

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

最初の行は、CL の機能と、これが過去からの変化である方法を説明しています。残りの説明では、具体的な実装、CL のコンテキスト、ソリューションが理想的ではないこと、および将来の方向性について説明します。また、この変更が行われている理由についても説明します。

#コンテキストが必要な小さな CL

例:

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

これにより、Python3 のように既にこれを使用しているコンシューマーは、独自のツリーのどこかではなく、元のステータスビルドルールの隣にあるルールに依存できます。 Python2 ではなく Python3 を使用できるようになった場合、Python3 を使用することを新しい消費者に奨励し、現在作業中の一部の自動ビルドファイルリファクタリングツールを大幅に簡素化します。

最初の文は、実際に行われていることを説明しています。説明の残りの部分では、変更が行われている理由を説明し、レビュー担当者に多くのコンテキストを提供します。

#CL を送信する前に説明を確認してください

CL はレビュー中に大幅に変更される可能性があります。 CL を送信する前に CL の説明を確認して、説明がまだ CL の動作を反映していることを確認することは価値があります。

#小さな CL

#小さな CL を書く理由

小さくシンプルな CL は次のとおりです。

より迅速にレビュー。 1 つの大きな CL をレビューするために 30 分のブロックを確保するよりも、小さな CL をレビューするために、レビュー担当者が 5 分間を数回見つける方が簡単です。
より徹底的に見直しました。大規模な変更を行うと、レビュアーと著者は、重要なポイントを見逃したり、削除したりするポイントまで、前後に移動する大量の詳細なコメントにイライラする傾向があります。
バグを導入する可能性が低い。変更する回数が少ないので、あなたとあなたのレビュー担当者は CL の影響について効果的に推論し、バグが導入されているかどうかを確認するのが簡単です。
拒否された場合の無駄な作業が減ります。巨大な CL を作成した後、レビュアーが全体的な方向が間違っていると言った場合、多くの作業を無駄にしています。
マージが簡単。大きな CL での作業には時間がかかるため、マージするときに多くの競合が発生し、頻繁にマージする必要があります。
設計が簡単です。大規模な変更のすべての詳細を改良するよりも、小さな変更の設計とコードの健全性を磨く方がはるかに簡単です。
レビューのブロックが少ない。変更全体の自己完結型部分を送信すると、現在の CL のレビューを待つ間、コーディングを続行できます。
ロールバックが簡単です。大きな CL は、最初の CL サブミッションとロールバック CL の間に更新されるファイルに触れる可能性が高く、ロールバックを複雑にします(おそらく、中間 CL もロールバックする必要があります)。
レビュー担当者には、変更が大きすぎるという唯一の理由で変更を完全に拒否する裁量があることに注意してください。通常、彼らはあなたの貢献に感謝しますが、どうにかしてそれを一連の小さな変更にすることを要求します。変更をすでに書いた後に分割するのは大変な作業になる場合があります。また、レビュー担当者が大きな変更を受け入れる理由について議論するのに多くの時間を必要とします。そもそも小さな CL を書く方が簡単です。

#小とは

一般に、CL の適切なサイズは自己完結型の変更です。この意味は:

CL は、たった 1 つのことに対処する最小限の変更を行います。これは通常、一度に機能全体ではなく、機能の一部にすぎません。一般に、小さすぎる CL と大きすぎる CL を書く側でエラーを起こす方が適切です。レビュアーと協力して、許容可能なサイズを確認します。
レビューアが CL について理解する必要があるすべて(将来の開発を除く)は、CL、CL の説明、既存のコードベース、または既にレビューした CL にあります。
CL がチェックインされた後も、システムはユーザーおよび開発者にとって引き続き適切に機能します。
CL はそれほど小さくないため、その意味を理解するのは困難です。新しい API を追加する場合は、レビュー担当者が API の使用方法をよりよく理解できるように、同じ CL に API の使用法を含める必要があります。これにより、未使用の API のチェックインも防止されます。
「大きすぎる」ことについての厳格なルールはありません。通常、CL には 100 行が妥当なサイズであり、1000 行は通常は大きすぎますが、レビュー担当者の判断次第です。変更が分散されるファイルの数も、その「サイズ」に影響します。 1 つのファイルの 200 行の変更は問題ないかもしれませんが、通常は 50 個のファイルにまたがって大きすぎます。

あなたがコードを書き始めたときからあなたはあなたのコードに深く関わっていますが、レビュアーはしばしば文脈を持たないことを覚えておいてください。容認できるサイズの CL のように見えるものは、レビュアーに圧倒されるかもしれません。疑わしい場合は、書く必要があると思うよりも小さい CL を書きます。レビュー担当者が、小さすぎる CL の取得について苦情を言うことはめったにありません。

#大規模な CL はいつ大丈夫ですか?

大きな変更がそれほど悪くない状況がいくつかあります。

通常、ファイル全体の削除は 1 行の変更としてカウントできます。これは、レビュー担当者がレビューするのにそれほど時間がかからないためです。
完全に信頼できる自動リファクタリングツールによって大きな CL が生成される場合があります。レビュアーの仕事は、健全性チェックだけで、変更を本当に必要としていると言うことです。これらの CL はより大きくなる可能性がありますが、上記のいくつかの注意事項(マージやテストなど)が引き続き適用されます。

#ファイルによる分割

CL を分割する別の方法は、異なるレビュー担当者を必要とするが、それ以外の場合は自己完結型の変更であるファイルのグループ化です。

たとえば、プロトコルバッファの変更のために 1 つの CL を送信し、そのプロトを使用するコードの変更のために別の CL を送信します。コード CL の前にプロト CL を提出する必要がありますが、両方を同時にレビューできます。これを行う場合、両方のレビュアーのセットに、作成した他の CL について通知し、変更のコンテキストがあるようにすることができます。

別の例:コード変更用に 1 つの CL を送信し、そのコードを使用する構成または実験用に別の CL を送信します。これは、必要に応じてロールバックするのも簡単です。構成/実験ファイルはコードの変更よりも速く本番環境にプッシュされることがあるためです。

#リファクタリングの分離

通常、機能の変更やバグ修正とは別の CL でリファクタリングを行うのが最善です。たとえば、クラスの移動と名前の変更は、そのクラスのバグの修正とは異なる CL で行う必要があります。レビュー担当者は、各 CL が別々の場合に導入される変更を理解する方がはるかに簡単です。

ただし、ローカル変数名の修正などの小さなクリーンアップは、機能変更またはバグ修正 CL の内部に含めることができます。リファクタリングが非常に大きいため、現在の CL に含めるとレビューが難しくなる時期を判断するのは、開発者とレビュー担当者の判断次第です。

#関連するテストコードを同じ CL に保持する

テストコードを別の CL に分割しないでください。コードの変更を検証するテストは、コード行数が増えた場合でも、同じ CL で行われる必要があります。

ただし、リファクタリングガイドラインと同様に、独立したテスト変更を最初に個別の CL に入れることができます。以下が含まれます。

既存の送信済みコードを新しいテストで検証します。
テストコードのリファクタリング(ヘルパー関数の導入など)。
より大きなテストフレームワークコードの導入(例:統合テスト)。

#ビルドを壊さない

相互に依存する複数の CL がある場合、各 CL が送信された後にシステム全体が機能し続けることを確認する方法を見つける必要があります。そうしないと、CL の提出の間に数分間、すべての仲間の開発者のビルドを中断する可能性があります(または、後の CL の提出で予期しない問題が発生した場合はさらに長くなります)。

#十分に小さくすることはできない

CL を大きくする必要があるような状況に遭遇する場合があります。これは非常にまれです。小さな CL の作成を実践する著者は、ほとんどの場合、機能を一連の小さな変更に分解する方法を見つけることができます。

大きな CL を作成する前に、リファクタリング専用の CL をその CL の前に付けて、よりクリーンな実装への道を開くことができるかどうかを検討してください。チームメイトと話し、代わりに小さな CL に機能を実装する方法について考えている人がいるかどうかを確認します。

これらのオプションがすべて失敗した場合(非常にまれです)、事前にレビュー担当者から同意を得て大規模な CL をレビューするため、今後の内容について警告されます。このような状況では、レビュープロセスを長期間経験し、バグを導入しないように注意し、テストの作成に特に注意を払ってください。
通常は、物事を行うのが最善です

WANT_LGTM は、CL が複数のレビュアーに送信されるときの期待を明確にします。 WANT_LGTM = any(デフォルトの動作)または WANT_LGTM = all を 使用して明確にすることができます。

#レビュー担当者のコメントを処理する方法

レビューのために CL を送信した場合、レビュー担当者が CL にいくつかのコメントを返信する可能性があります。レビュー担当者のコメントの処理について知っておくと便利な情報を次に示します。

#個人的に受け取らないでください

レビューの目的は、コードベースと製品の品質を維持することです。レビュアーがコードの批評を提供するとき、それはあなたやあなたの能力に対する個人的な攻撃としてではなく、あなた、コードベース、そしてグーグルを助ける試みとして考えてください。

時々、レビュアーはフラストレーションを感じ、コメントでそのフラストレーションを表明します。これはレビュアーにとって良い習慣ではありませんが、開発者としてこれに備える必要があります。 「レビュアーが私に伝えようとしている建設的なことは何ですか?」そして、それが彼らが実際に言ったことのように動作します。

コードレビューのコメントに怒りで応答しないでください。それはプロのエチケットの重大な違反であり、コードレビューツールで永遠に生き続けます。怒りやイライラがあり、親切に対応できない場合は、しばらくコンピューターから離れるか、丁寧に返事できるほど落ち着くまで他の作業を行います。

一般に、レビュアーが建設的で丁寧な方法でフィードバックを提供していない場合は、直接レビューアに説明してください。直接会ったりビデオハングアウトしたりできない場合は、プライベートメールを送信してください。好きではないことと、違うやり方でやりたいことを親切に説明してください。彼らがこの非公開の議論にも非建設的な方法で応答する場合、または意図した効果がない場合は、必要に応じて上司にエスカレーションします。

#コードを修正する

コード内の何かを理解していないと校閲者が言った場合、最初の対応はコード自体を明確にすることです。コードを明確にできない場合は、コードが存在する理由を説明するコードコメントを追加します。コメントに意味がないと思われる場合は、コードレビューツールでの回答を説明してください。

レビュー担当者がコードの一部を理解していなかった場合、将来コードを読んでいる他の読者も理解できない可能性があります。コードレビューツールで応答を作成しても、将来のコードリーダーには役立ちませんが、コードの明確化やコードコメントの追加は役立ちます。

#自分のためだと思います

CL の作成には多くの作業が必要になる場合があります。レビューのために最終的に 1 つを送信し、完了したように感じ、さらに作業が必要ないことをかなり確信することは、しばしば本当に満足です。そのため、レビュー担当者が改善できる点についてコメントを返した場合、コメントが間違っていると反省しやすく、レビュアーが不必要にあなたをブロックしているか、CL を送信するだけで済むはずです。ただし、この時点でどれだけ確実であるかに関係なく、少し時間をおいて、レビュー担当者がコードベースと Google に役立つ貴重なフィードバックを提供しているかどうかを検討してください。自分への最初の質問は、常に「レビュアーは正しいですか?」でなければなりません。

その質問に答えられない場合は、レビュー担当者がコメントを明確にする必要がある可能性があります。

あなたがそれを考慮してもまだ正しいと思うなら、あなたの物事のやり方がコードベース、ユーザー、そして/または Google にとってより良い理由の説明で気軽に答えてください。多くの場合、レビュアーは実際に提案を提供しており、彼らはあなたに自分が何が最善かを考えてほしいと思っています。ユーザー、コードベース、または CL について、レビュー担当者が知らないことを実際に知っているかもしれません。記入してください。より多くのコンテキストを与えます。通常、技術的な事実に基づいて、自分とレビュアーの間でコンセンサスを得ることができます。

競合の解決

競合を解決するための最初のステップは、常にレビュアーとコンセンサスを得ることです。コンセンサスが得られない場合は、このような状況で従うべき原則を提供するコードレビューの基準を参照してください。

#用語

これらのドキュメントの一部で使用されている Google 内部の用語がいくつかあります。
外部読者のためにここで明確にします:

  • CL:「チェンジリスト」の略。
         バージョン管理に提出されているか、コードレビューを受けています。
         他の組織では、これを「変更」または「パッチ」と呼ぶことがあります。
  • LGTM:「Looks Good to Me」を意味します。コードレビュアーが言うのは
        CL を承認します。

#ライセンス

このプロジェクトのドキュメントは、CC-By 3.0 ライセンスの下でライセンスされています。
これらのドキュメントを共有することをお勧めします。見る
詳細については、https://creativecommons.org/licenses/by/3.0/
(opens new window)

2019-12-06
  • share