コードレビューの観点

（注）以下のポイントを検討する際にはつねにコードレビューの基準を忘れないでください。

設計

レビューで確認すべき最も大切なことは、CL の全体的な設計です。 CL のコードの各部分は相互にきちんと連携するでしょうか？この変更はコードベースに属するものでしょうか、それともあるライブラリに属するものでしょうか？システムの他の部分とうまく統合するでしょうか？この機能を追加するタイミングは今がふさわしいでしょうか？

機能性

この CL は開発者の意図通りに動作しますか？開発者の意図はこのコードのユーザーにとって適切でしょうか？「ユーザー」とは普通、エンドユーザー（その変更によって影響を受ける場合）と開発者（将来このコードを「使う」必要のある人）の両方を指します。

通常、CL がコードレビューに至るまでには、コードが正しく動作することを開発者が十分にテストしていると期待できます。 それでもレビュアーはエッジケースを想定する、並行処理の問題を探す、ユーザーになりきって考えるなど、コードを読むだけではわからない不具合がないかを確認してください。

必要に応じて CL を検証しても構いません。特に、UI の変更といった、ユーザーに影響する変更があるときには、レビュアーが CL の動作を確認するべき最も重要な機会です。 変更がユーザーにどのような影響を与えるかはコードを読むだけではわかりにくいものです。 そのような変更に対して、CL の変更を反映して自分で動作確認するのが難しければ、開発者にその機能のデモを依頼することもできます。

また別のケースですが、コードレビュー中に機能について熟慮が特に求められるのは、CL にある種の並行プログラミングが行われていて、理論的にデッドロックや競合状態を引き起こす可能性がある場合です。 こういう問題はコードを実行するだけでは不具合の発見が難しいため、通常、コード全体を見て問題が発生していないことを注意深く確認する人（開発者とレビュアーの両方）が必要です。 （なお、これこそがやはり、競合状態やデッドロックが発生しうるところで並行処理モデルを採用すべきでない十分な理由です。コードレビューを行うにしてもコードを理解するにしても非常に複雑になりうるからです。）

複雑性

CL が必要以上に複雑になっていないでしょうか？ CL のあらゆるレベルで確認してください。 一行一行は複雑すぎないでしょうか？関数は複雑すぎないでしょうか？クラスは複雑すぎないでしょうか？ 「複雑すぎる」とは普通、「コードを読んですぐに理解できない」という意味です。 あるいは、「開発者がこのコードを呼び出したり修正したりしようとするときに不具合を生み出す可能性がある」という意味でもあります。

あるタイプの複雑性は、オーバーエンジニアリングです。開発者が必要以上にコードを一般化していたり、現在のシステムにとってまだ必要のない機能を盛り込んでいたりするということです。 レビュアーはオーバーエンジニアリングを特に警戒すべきです。 開発者には、現在解決する必要のある既知の問題に取り組むべきであって、将来解決する必要が出てくるかもしれない推測に基づいた問題には目を向けないよう勧めてください。 将来の問題はそれが発生してから取り組めばよいのです。問題が発生すれば、この物理的な宇宙の中でその実際の形と要件を知ることができます。

テスト

変更に適したユニットテスト、結合テスト、E2E テストを依頼してください。 一般に、テストはプロダクションコードと同じ CL に追加してください。 例外は、CL が緊急事態に対処している場合です。

CL の中のテストが正確で、適切で、有用であることを確認してください。 テストがテストコード自体をテストすることはありませんし、テストのためのテストコードを書くこともめったにありません。テストの有効性は人間が確認しなければなりません。

コードが壊れているときにテストはきちんと失敗するでしょうか？ そのテストの下でコードを変更すると、テストが誤検知を起こさないでしょうか？ 各テストはシンプルで有用なアサーションを使っているでしょうか？ テストは異なるテストメソッドごとに適切に分割されているでしょうか？

テストもまた保守すべきコードであることを覚えていてください。 メインのバイナリに含まれないからといって、テストが複雑になるのを許容しないでください。

命名

開発者はあらゆるものに適切な名前を与えているでしょうか？ 適切な名前とは、それが何であるか／何をするかを伝えるのに十分に長く、しかし読むのに困難を覚えないほど短いものです。

コメント

開発者はわかりやすい英語で明確なコメントを書きましたか？ すべてのコメントは実際に必要でしょうか？ コメントは普通、あるコードが「なぜ」存在するのかを説明するのに役立ちますが、コードが「何」をしているのかを説明すべきではありません。 コードがそれ自身を説明するほど明確でないのなら、コードをもっとシンプルにすべきです。 例外はあります（たとえば正規表現や複雑なアルゴリズムでは何をしているのかを説明するコメントは非常に有益です）が、ほとんどの場合、コメントは決定の背後にある理由といった、コード自体が語ることのできない情報を伝えるために書きます。

CL の前にその箇所にあったコメントに注目するのが有益であることもあります。 今となっては削除すべき TODO があるかもしれませんし、この変更を行うべきではないと助言するコメントなどがあるかもしれません。

なお、コメントはクラス、モジュール、関数のドキュメンテーションとは違います。ドキュメンテーションコメントはコードの目的や、使い方や、使われたときのふるまいを記述するものです。

スタイル

Google にはスタイルガイドがあります。メジャーな言語に関してはすべて、マイナーな言語でも多くはスタイルガイドが揃っています。 CL が適切なスタイルガイドを従っているかを確認してください。

スタイルガイドに記載のないスタイルの改善をしたい場合、コメントに「Nit:」というプレフィックスを付けて、それが細かい指摘 (nitpick) であることを開発者に知らせるのがよいでしょう。そうすると、コードを修正してほしいが強制ではないということが伝わります。 個人的なスタイルの好みで CL の提出をブロックしないでください。

CL の作成者はスタイル上の大きな変更を他の変更に混ぜないようにしてください。 それをすると CL で何が変更されているのかを見るのが難しくなるばかりか、マージ後にロールバックするのはもっと大変ですし、さらに他の問題も引き起こします。 たとえば、作成者がファイル全体を再フォーマットしたいと思ったら、再フォーマットだけを一つの CL として提出し、その後で機能的な変更を別の CL として提出するようにしてください。

ドキュメンテーション

CL がコードのビルド、テスト、相互連携、リリースのやり方を変更する場合、それに関連するドキュメンテーションも更新しているかを確認してください。 関連するドキュメンテーションには、README、g3doc ページ、自動生成されたリファレンスドキュメントなどがあります。 CL がコードを削除あるいは非推奨にしたら、ドキュメンテーションも削除するべきかどうか検討してください。 ドキュメンテーションが存在しなければ、作成するように依頼してください。

一行ずつ

レビューをアサインされたらコードを一行ずつ見てください。 データファイル、自動生成されたコード、巨大なデータ構造などはざっと見れば済むこともありますが、人間の書いたクラス、関数、コードブロックなどはそうもいきません。 コードの中に書かれているものが正しいと決めてかからず、じっくり読んでください。 明らかに他のコードよりも精密に調べるに値するコードもあります—そうすべきかどうかはレビュアーが自分で判断しなければなりません—が、少なくとも全コードが何をしているかを確実に理解するようにしてください。

コードが読みにくく、そのことがレビューを遅らせているなら、レビューをいったん置いて開発者に知らせ、コードを明確にしてくれるのを待ちましょう。 Google には優秀なソフトウェアエンジニアが雇われていますし、あなたはその一人です。 あなたがコードを理解できないのなら、他の開発者もきっと理解できません。 ですから、開発者にコードを明確にするよう依頼するのは、未来の開発者のためにこのコードを理解しやすくする手助けでもあります。

コードを理解できてもレビューのある部分で自分にはレビューする資格がないと感じる場合、その CL について他に適切なレビュアーがいることを忘れないでください。 特に、セキュリティ、並行処理、アクセシビリティ、インターナショナライゼーションといった複雑な問題に関しては適任者がいます。

コンテキスト

CL を広いコンテキストの中に置いて眺めると有意義なことがよくあります。 普通コードレビューツールは変更のあった箇所の周りを数行ほど表示するだけです。 変更がうまく機能することを確認するためにファイル全体を見なければならないときもあります。 たとえば、追加された行が 4 行だけだったとしても、ファイル全体を眺めたらそれが 50 行に及ぶメソッドの中に書かれていることがわかり、メソッドを分割する必要があると判明することもあります。

CL をシステム全体のコンテキストの中に置いて考えてみることも有益です。 この CL はシステムのコードの健康状態を改善しているでしょうか、それともシステム全体を複雑にし、テスト不足な状態にしているでしょうか？ コードの健康状態を悪化させる CL を受け入れないでください。 ほとんどのシステムは小さな変更が積み重なってだんだんと複雑化します。 だからこそ、新たな変更があったときに小さな複雑性でも混入させないようにするのが大切です。

良いこと

CL の中に素晴らしいものを見つけたら、開発者に教えてあげてください。特に、あなたのレビューコメントの一つに取り組んで素晴らしくやり遂げたらそうしてください。 コードレビューは間違いにばかり目が行きがちですが、良い実践に対しての励ましや感謝の言葉も伝えるべきです。 メンタリングの観点では、開発者が正しいこと行ったときにそれを伝えるほうが、間違いを指摘するよりもずっと価値がある場合があります。

要約

コードレビューをする際には、次のことを確認してください。

• コードがうまく設計されている
• 機能性がコードのユーザーにとって適切である
• UI の変更がある場合、よく考えられていて見た目も適切である
• 並行処理がある場合、安全に行われている
• コードが必要以上に複雑でない
• 開発者は将来必要になるかもしれないものではなく、現在必要だとわかっているものを実装している
• コードには適切なユニットテストがある
• テストがうまく設計されている
• 開発者はあらゆるものに明確な名前を使った
• コメントは明確で有意義なもので、「何」ではなく「なぜ」を説明している
• コードは適切にドキュメント化されている（一般的には g3doc で）
• コードはスタイルガイドに準拠している

レビューを依頼されたコードを一行ずつレビューすること、コンテキストを確認すること、コードの健康状態を改善しているかを見極めること、開発者が良いことをしたらそれを褒めることを忘れずに。

次: レビューで CL を閲覧する