Google コードレビュー・ガイドライン要約【コードレビューの基準編】

はじめに

レビュー基準について再考する機会があり、調べていたところ、すばらしい記事を発見しました。

https://google.github.io/eng-practices/

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 を取り入れてコードの全体的な健全性がだんだんと悪化することは問題のため、きちんと確認する

レビュアーのコメント

• 積極的なコメント
  • レビュアーは改善できそうな点を見つけたら、 いつでも気軽にコメントを残すことができる
  • コメントを残すのに躊躇する必要はない
• Nit プレフィックス
  • 重要でない指摘であれば「Nit: 」（あら探しや細かい指摘という意味）のようなプレフィックスを付ける
  • この CL で作成者が解決する義務はないことを伝える

メンタリング

• 知識の共有
  • コードレビューは、新しい知識を教える重要な機会となる
  • 知識の共有は、時間をかけてコードの健全性を改善する試みの一部分である
• Nit プレフィックス
  • コメントが純粋に教育目的であれば「Nit: 」プレフィックスを付ける

原則

• 客観性
  • 技術的な事実とデータが、個人的な意見と好みをくつがえす
• スタイル
  • スタイルに関しては、スタイルガイドが絶対的な権威である
    • スタイルはシステム内で一貫している必要がある
    • 例えば、Google のスタイルガイドは こちら
  • スタイルガイドに記載のない、純粋なスタイルの選択は、個人の好みの問題である
    • 空白をどうするかなど
    • スタイルに前例がなければ、CL 作成者のやり方を受けれる
• 基本原則
  • ソフトウェア設計に関する論点は、ほとんどの場合、個人的な好みの問題ではない
  • ソフトウェア設計の標準的な原則を優先することに重点に置く
• 一貫性
  • 以下の条件を満たす場合、レビュアーは作成者に、現在のコードとの一貫性を維持するよう求めることができる
    • コードレビューにおける判断基準やガイドラインの範囲外である
    • システムのコードの全体的な健全性を悪化させない

意見の対立の解消

• 有効な選択肢がいくつかあるとき
  • 複数の方法が同等に有効であると作成者が証明できるとき（データを示したり、堅固な工学原理に基づいた説明）
  • レビュアーは作成者の選好を受け入れる
• コードレビューで意見が対立するとき
  • 技術的な事実とデータや設計原則に基づく議論を行う
  • 本ドキュメントや他のドキュメントに基づいて、開発者とレビュアーの間でコンセンサスが得られるように調整する
• コンセンサスを得るのが特に難しいとき
  • レビュアーと作成者で同期コミュニケーションを取る
    • 対面のミーティングやテレビ会議など
    • コメントのやり取りだけで対立を解消しようとするよりも、効果的な場合がある
• それでも状況が変わらないとき
  • 通例では、巻き込む人を増やすことが解決方法である
  • 例えば、広範にチーム内で議論すること
    • テックリードに相談する
    • 技術マネージャーに助力を求める

おわりに

いかがだったでしょうか。
個人的に Google Engineering Practices Documentation を通してかなりの学びがありました。

• CL 提出者への気遣いが何度も出てくる
  • レビュアーが追求すべきは、完璧さではなく継続的な改善
    • 完璧なコードは存在しない
    • すべてに難色を示したりして CL を止めない
  • Nit プレフィックスを付与して「あなたにこの修正を行う義務はない」ことを伝える
• 反面、CL の品質担保も強調している
  • CL を取り入れて、コードの全体的な健全性がだんだんと悪化するのは問題
• ソフトウェア設計に関する論点
  • 「ソフトウェア設計に関する論点はほとんどの場合、純粋なスタイルの問題でも個人的な好みの問題でもない」と言い切っている
  • ソフトウェア設計の基本的な原則に重点を置くべきである

このドキュメントを通じて少しでも皆さんの助けになれば幸いです。

参考文献

• google/eng-practices - Google's Engineering Practices documentation
  • Google Engineering Practices Documentation
• FujiHaruka/google-eng-practices-ja
  • Google Engineering Practices Documentation (日本語訳)
• Googleに学ぶコードレビューのポイント