Zenn
NCDCエンジニアブログ
📝

Google Engineering Practices Documentationからコードレビューを考える 〜実装者ガイド編〜

ゆーと
2024/11/28に公開
GitHub
Google
pr
codereview
idea

はじめに

コードレビューをするうえで適切なコードレビューができているか？
個人の趣味嗜好のレビューになっていないか？もっといいレビュー方法や観点がないか？

などなど、考えることが多々あったので、今一度コードレビューの考慮点を Google のドキュメントを参考に学び直してみました。

※ 基本的にはドキュメントの内容を踏襲してますが、直訳だと理解しづらい部分も多々あるので、自分なりに解釈して記載している部分もあることご了承ください。




Google Engineering Practices Documentation

Google Engineering Practices Documentationとは、Google が公開しているエンジニアリングプラクティスについてのドキュメントです。

Google が公開しているドキュメントなので、まずはここに記載のプラクティスを参考にすれば間違いないかなと思います。
ここに記載の内容をベースに、チームにあったルール等を追加する感じがいいのかなと。

ドキュメントの大枠構成は以下。

Google’s Code Review Guidelines
├─ The Code Reviewer’s Guide
└─ The CL Author’s Guide（The CL author’s guide to getting through code review）

※ このドキュメントではCL（ChangeList）という言葉が出てきますが、Google の社内用語のようです。記載にもある通りPRのこと、もしくは文脈にもよりますがcommitのことを指していると解釈してよささそうです。

CL: Stands for “changelist”, which means one self-contained change that has been submitted to version control or which is undergoing code review. Other organizations often call this a “change”, “patch”, or “pull-request”.




Google’s Code Review Guidelines

What Do Code Reviewers Look For?（レビュアーは何を見るか？）

コードレビューでは以下のような点に注目する。

  • 設計：コードがよく設計され、システムに適しているか。
  • 機能性：コードは作者が意図したとおりに動作するか？コードの動作はユーザーにとって良いものか？
  • 複雑さ：コードをもっとシンプルにできるか？別の開発者が将来このコードに出会ったとき、簡単に理解して使えるだろうか？
  • テスト：コードに正しく設計された自動テストがあるか？
  • 命名：開発者は変数、クラス、メソッドなどにわかりやすい名前を選んだか？
  • コメント：コメントは明確で有用か？
  • スタイル：コードはスタイルガイドに従っているか？
  • ドキュメント：開発者は関連ドキュメントも更新したか？

→ PR テンプレにチェックボックスとして上記項目を入れておくのもいいかも。

Picking the Best Reviewers（最高のレビュワーを選ぶ）

一般的に、あなたは、合理的な期間内にあなたのレビューに応答することができるあなたができる最高のレビュアーを見つけたい。

最良のレビュアーとは、あなたが書こうとしているコードについて、最も綿密で正しいレビューをしてくれる人のことです。これは通常、コードの所有者を意味し、OWNERS ファイルの人たちである場合もありますし、そうでない場合もあります。時には、これは CL の異なる部分のレビューを異なる人に依頼することを意味します。

もし理想的なレビュアーが見つかっても、その人がいない場合は、少なくともあなたの変更についてその人に CC すべきです。

適切なレビュアーを選ぶことが重要だよ。っていう記載。

理想はその通りだけど、人的リソースが潤沢な大きな開発チームじゃないと厳しそう。
ただ、数人の開発チームでも、PR の異なる部分のレビューを異なる人に依頼するというのは良さそう。

全体構成や方向性などを信頼できるレビュワーに相談できるってのは良い。





「実装者ガイド」と「レビュワーガイド」の章があるが、まずは「実装者ガイド」から見ていく。

The CL author’s guide to getting through code review（PR 作成者のためのコードレビュー突破ガイド）

このセクションのページには、コードレビューを受ける開発者のためのベストプラクティスが書かれています。
これらのガイドラインは、レビューをより速く、より質の高い結果で通過するのに役立つはずです。
すべてを読む必要はありませんが、Google のすべての開発者に適用できるよう意図されています。

項目は以下の三つ。順に見ていく。

  • Writing Good CL Descriptions
  • Small CLs
  • How to Handle Reviewer Comments




Writing good CL descriptions（優れた PR の書き方）

PR や Commit は、公的な変更記録であり、読み手に伝わること、未来に伝えることが重要。

  • RP 内容は github 上で永久的に残り、何年にも渡って沢山の人に読まれる可能性がある。
  • 将来に、誰かがある機能が実装された PR を検索することがあるでしょう。そこで PR 説明分に必要な情報が書かれてないと、見つけるのは非常に困難に。知りたいのはコードの中身や実装背景なのに、そもそも検索段階で見つけれなくなる。
  • また、見つけれたとしても、何をしているコードか、中身を詳細に読めば理解できるだろうが、変更された背景がわからない。ソースコードから背景を推測するしかなくなる。（ただの憶測となってしまう。）

→ 丁寧に書かれた PR は、将来のチームメンバーを助けることになる。


【PR 時のポイント】

  1. どんな変更がなされるか。
    変更コードを読まなくても、何が変更されたかがわかるように、主要な変更点を要約すること。
  2. なぜこの変更が加えられたのか。
    変更の背景や目的を説明すること。ソースコードだけでは表現できない背景を説明すること。


First Line

※ 以下、1 行目〜3 行目といった記載になっているが、個人的には以下の解釈として当てはめて考えてみます。
1 行目：PR title、コミットメッセージ 1 行目
3 行目：PR description、コミットメッセージ 3 行目以降

  • PR のタイトルやコミットメッセージ 1 行目には、その PR が何を行っているのかを明確に記した短い要約を書くこと。
    • PR の全貌を確認しなくても、その PR が何を行っているのかが理解できるぐらいに明確であること。
    • 読み手によって分かりやすさと実用性を第一に考えること。
  • 完全な文章であるべき。（日本語だと。で終わる文章）


※ 補足
ドキュメントには他にも命令文で書くことが推奨されているが、日本語で記述する場合は、あまり明確な使い分けはないかも。英語文法での命令文は日本語だとあまり違いないかも。

"Delete the FizzBuzz RPC and replace it with the new system.”
"Deleting the FizzBuzz RPC and replacing it with the new system.”


Body is Informative（本文は詳細に）

  • 本文には PR 全体を理解するために必要な補足情報を含めること。
    • 解決しようとしている問題の簡単な説明や、変更の背景、なぜこの変更が最良かと思ったか、などを記載するのも良い。
    • もし変更内容に欠点が既にある場合は、それについても記載すること。
    • 関連があれば、ドキュメントなどのリンクなどの背景情報も記載すること。
      • チャットで仕様確認してるような場合は、やり取り先のリンク添付も必要な場合も。
      • 外部リンクを含める場合は、将来の読み手が閲覧できない可能性に注意。できれば 外部リンクに飛ばさなくても PR 内で完結するのが理想。
      • その場合は スクショとかでもいいかも。
  • 小さな PR でも細部に注意を払い丁寧に記載すること。


Bad CL Description（悪い PR 例）

以下のような説明文だけでは不適切。
「どんな？なんのため？何をした？」などの有用な情報を十分に提供できていない。
短いとはいえ、情報が十分でなさすぎる。

  • “Fix build.”
  • “Add patch.”
  • “Moving code from A to B.”
  • “Phase 1.”
  • “Add convenience functions.”
  • “kill weird URLs.”


Good CL Description（良い PR 例）

※ 日本語直訳で記載してます。原文はドキュメント参照ください。

「RPC: RPC サーバーのメッセージフリーリストのサイズ制限を撤廃します。

FizzBuzz のようなサーバーでは非常に大きなメッセージが扱われ、再利用が有効です。
フリーリストを大きくし、ゴルーチンを追加して、時間をかけてフリーリストのエントリを徐々に解放することで、アイドル状態のサーバーは最終的にすべてのフリーリストエントリを解放するようにします。」


「status.py に対する Python3 のビルドルールを作成してください。

これにより、既に Python3 を使用している利用者は、自分たちのディレクトリツリーのどこかではなく、元のステータスビルドルールの隣に依存するルールを持つことができます。
これは、Python2 ではなく、可能であれば Python3 を使用するよう新しい利用者に促すものであり、現在作業中の一部の自動化されたビルドファイルリファクタリングツールを大幅に簡素化します。」


Using tags

PR を分類するためのラベル。
ツールで自動生成されるものや、チームの慣習で使用するものなど。

例えば、

  • “[tag]”
  • “[a longer tag]”
  • “#tag”
  • “tag:”

タグの使用は任意である。
もし、タグを使用する場合は、PR タイトル に入れるか、PR 本文 の最初に入れるかを検討すること。

注意として、PRtitile にタグを入れる場合は、タグが長すぎると PR のタイトルが見づらくなるので注意。短い tag を使用するなどのルールはチームで必要。

悪い例
// タグが多すぎる（あるいは長すぎる）ため、タイトルが圧迫されます。
// 代わりに、タグを本文に移したり、短縮したりできるかどうかを検討してください。
[banana peeler factory factory][apple picking service] Assemble a fruit basket.


※ ここでいう tag は prefix のことも含まれると解釈してもよいかもです。
https://qiita.com/muranakar/items/20a7927ffa63a5ca226a


Generated PR descriptions（生成された PR について）

自動生成される PR タイトルやコミットメッセージ、PR 本文なども、できるだけ前述のルールに従うこと。

あくまでも自動生成は雛形の作成として、出来上がった中身はしっかりと確認することが大事。
便利なので、使うとこは使いつつも。


Review the description before submitting the CL（PR を出す前に PR 内容を確認する）

PR を出す際（レビュー依頼する際）は、PR の内容をしっかりと確認することが重要。

  • PR 出した後、指摘修正によっては PR 内容が当初から変更される可能性もあることに注意。

    • 指摘修正した内容に対して PR 説明文も追随すること。
      • 削除して書き直すは微妙かも。軽微な記載修正ならいいが。
      • 指摘修正後は、取り消し線やアコーディオンなどで変更履歴として見れる方が適切かも。将来見返した時のためにも。

  • 他の作業者の PR マージによって、当初予定していた 変更内容が変わる可能性も考慮に。

    • コンフリクト解消作業によって、PR 説明文に修正が必要になることも。




Small CLs（小さな PR）

このセクションでは、PR を小さくする理由や、ポイントが記載されています。

Why Write Small CLs?（なぜ小さな PR を書くのか？）

小さくシンプルな PR の利点は以下。

  • より迅速にレビューできる：レビュワーにとっては、1 つの大きな PR を査読するために 30 分の時間を確保するよりも、小さな PR を査読するために 5 分の時間を数回確保する方が簡単である。
  • より徹底的にレビューされる：大規模な変更の場合、レビュアーや実装者は、大量の詳細なコメントが行ったり来たりすることにイライラしがちです。
  • バグが発生しにくい：変更箇所が少ないので、あなたとレビュアーが PR の影響について効果的に推論し、バグが導入されたかどうかを確認することが容易になります。
  • リジェクトされた場合の無駄な作業が減る：もしあなたが膨大な PR を書いて、レビュアーが全体的な方向性が間違っていると言ったら、あなたは多くの仕事を無駄にしたことになります。
  • マージしやすい：大きな PR に取り組むには時間がかかるので、マージするときに衝突が多くなり、頻繁にマージしなければならなくなる。
  • デザインしやすい：小規模な変更の方が、デザインやコードの磨きをかけることが容易であり、大規模な変更のすべての詳細を洗練するよりも効率的です。
  • レビューのブロックが減る：変更全体の自己完結的な部分を送信することで、レビュー中の PR を待つ間、コーディングを続けることができます。
  • ロールバックが簡単：大規模な PR は、ロールバックが発生した際に複雑になる可能性が高い。

レビュアーには、「変更が大きすぎる」という理由だけで、あなたの変更を真っ向からリジェクトする裁量権があることに注意してください。

レビュアーは実装者に感謝しつつも、粒度が大きいと適切な粒度の PR にまとめるよう指摘/要求することもあるでしょう。
その場合、

  • すでに PR まで出した後で PR を分割する作業が発生。→ 骨の折れる作業となる。
  • 大きな PR をレビュアーに受け入れてもらうにしても、受け入れるための納得する理由をレビュアーとやり取りする時間が必要になる。→ 無駄なやり取り工数の発生。
    → 最初に小さな PR を書く方がお互いが楽。


What is Small?（小さいとはどういうこと？）

一般的には 「PR の適切なサイズは、一つの自己完結的な変更」であるべきです。
つまり、一つの事柄だけを対象とし、その一つの事柄を対処する最小限の変更（実装）にすること。

  • 機能全体を一度の変更（実装）するのではなく、機能の一部分だけを変更すること。

  • 一般的には、大きすぎる PR よりも小さすぎる PR を書く方が望ましい。レビュアーと協力して許容できる PR サイズを見極めましょう。

  • PR は関連するテストコードを含むべきである。

  • レビュアーがこの PR を理解するために必要な情報は全て、PR そのもの、その説明文、プロジェクトの既存コード、または以前にレビューした PR の中にあります（ただし、将来の開発計画は除く）。

  • PR の変更が develop や production ブランチにマージされた後も、システムは引き続きユーザーや開発者に対して問題なく動作することが期待される。

  • PR は小さすぎると、何のための変更なのか理解しづらくなることがあります。新しい機能を作ったらその機能はどのように使われるのかを示す例（ドキュメントやテストコードなど含む）を同じ PR に含むべきです。これを行うことで、他の開発者がその新しい機能をどのように使えばいいのかをすぐに理解できるようになります。また、実際には使用されない機能がシステムに加えられることを避けることもできます。

どのくらいの大きさが "大きすぎる"かについて、厳密なルールはありません。
通常、100 行は PR として妥当な大きさであり、1000 行は大きすぎますが、レビュアーの判断次第です。
変更が及ぶファイルの数も "サイズ "に影響します。1 つのファイルで 200 行の変更は問題ないかもしれませんが、50 のファイルにまたがる変更は通常大きすぎます。

実装者は、コードを書き始めた瞬間からそのコードに深く関わっていますが、レビュアーにはその背景がないということを心に留めとおいてください。
自分ではちょうど良いと思う PR も、レビュアーにとっては大きすぎると感じることがあります。不安な時は思ってるよりも小さい PR を書くようにしてください。
レビュアーは通常、小さすぎる PR について不満を述べることはありません。


When are Large CLs Okay?（どのような場合なら大きい PR は OK か？）

大きな変更がそれほど問題にならない状況もいくつかあります。

  1. ファイル全体を削除した時。
    変更量が多く見えても実質一行の変更と捉えることができるため。
  2. 自動リファクタリングツールによって生成された大きな変更がある場合。
    ツールが信頼できるものであれば、レビュワーの役割はその変更が正しい稼働かを確認するだけです。
    この場合は PR は大きくても問題ないが、上記で述べたマージによる影響やテストなどは依然として考慮する必要があることは注意。


Writing Small CLs Efficiently(小さな PR を効率的に書く)

小さな PR を書いて、レビュアーの承認を待ってから次の PR を書くとしたら、多くの時間を無駄にすることになる。
そのため、レビュー待ちの間も作業を止めずに進める方法を考えることが大切です。

レビュー待ちの間も作業を止めずに進める方法としては、以下のような対応が挙げられます。

  • 複数のプロジェクトに同時に取り組む
  • すぐにレビューできる人を確保する
  • 直接会ってレビューを行う
  • できるだけ PR 同士の依存関係がないように作業を切り出す
  • PR 出してるブランチから新たにブランチを切って作業を開始する


Splitting CLs（PR を分割する）

複数の PR が互いに依存関係を持つ作業を始める場合、コーディングに着手する前に、それらの PR をどのように分割し、整理するかを高いレベルで考えることがしばしば役立ちます。

これは、あなた自身が PR を管理し整理するのを容易にするだけでなく、レビュアーにとっても作業が容易になります。

以下は、作業を異なる PR に分割するための戦略です。

Stacking Multiple Changes on Top of Each Other（複数の変更を積み重ねる）

レビュー待ちで作業を止めない方法として、PR を出したブランチから、新しいブランチを切って新たな PR のための作業を始めること。

Splitting by Files(ファイルごとに分割)

異なるレビュアーが必要だが、PR を出す際に、ファイル種類によって別々のレビュアーにレビュー依頼を出す方法。

例えば、
「設計書などのドキュメント類」と「そのドキュメントに付随するコード」を別々の PR とする。
「実装部分」と「テスト部分」を別々の PR とする。
など。

それぞれを異なるレビュアーに依頼することで、進行がスムーズになるし、問題があった場合にも修正しやすい。

その他

以下の項目も記載があるが、よく理解できなかったので、詳細はドキュメントを参照ください。

Splitting Horizontally
Splitting Vertically
Splitting Horizontally & Vertically


Separate Out Refactorings(リファクタリングを分離する)

通常、リファクタリングは機能変更やバグ修正とは別の PR で行うのがベストです。

例えば、クラスの移動や名前の変更は、バグ修正とは別の PR で行うべきです。
レビュアーにとっては、各 PR が別々に行われる方が導入された変更を理解しやすくなります。

ただし、ローカル変数名の修正などの小さなクリーンアップであれば、機能変更やバグ修正の PR 内に含めても OK です。

行うリファクタリングの規模が、現在の PR に含めるとレビューが難しくなるかどうかは、開発者とレビュアーの判断に委ねられます。


PR には関連するテストコードを含めるべきです。（別々の PR としないこと。）

ここでの「小ささ」は、PR が「焦点を絞ったものであるべき」という概念的なアイデアを指し、単純な行数に基づくものではありません。

全ての変更に対して、テストが期待されます。

ロジックを追加したり変更したりする PR には、その新しい動作のための「新規または更新されたテスト」が伴うべきです。

純粋なリファクタリング PR（動作を変更する意図がないもの）も、テストでカバーされるべきです。
理想的には、これらのテストは既に存在していますが、存在しない場合は追加すべきです。

独立したテストの修正は、separate-out-refactoringsと同様に、別の PR として OK です。
それには以下が含まれます。

  • 既存の実装済みコードに対して、新しいテストを実施して検証する場合。
    • 既存のロジックが正しく動作しているかをテストによって保証するため。
    • リファクタリング実施の際に、先にテストを追加しておくことで、結果に影響がないリファクタリングとなってるかが確認ができるため。
  • テストコードのリファクタリング（例：ヘルパー関数の導入）。
  • 大規模なテストフレームワークコードの導入（例：統合テスト）。


Don’t Break the Build（ビルドを壊さない）

複数の PR が互いに依存している場合、各 PR を提出した後もシステム全体が正常に動作し続けるようにする必要がある。

そうでないと、PR を提出する間に、何か問題が発生した場合はさらに長い時間、全ての開発者のビルドを壊すことになるかもしれません。


Can’t Make it Small Enough(十分に小さくできない場合)

時々、PR を大きくせざるをえない状況に遭遇することがあります。しかし、これは非常に稀です。
小さな PR を書く習慣がある実装者は、ほとんど常に機能を一連の小さな変更に分解する方法を見つけることができます。

大きな PR を書く前に、それに先立ってリファクタリングのみを行う PR を用意することで、よりクリーンな実装の道を準備できるかどうかを考えてみてください。
チームメイトと話し合い、小さな PR で機能を実装する方法についての意見を聞いてみてください。

これらの選択肢がすべて失敗した場合（非常に稀ですが）、事前にレビュアーから大きな PR をレビューする承諾を得てください。事前にレビュアーに大きな PR について警告しておくべきです。
この状況では、レビュー過程が長引くことを覚悟し、バグを導入しないように注意深く、テストの記述に特に注意を払う必要があります。


How to handle reviewer comments（レビューコメントの扱い方）

あなたが CL をレビューに出した場合、レビュアーがあなたの CL に対していくつかのコメントを返してくる可能性があります。
査読者のコメントを処理するために知っておくと便利なことがいくつかあります。

PR を出すと、レビュアーからのコメントが返ってくることがあります。
レビュワーコメントに対応する時の対処ポイントがいくつかあります。


Don’t Take it Personally（個人の問題として受け止めない）

大前提として、レビュアーの指摘コメントは、
あなた自身やあなたの能力に対する個人的な攻撃ではありません。

あなたの今後のため、PJ のため、ひいてはチームのため、より良くしていこうと考えてしてコメントしてくれていると思い、受け取りましょう。
レビューの目的は、コードベースと製品の品質を維持することです。

あってはならないことですが、ときにはレビュワーがイライラしてコメントに表現することもあるかもしれません。
その場合に備えて実装者は心構えをしておく方が良いでしょう。
「レビュアーが私に伝えようとしている建設的なことは何か？」と自問し、それが実際に言われたことであるかのよう、いいように解釈して受け止めましょう。

コード・レビューのコメントに対して、決して怒りで返答してはいけません
それはプロ意識に欠ける行為です。
また、返答内容はバージョン管理と共に一生公開され続けます。
もし、イライラがおさまらない場合は、別の作業をするかいっそ散歩にでも出かけて気持ちを落ち着かせましょう。

一般的に、レビュアーが建設的で礼儀に欠ける場合は、直接会って説明しましょう。対面が難しいならメールやチャットを送りましょう。何に嫌な思いをしてどう改善して欲しいのかを丁寧に建設的に。
上記の対応でも改善しない場合は上司に相談しましょう。


Fix the Code（コード修正）

レビュワーが「コードに理解できない箇所がある。」とコメントした場合、まず取り組む対応としては、そのコード箇所が明確になるように修正することです。
レビュワーがあなたのコードを理解できないなら、将来コードを読む人も理解できない可能性が高いです。

コードを明確にできない場合は、その理由や背景などをコードコメントとして追加してください。

コードコメントが冗長になる場合や、または説明が難しい場合に限り、PR コメントで説明するようにしてください。
PR コメントに説明を詳細に書いても、将来コードを読む人にはわからない（PR を遡って検索しないとわからない）ため。


Think Collaboratively（協力的に考える）

PR を書くのは大変な作業です。実装して PR を出すと、満足して完了した気になります。これ以上作業することは無いと思ってしまいます。
その状態でレビュワーから指摘が入ると、めんどくさく思ったり、指摘に対して反論して修正を入れなくて済むように反論したくもなります。

ですが指摘が入った際は冷静に、自分やチーム、PJ にとって役立つ貴重なフィードバックが記載されているかどうかを考えましょう。
自分の実装が正しいと思い込まず、まずは「レビュワーの指摘内容が正しいのでは？」と思って確認するようにしましょう。

レビュワーの指摘内容がよくわからない場合は、レビュワーにより詳細に分かりやすくコメント入れてもらうように求めましょう。
また、指摘内容に理解つつも、同意できない場合は、建設的に丁寧にその旨を回答しましょう。

改めて、礼儀と敬意を常に最優先することを忘れないでください。
その上で意見が合わない場合は、お互いの落とし所、同意点を見つけましょう。
明確な説明を求めたり、賛否両論を話し合ったり、実装背景を説明したり、などで。


Resolving Conflicts（意見の対立の解消）

レビュワーとの意見が対立する場合は、まずは、最大限レビュアーと合意を得れるよう試みてください。
コメントでのやり取りだけでなく、通話を設けたりしてのディスカッションなど。

それでも合意が得られない場合、一般的な解決方法として、上司や他のチームメンバー、リーダーなどに意見を聞いた入り、議論に参加してもらうことです。
大事なこととして、実装者とレビュワーが合意できないからといって PR を放置しないでください。


おわり

「実装者ガイド」と「レビュワーガイド」の章があるが、「実装者ガイド」だけでも一読するのにそこそこの量があるため、記事分けました。

「レビュワーガイド」については、また別途記事にまとめる予定です。（仮）

GitHubで編集を提案
NCDCエンジニアブログ
NCDCエンジニアブログ

NCDC株式会社( ncdc.co.jp/ )のエンジニアチームです。 募集中のエンジニアのポジションや、採用している技術スタックの紹介などはこちら( github.com/ncdcdev/recruitment )をご覧ください！ ※エンジニア以外も記事を投稿することがあります

Discussion