miso_soup3 Blog

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

Google's Code Review Guidelines を読んでアクションに落とす

Summary

Google's Code Review Guidelines を読み、具体的にどうアクションするかを念頭に置くため、アクションリストだけをまとめました。

レビュアー向けのガイド

The Standard of Code Review 基準
  • プルリクのコードが完璧でなくても、確実に改善するのであれば、積極的に承認する。
  • 重要でない指摘は、”Nit:” をつけることで、変更するか否かは作者に委ねることを伝える。
  • コードレビュー上のコメントでのやりとりが難航しそうなとき、対面のミーティングやビデオ会議を行う。
    • そのとき、他のレビューワーのために、議論の結果はコメントとして記録する。
  • 合意を得られないからといって、プルリクを放置しない。
What to Look For In a Code Review 何を見るか
  • ユーザーへの影響を理解するにはコードだけでは分からないこともあるので、動作確認を行う。
    • 難しい場合は、作者にデモをお願いする。
  • オーバーエンジニアリングに警戒する。
  • 適切なテストを求める。
  • コード中のコメントがWhyであるか、Whatになっていないかを確認する。コメントが必要なものかを確認する。
  • 個人的なスタイルの好みに基づくものは“Nit:”とする。
  • 関連するドキュメントのアップデートを依頼する。
  • すべての行を読み、自分で理解する。
    • コードを読むのが難しい場合は、明快にするよう作者に伝える。
    • レビュアーとして自分がふさわしくないと感じるのなら、他の人をレビュアーにアサインする。
  • コードの変更で良いところがあったら、それを作者に伝える。
Navigating a CL in Review どう進めるか
  • ステップ1: 説明をよく読む
    • プルリクの説明はよく読む。
    • 変更が行われるべきでない場合は、すみやかに、理由・代替案を丁寧に説明する。
    • 望ましくない変更が多く発生する場合は、コードレビューの前の開発プロセスを見直す。
  • ステップ2: メインとなる部分を見つけ、設計を確認する
    • まず、メインとなる部分を探す。
    • コードの変更が大きい場合は、分割を依頼する。
    • 設計に問題がある場合は、すみやかにコメントを送る。
  • ステップ3: 他の部分を読む
Speed of Code Reviews スピード
  • 集中しているときでなければ、レビューがきたらすぐに行う。レビューで自分の集中を妨げない。
  • コーディングが完了したとき、昼食の後、キッチンから戻ってきたときにレビューを行う。
  • 1営業日中までにレスポンスする。
  • レビューは翌朝の最初に行う。
  • 忙しい時は、いつできるかをレスポンスする。他のレビュアーを提案する。最初に大雑把なコメントをする。
  • 未解決なコメントがあったとしても、細かい変更であったり、後で変更されるだろうと言う場合は、承認する。
How to Write Code Review Comments コメントの書き方
  • 礼儀、あいまいではなくはっきりとした態度、尊敬する気持ちをもつ
  • 開発者に対してコメントをしない。コードについてコメントをする。
  • 「なぜか」を説明する。
  • 問題を指摘することと、具体的なコードを提供することの間で適切なバランスを取る
Handling Pushback in Code Reviews
  • 対立がおきたとき:
    • 立ち止まり、開発者の意見が正しいのではと考える。
    • 理解しようとする姿勢が伝わるようにする。
    • なぜ、提案が正しいと思うのかを、丁寧に説明する。
  • 後でやる、が発生したとき:
    • 「後でやる」が実行されることは少ないので、今すぐやるべきと主張する
    • すぐできないときは、チケットを切る
  • レビュー基準のブレにより不満が発生したとき:
    • 恐らくその不満は、レビューのスピードをあげることで解消できるので、それを検討する

プルリクの作成者向け

Writing Good CL Descriptions
  • 1行目:短い要約。コードの変更によって具体的に何がなされたのか。(英語の場合は、命令形を使う)
  • 残りの本文は、十分な説明を書く。何を解決したか。なぜそれが良い解決方法であるのか。欠点は何か。なぜ変更したか。
  • プルリクを送信する前に、見直す。
Small CLs
  • コードの変更は、1つのことだけを扱う。
  • 100行は妥当、1000行は大きすぎる。1ファイルの200行は問題ないかもしれないが、50ファイルにわたる200行は大きすぎる。
  • 分割した場合は、他のプルリクがあることを伝える。
  • リファクタリングは分離する。
  • 関連するテストコードは分離しない。
  • 大きなコード変更をする前に、先にリファクタリングのみ行う。
  • コードの変更が大きくなった時は、レビュアーに了承を得る。
How to Handle Reviewer Comments
  • コメントに怒りで返事をしてはいけない。
  • レビュアーのコメントが建設的でないときは、対面かビデオ会議する。
  • レビュアーがコードを理解できなかった場合、コードを適切に変更するか、コードにコメントを残す。
  • コメントが来たとき、反射的に考えるのではなく、立ち止まり、そのコメントは正しいか?をよく考える。
  • 対立したら:
    • コードレビューの基準を読む。