コードレビューで注目すべきポイント
注意：これらのポイントを考慮する際には、常にコードレビューの標準を参考にしてください。
デザイン
レビューで最も重要なのは、CLの全体的なデザインです。 CL内のさまざまなコードの相互作用は意味をなしていますか？この変更は、コードベースではなくライブラリに属していますか？システム全体との統合はうまくいっていますか？この機能を追加するのに適切な時期ですか？
機能性
このCLは、開発者が意図した通りの動作をしていますか？開発者が意図したことは、このコードのユーザーにとって良いものですか？「ユーザー」とは、通常、エンドユーザー（変更の影響を受ける場合）と開発者（将来このコードを「使用」する必要がある）の両方を指します。
ほとんどの場合、開発者はCLを十分にテストして、コードレビューに到達するまでに正しく動作するようにすることが期待されます。ただし、リビューアとしては、エッジケースを考え、並行性の問題を探し、ユーザーのように考え、コードを読むだけで見えるバグがないか確認する必要があります。
CLの動作を検証することもできますが、リビューアがCLの動作をチェックするのは、UIの変更など、ユーザーに影響を与える変更の場合が最も重要です。コードを読むだけでは、どのような変更がユーザーにどのような影響を与えるかを理解するのは難しいです。そのような変更の場合、開発者にデモをしてもらうか、CLにパッチを当てて試すことができない場合は、機能のデモをしてもらうことができます。
また、コードレビュー中に特に機能性について考える必要があるのは、CLに理論的にデッドロックや競合状態を引き起こす可能性のある並行プログラミングが行われている場合です。これらの問題は、コードを実行するだけでは検出するのが非常に難しく、問題が導入されていないことを確認するために、開発者とリビューアの両方が注意深く考える必要があります。（競合状態やデッドロックが発生する可能性のある並行性モデルを使用しないことは、コードレビューやコードの理解を非常に複雑にする可能性があるため、非常に重要な理由です。）
複雑さ
CLは必要以上に複雑ですか？CLの各レベルで確認してください。個々の行は複雑すぎますか？関数は複雑すぎますか？クラスは複雑すぎますか？「複雑すぎる」とは通常、「コード読み手がすぐに理解できない」という意味です。また、「開発者がこのコードを呼び出したり変更しようとすると、バグを発生させる可能性が高い」という意味もあります。
特定のタイプの複雑さは「過剰設計」です。開発者がコードを必要以上に一般化したり、現在のシステムで必要のない機能を追加したりしている場合です。リビューアは特に過剰設計に対して警戒すべきです。開発者に対して、現在解決する必要のある問題を解決するように促し、将来解決するかもしれない問題ではなく、現在の問題を解決するようにしましょう。将来の問題は、実際の形状と要件を物理的な宇宙で確認できるようになった時に解決すべきです。
テスト
変更に適した単体テスト、統合テスト、エンドツーエンドテストを要求してください。一般的には、テストは本番コードと同じCLに追加する必要がありますが、CLが緊急事態を処理している場合を除きます。
CL内のテストが正確で、意味のある、有用なものであることを確認してください。テストは自己テストを行いませんし、テストのためにテストを書くことはほとんどありません。人間がテストが有効であることを確認する必要があります。
コードが壊れたときにテストは実際に失敗しますか？コードが変更された場合、テストは誤った結果を出力し始めますか？各テストは単純で有用なアサーションを行っていますか？テストは異なるテストメソッド間で適切に分離されていますか？
テストもメンテナンスが必要なコードですので、メインのバイナリの一部ではないためにテストに複雑さを受け入れないでください。
名前付け
開発者はすべての要素に適切な名前を選びましたか？適切な名前は、アイテムの内容や機能を完全に伝えるのに十分な長さであり、読みにくくなるほど長くなっていませんか？
コメント
開発者は理解しやすい英語で明確なコメントを書いていますか？すべてのコメントが実際に必要ですか？通常、コメントはコードが存在する理由を説明するために使用され、コードが何をしているかを説明するためには使用されません。コード自体が十分に明確でない場合は、コードをよりシンプルにする必要があります。ただし、正規表現や複雑なアルゴリズムなどの場合は、自身のコードに含まれていない情報を説明するコメントが非常に役立つことがあります。ただし、ほとんどの場合、コメントは意思決定の背後にある理由など、コード自体に含まれない情報のために使用されます。
また、この変更リストの前にあったコメントも参考になる場合があります。今は削除できるTODOコメントや、この変更を行わないように忠告するコメントなどがあるかもしれません。
コメントは、クラス、モジュール、または関数のドキュメントとは異なり、コードの目的、使用方法、使用時の動作などを表現するべきです。
スタイル
Googleでは、主要な言語だけでなく、ほとんどのマイナーな言語に対してもスタイルガイドがあります。適切なスタイルガイドに従ってCLを作成してください。
スタイルガイドにないスタイルの改善をしたい場合は、「Nit:」という接頭辞をコメントに付けて、開発者にそれがコードを改善するための細かい指摘であることを伝えてください。個人的なスタイルの好みだけでCLの提出をブロックしないでください。
CLの作者は、主要なスタイルの変更を他の変更と組み合わせてはいけません。CLで何が変更されているかがわかりにくくなり、マージやロールバックが複雑になり、他の問題を引き起こします。たとえば、ファイル全体の書式を変更したい場合は、まず書式変更のみを含んだ別のCLを送り、その後に機能的な変更を含んだ別のCLを送ってください。
一貫性
既存のコードがスタイルガイドと一貫していない場合はどうなりますか？コードレビューの原則によれば、スタイルガイドが絶対的な権威です。つまり、スタイルガイドで要求されているものは、CL（変更リスト）はガイドラインに従うべきです。
一部の場合、スタイルガイドは要件ではなく、推奨事項を示しています。これらの場合、新しいコードが推奨事項に従うべきか、周囲のコードに一貫性を持たせるべきかは判断に委ねられます。ローカルな一貫性が混乱を招く場合を除き、スタイルガイドに従うことを優先しましょう。
他のルールが適用されない場合は、著者は既存のコードと一貫性を保つべきです。
いずれの場合でも、著者に対してバグの報告と既存のコードの整理のためのTODOの追加を促してください。
ドキュメンテーション
もしCLがユーザーがコードをビルド、テスト、操作、リリースする方法に変更を加える場合、関連するドキュメンテーション（README、g3docページ、生成されたリファレンスドキュメントなど）も更新されているか確認してください。もしCLがコードを削除または非推奨化する場合、ドキュメンテーションも削除すべきかどうか考えてください。 もしドキュメンテーションが不足している場合は、それを要求してください。
すべての行
一般的な場合、レビューすることに割り当てられた_すべて_のコードの行を見てください。データファイルや生成されたコード、大きなデータ構造などは、時々スキャンすることができますが、人間が書いたクラス、関数、またはコードブロックをスキャンして、その中身が問題ないと仮定しないでください。 明らかに、あるコードは他のコードよりも注意深く調査する価値がありますが、それはあなたが判断しなければならないことです。少なくとも、すべてのコードが何をしているのかを_理解_していることを確認してください。
もしコードを読むのが難しく、それがレビューを遅らせている場合は、開発者にそれを伝えて、レビューする前にそれを明確にしてもらうように待ってください。Googleでは、優れたソフトウェアエンジニアを採用しています。あなたもその一人です。もしコードが理解できない場合、他の開発者も理解できない可能性が非常に高いです。ですので、開発者にそれを明確にしてもらうことで、将来の開発者がこのコードを理解するのを助けることにもなります。
もしコードが理解できるけれども、レビューの一部を適任と感じない場合は、レビュアーがいることを確認してください。特にプライバシーやセキュリティ、並行性、アクセシビリティ、国際化などの複雑な問題については、適任なリビューアがいることを確認してください。
例外
もし、すべての行をレビューすることが意味をなさない場合はどうなりますか？ 例えば、あなたがCLの複数のレビュワーのうちの一人であり、次のような要求がある場合です。
- 大きな変更の一部である特定のファイルのみをレビューするように求められる場合。
- 高レベルの設計、プライバシーやセキュリティの影響など、CLの特定の側面のみをレビューするように求められる場合。
これらの場合、レビューした部分をコメントで明記してください。コメント付きのLGTMを優先してください。
もし、他のレビュワーがCLの他の部分をレビューしたことを確認した後にLGTMを与えたい場合は、それを明示的にコメントで記述して、期待値を設定してください。CLが望ましい状態に達したら、迅速に応答することを目指してください。
文脈
CLを広い文脈で見ることは、しばしば役立ちます。通常、コードレビューツールは、変更される部分の周りの数行のコードしか表示しません。実際に変更が意味をなしているかどうかを確認するには、ファイル全体を見る必要があることもあります。たとえば、追加されるのはわずか4行のコードだけかもしれませんが、ファイル全体を見ると、その4行が50行のメソッドにあることがわかり、このメソッドをより小さなメソッドに分割する必要があることがわかります。
また、CLをシステム全体の文脈で考えることも有用です。このCLはシステムのコードの健全性を向上させているのか、システム全体をより複雑にしたり、テストが少なくなったりしているのか、などを考えると良いでしょう。**システムのコードの健全性を損なうCLは受け入れないでください。**ほとんどのシステムは、多くの小さな変更を通じて複雑になっていくため、新しい変更においても小さな複雑さを防ぐことが重要です。
良いこと
もしCLで素敵なものを見つけたら、開発者に伝えてください。特に、彼らがあなたのコメントに素晴らしい方法で対応した場合は。コードレビューはしばしばミスに焦点を当てますが、良い実践に対する励ましと感謝も提供すべきです。開発者に何が間違っているかを伝えるよりも、彼らが何を正しく行ったかを伝える方が、メンタリングの観点からはさらに価値があります。
概要
コードレビューを行う際には、以下の点に注意してください：
- コードがよく設計されていること。
- 機能がコードのユーザーにとって良いものであること。
- UIの変更が合理的で見栄えが良いこと。
- 並行プログラミングが安全に行われていること。
- コードが必要以上に複雑でないこと。
- 開発者が将来必要になるかもしれないものを実装していないこと。
- コードに適切な単体テストがあること。
- テストがよく設計されていること。
- 開発者がすべての要素にわかりやすい名前を付けていること。
- コメントが明確で有用であり、主に「なぜ」を説明していること。
- コードが適切にドキュメント化されていること（一般的にはg3docで）。
- コードがスタイルガイドに準拠していること。
レビューを依頼されたコードのすべての行を確認し、文脈を考慮し、コードの健全性を向上させ、開発者が行った良い点を褒めるようにしてください。