目次

はじめに

Google’s Engineering Practices documentation にて公開されている レビュー方法 を読んでみました。
こちら：How to do a code review

これを読んでどんなことが書いてあるのかをサッとまとめるとともに、自分の考えも交えながら整理してみます。
※基本的に ※印 の箇所が自分の考えとかだったりします。

また、本書ではCLという単語が頻繁に出現しますが、Create a change list のことです。
参考：CL

標準的なコードレビュー

コードレビューの目的は、コードレビューを通してコードの健全性が向上することです。
コードの健全性については、こちらを参照して下さい。
参考：Code Health

ただし、コードレビューの実践にはいくつかのトレードオフが存在します。

• 開発者がコミットしない限りはコードは良くならないが、レビュワーが厳しすぎると開発者は意欲を失う
• 一方で、レビュワーはその変更がコードの品質が向上するものであることをしっかりと確かめる必要がある

一般的に、プロジェクトが進むにつれてコードの健全性は低下していく傾向にあります。
レビュワーは、コードの一貫性やメンテナンス性やその他、
What to look for a code review で見るべき観点に対して責任を持たなければなりません。
そうすると、どこまで（どのような塩梅で）レビューするかといったことが問題となってきます。

そこで、これらのトレードオフを踏まえて 下記のようなルールをGoogle では定めています。

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.

引用：The Standard of Code Review

つまり、CLが完璧でなくともコードの健全性が向上していればOK という基本的なガイドラインを設けています。
ただし、システムに不必要な機能が追加されようとしている場合は、これを拒否できます（それがよくできたコードであっても）。

ここで重要なのは、完璧なコードを求めない ということです。
CLの小さな変更にこだわらず、プロジェクトとして求められている重要な機能追加や変更にフォーカスすることが大事です。

レビュワーが務めなければならないのは、継続的改善です。

以下は、レビュー時に頭にいれておくべき原理原則です。

• 技術的事実とデータに基づいて改善を行う
• コードのスタイル（スペースかタブかといった問題など）は チームのコーディングスタイルガイドを定め。これに従う
• ソフトウェア設計の原則に従う（個人の好みでソフトウェア設計、アーキテクチャ設計を行わないこと）

とにかく、個人の好みを反映させないことが大事！

コードレビューで見るポイント

• デザイン
• 機能性
• 複雑性
• テスト
• ネーミング
• コメント
• スタイル
• ドキュメント
• 行単位レビュー
• コンテキスト
• 良い点（グッドポイント）

以降では、これらを詳しく見て行きます。

デザイン

CL全体のデザインを見ます。
コードの相互作用性や、どのコードの修正なのか、他のシステムとの連携はあるか、リリースのタイミングは問題ないか、
などがきちんと表現されているか。
※うーん、このデザインの観点がよくわからないな。。。

機能性

このCLにはどのような目的があるか（どのような目的で修正が施されているか）を確認します。
エンドユーザ及び他の開発者にとって良い変更（良い機能追加や修正）となっているかを確認します。

レビュワーは、極端なユースケースについても考慮し、かつユーザのように考え、
コードの表面的な記述からバグがないことが確認できる必要があります。
そのような変更がすぐにわからない時は、開発者にデモをしてもらう等して確かめて下さい。

並列プログラミングなどデッドロックやレースコンディションが発生する場合は、特に気をつけること！

複雑性

複雑すぎないCLとなっているかどうかを確認します。
複雑すぎるとは、
読んですぐに理解できないコード
ということになります。
このようなコードは、開発者がバグを仕込んでしまう要因となりえるため注意して下さい。

オーバーエンジニアリングはしないこと（させないこと）！

テスト

CLと合わせてテストコードが含まれていることを確認して下さい。
テストコードが正しく動作し且つ有用なものであることも確認して下さい。

正しく動作し有用なテストとは、エラー時にはテストがきちんと失敗したりテストメソッドが他のテストと分離していたりすることです。
※ここはもう少し勉強します。。。具体性にかける。。。

なお、テストにも複雑さを持ち込まないこと！

ネーミング

良い変数名、関数名、クラス名が付けられているかを確認して下さい。
長過ぎたり、意味が読み取りづらくないかを確認して下さい。

コメント

なぜそのコードが存在するのかをコメントで説明されているか確認します。

そのコードが何をするか（どんな処理をするか）をコメントしないこと

シンプルなコメントで表現できないようなコードでれば、コードがシンプルとなる修正を行うように案内して下さい。

なお、ドキュメントとコメントが異なる目的で書かれることに留意して下さい。
ドキュメントには、クラス、モジュール、関数の目的や使い方、振る舞いを記載します。

※コメントにはドキュメントに書くようなことは書きません、ドキュメントとコメントの役割をしっかり区別しましょう。

スタイル

Google には あらゆるプログラミング言語のスタイルガイドが存在し、CLはこれに従います。
スタイルガイドについては、下記を参考にして下さい。
参考： Google Style Guide

スタイルガイドにない観点で修正したい場合は、コメントにその旨を記載しているか確認します。
個人的な好みでCLをブロックすることはやめましょう。

仮にスタイルを変更するCLを送信する場合、スタイル変更だけを行ったCLを作成するように案内して下さい。
他の変更点と合わせてCLすることはやめましょう。

ドキュメント

CLにコード追加、変更等が含まれる場合は、
それらのコードについて言及したドキュメントも更新してあるか確認します。
CLにコードの削除が含まれる場合は、
削除したコードに言及したドキュメントも合わせて削除するかを検討します。

行単位レビュー

自分の担当となっている箇所は、1行1行見てそのコードが何を行っているかを確認して下さい。
コードが難しすぎる場合は、開発者に依頼して説明してもらったりしながら内容を明らかにしましょう。
コードの専門性が高い場合（セキュリティや並行動作など）は、そのコードをレビューできる人をアサインするかその人に委任しましょう。

コンテキスト

レビューする際は、変更のあった箇所だけでなく、ファイル全体やシステム全体を通してレビューして下さい。
つまり、ある修正を全体を通してみた時に、それがコードの健全性を向上させるか、複雑になっていないか、
といったことを確認して下さい。

良い点(グッドポイント)

CLにおける良い修正はちゃんと褒めましょう。
レビュワーの責務であり、これがメンタリングというものです。

CLレビューの進め方

CLは、以下のような流れで確認するのが良いでしょう。

• CLは意味のある（妥当な）変更かを確認する
• CLの主要な変更箇所を第一に確認し、当該箇所がうまく設計されているかを確認する
• その後、残りのCLを確認する

コードレビューの速度

Google では、開発者のコーディング速度を早くするのではなく、
デベロッパチームと一緒にプロダクトを作るための速度を向上させています。

コードレビューが遅くなると、その分開発者は待たされることになり、
レビューバックが発生する毎に開発はさらに遅れます。
ゆくゆくはコードの健全性にも影響し、より良いCLの提出を妨げてしまいます。

ではどのようにしてコードレビューを高速化するかというと、
CLへの第一回答は1営業日以内にする という方針の下で行うようにします。
もし、あなたがとても忙しくレビューの第一回答が間に合わなそうな場合は、
CLを受け取ってすぐに別のレビュワーをアサインする旨を連絡します。

なお、レビュワーがコードレビューを行うための十分な時間をかけて行うことも重要です。
同時に素早いCLへの第一回答も同じように重要なのです。

まれに、巨大なCLが来る場合もありますが、その場合はいくつかの小さいCLに分割して送信するように案内して下さい。
どうしても分割できない場合は、CL全体に関するコメントのみ記載して下さい。

どのようにレビューコメントを書くか

レビューコメントは以下のような方針で記載するのが良いでしょう。

• 親切なコメントを心がけましょう
• 理由を明確に延べましょう
• 開発者に裁量のある指摘表現にしましょう
• 複雑なコードは、シンプルな設計とするかコメントを追加するような案内をしましょう

レビューバックのやり方

レビューバックについては、CL が要件やCL標準を満たしていないと判断した場合に発生します。
その際、開発者と意見が対立するケースがあります。
この場合は、CL全体を通してそれ自体が妥当であるか、健全性が保たれているか、をもう一度確認します。

もちろん、開発者が正しい主張をしていないケースもあるため、この場合はなぜレビューバックなのか、
という理由をきちんと説明します。
また、レビューバックによってコードの健全性が保たれ且つこのレビューバックによって追加のコード修正が発生する場合は、
開発者にこれらの作業を依頼します。
なお、追加の修正等が発生する場合は、その修正をいくつかのステップに分けるなどして進めると良いでしょう。