Google コードレビュー・ガイドライン要約【コードレビューの基準編】
はじめに
レビュー基準について再考する機会があり、調べていたところ、すばらしい記事を発見しました。
Google がコードレビュー・ガイドラインを公開しているようです。
有志の方による 日本語訳版 もありました。
今回は日本語訳版の内容を元に、筆者なりに概念をまとめてみました。
「Google コードレビュー・ガイドライン要約」シリーズとして、書いていきます。
シリーズ構成
※リンクが貼られていない箇所は未要約です
- コードレビューの仕方
- コードレビューの基準 ← here!!
- コードレビューの観点
- コードレビューの進め方
- コードレビューのスピード
- コメントの書き方
- 取り下げへの対応
- CL 作成者のガイド
- 適切なディスクリプション
- 小さな CL
- レビューコメントへの対応
- 緊急事態の CL
本シリーズ通して
本記事のポジション
本記事の要約は「1企業のサービス運営者」目線でまとめています。
Google Engineering Practices Documentation は、Googleの開発ドキュメントです。内容には度々 OSS や Google 社内の文脈が登場します。私は1企業の Web サービスの開発者であり、そういった文脈は意図して省略する場合があります。また、そういった文脈であると判断した場合は[OSS] や [Google] というタグをつけています。
要約の方向性
この要約は筆者なりの解釈が含まれていることに留意してください。筆者が「概念的にこのようなことを言っている」「こちらの表現の方が的を射ている」と感じた場合は、そのようにまとめています。
あえて原著・元記事の表現を残している部分があります。必ずしもきれいに一言で言い表せないケースもあり、ニュアンスで伝えようとしている部分があると感じたためです。その場合は、仮に日本語的に読みづらくなっていても、言葉を削らすに書いています。
用語
※日本語訳の記事にある「健康状態」を「品質」に置き換えています
- CL
- 「changelist」の略
- 一つの独立した変更を意味していて、バージョン管理に提出されたものや、まだコードレビュー中のものを指すこともある
- 他組織では「変更」「パッチ」とよく呼ばれる
- LGTM
- 「Looks Good to Me」という意味
- コードレビュアーが CL を承認するときに言う
- 開発者
- CL の提出者のこと
- もしくは、コードベースを触る他の人のこと
コードレビューの基準
コードレビューの目的
- コードの全体的な品質を時間をかけて改善すること
- それを実現するために、さまざまなトレードオフのバランスを取る必要がある
コードレビューの基本方針
- 認証基準
-
レビュアーは、CL が完璧でなくとも、コードの品質を高めることが確実なら、積極的に承認する
- コードレビューの全ガイドラインで最上位の原則である
- レビュアーがすべてに対していちいち難色を示し、変更を取り入れずにいるようなことをすると、開発者は改善の意欲を失う
-
レビュアーは、CL が完璧でなくとも、コードの品質を高めることが確実なら、積極的に承認する
- 完璧さを求めない
- 完璧なコードといったものは存在しない
- より良いコードがあるのみ
- レビュアーが追求すべきは継続的な改善である
- タスクを前に進めることの必要性と、変更の重要性を比較して、バランスよく検討する
- 完璧なコードといったものは存在しない
- 品質チェックはレビュアーの義務
- このドキュメントは、コードの品質を悪化させるとわかりきっている CL を正当化しない
- 唯一それが許されるケースは、緊急事態 のみである
- CL を取り入れてコードの全体的な品質がだんだんと悪化することは問題のため、きちんと確認する
- このドキュメントは、コードの品質を悪化させるとわかりきっている CL を正当化しない
レビュアーのコメント
- 積極的なコメント
- レビュアーは改善できそうな点を見つけたら、 いつでも気軽にコメントを残すことができる
- コメントを残すのに躊躇する必要はない
- Nit プレフィックス
- 重要でない指摘であれば「Nit: 」（あら探しや細かい指摘という意味）のようなプレフィックスを付ける
- この CL で作成者が解決する義務はないことを伝える
メンタリング
- 知識の共有
- コードレビューは、新しい知識を教える重要な機会となる
- 知識の共有は、時間をかけてコードの品質を改善する試みの一部分である
- Nit プレフィックス
- コメントが純粋に教育目的であれば「Nit: 」プレフィックスを付ける
原則
- 客観性
- 技術的な事実とデータが、個人的な意見と好みをくつがえす
- スタイル
- スタイルに関しては、スタイルガイドが絶対的な権威である
- スタイルはシステム内で一貫している必要がある
- 例えば Google のスタイルガイドは こちら
- スタイルガイドに記載のない、純粋なスタイルの選択は、個人の好みの問題である
- 空白をどうするかなど
- スタイルに前例がなければ、CL 作成者のやり方を受けれる
- スタイルに関しては、スタイルガイドが絶対的な権威である
- 基本原則
- ソフトウェア設計に関する論点は、ほとんどの場合、個人的な好みの問題ではない
- ソフトウェア設計の標準的な原則を優先することに重点に置く
- 一貫性
- 以下の条件を満たす場合、レビュアーは作成者に、現在のコードとの一貫性を維持するよう求めることができる
- コードレビューにおける判断基準やガイドラインの範囲外である
- システムのコードの全体的な品質を悪化させない
- 以下の条件を満たす場合、レビュアーは作成者に、現在のコードとの一貫性を維持するよう求めることができる
意見の対立の解消
- 有効な選択肢がいくつかあるとき
- 複数の方法が同等に有効であると作成者が証明できるとき（データを示したり、堅固な工学原理に基づいた説明）
- レビュアーは作成者の選好を受け入れる
- コードレビューで意見が対立するとき
- 技術的な事実とデータや設計原則に基づく議論を行う
- 本ドキュメントや他のドキュメントに基づいて、開発者とレビュアーの間でコンセンサスが得られるように調整する
- コンセンサスを得るのが特に難しいとき
- レビュアーと作成者で同期コミュニケーションを取る
- 対面のミーティングやテレビ会議など
- コメントのやり取りだけで対立を解消しようとするよりも、効果的な場合がある
- レビュアーと作成者で同期コミュニケーションを取る
- それでも状況が変わらないとき
- 通例では、巻き込む人を増やすことが解決方法である
- 例えば、広範にチーム内で議論すること
- テックリードに相談する
- 技術マネージャーに助力を求める
おわりに
いかがだったでしょうか。
個人的に Google Engineering Practices Documentation を通してかなりの学びがありました。
特に CL 提出者への気遣いと、レビュアーによる品質担保のバランスがトレードオフになっており、まとめていて難しいところだなと感じました。
コードのすべての細部にコメントし、それが通るまで承認しないと、CL 提出者側が疲れてしまう、という部分に触れていることが印象的でした。
その反面、明確にコードの品質を悪化させる CL を正当化しないことを強調している点が、単純な話ではないことを示していると感じました。
このドキュメントを通じて少しでも皆さんの助けになれば幸いです。
Discussion
原著では「code health」と表記されており、日本語での適切な表現がわかりませんでした
暫定で品質としています
こちら詳しい方いましたらご享受願いたいです
勉強になる内容でありがとうございました、他人の要約をみると改めて考えが整理されます。
暫定表現について、私見を以下の通りまとめましたので
一助になれば幸いです
Code Health の適切な訳は難しいところですが、
一方で「品質」という訳は、必ずしも完全さを求めない Code Health の概念と
やや衝突する印象があります。
この点については英語表現で "quality" ではなく "health" を使っている点からも、
ニュアンスの違いが伺えるのではないでしょうか。
「The Standard of Code Review」から：
これに基づくと、Code Health は、べき論や理想論だけでなく、
チーム事情を加味してバランスの良い状態に
どれだけ近いかを示す状態を意図しているように受け取れます。
上記の考えから、Code Health の訳としては意訳ながら
「健全さ」「健全性」などしっくりくるように感じました。
(「健康状態」と似ていますが、健康と健全でのニュアンスの差が割と大きいかなと)
たしかに過ぎました！
「健全さ」とても良いと感じたので、随時こちらに書き換えようと思います 🙏