トップ > 読書感想文 > Google開発者用のコードレビューガイドの読み合わせ会をしました

Google開発者用のコードレビューガイドの読み合わせ会をしました

読書感想文 プログラミング
記事タイトルとURLをコピーする

こんにちは。AS 部の兼安です。 お盆で実家に帰ったら、学生の頃の C 言語の教科書が出てきました。C 言語のポインタとアドレスの概念は、結局全部アドレス持っているのだから関数だって変数に代入できるよね！などという形で一応今でも役に立っています。

ちなみに、当時私が書いたコードも見つけました。コードレビューしたら確実に弾かれそうです。というわけで（？）今回はコードレビューのお話です！

この記事の内容について

先日、社内で Google の Code Review Developer Guide の読み合わせ会をしました。

google.github.io

本記事は、その時の感想コメントをまとめたものです。ガイドの内容について、大なり小なり刺さった部分の感想を、ページ毎に箇条書きでまとめています。 読み合わせ会の結果、何かの方針が決まったとかそういうことではありませんが、何か考えるキッカケなどになれば幸いです。

なお、このガイドに出てくる CL とは Change List の略で、私達は CL をプルリクエスト（PR）のことだと解釈して読みました。

The Standard of Code Review(コードレビューの基準)

The Standard of Code Reviewより

The primary purpose of code review is to make sure that the overall code health of Google’s code base is improving over time. All of the tools and processes of code review are designed to this end.x

In order to accomplish this, a series of trade-offs have to be balanced.

コードレビューの主な目的は、Googleのコードベースの全体的なコードの健全性が時間とともに向上していることを確認することです。コードレビューのすべてのツールとプロセスは、この目的を達成するために設計されています。

これを実現するためには、一連のトレードオフをバランスよく取る必要があります。
  • バランスが大事、この段落の意味を忘れないようにしたい。

On the other hand, it is the duty of the reviewer to make sure that each CL is of such a quality that the overall code health of their codebase is not decreasing as time goes on.

一方で、レビュアーの責務は、時間の経過とともにそのコードベースの全体的なコードの健全性が低下しないように、各CLが十分な品質を持っていることを確認することです。
  • レビュアーの責務と言われると、ちょっと言葉として強すぎる気がする。責務を意識しすぎて、レビュアーがゲートキーパーになるのは功罪ありそう。

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.

一般的に、レビュアーはCLが完璧でなくとも、作業中のシステムの全体的なコードの健全性を明らかに向上させる状態にある場合、そのCLを承認することを優先すべきです。
  • 大事なことだが、コードに対して熱い思いがあると忘れがちになる。
  • プロジェクトは前に進めないといけないので、仕方なく承認するという状況は存在するだろう。そういう場合でも、今はここまでだが、次はもっと改善したいという意思を持ちたい。

Instead of seeking perfection, what a reviewer should seek is continuous improvement.

レビュアーが追求すべきは、完璧さではなく継続的な改善です。
  • この観点は素晴らしい。
  • 完璧さは追求しなくてもよいが、改善は継続するという意思を示してくれるとレビュアーは承認しやすいかもしれない。

Reviewers should always feel free to leave comments expressing that something could be better, but if it’s not very important, prefix it with something like “Nit: “ to let the author know that it’s just a point of polish that they could choose to ignore.

レビュアーは、何かがより良くできると感じる点について常にコメントを残す自由があります。しかし、それがあまり重要でない場合は、"Nit: "のような接頭辞をつけて、それが磨きの一点であり、無視を選択しても良いことを著者に知らせるようにしてください。
  • この手法を実践してみたくなった。ただし、これを実際に使用する時は、感想と事実と推測は分けて伝えたい。
  • コードレビューもコミュニケーションなので簡単に発言の意図や背景はすれ違ってしまう。提案のニュアンスはわりと大事。
  • コメントに段階があると、指摘コメントを潰すのが大変になりそう。
    • 指摘コメントを潰すという発想自体が違うかもしれない。

What to look for in a code review(コードレビューの観点)

What to look for in a code reviewより

Ask for unit, integration, or end-to-end tests as appropriate for the change. In general, tests should be added in the same CL as the production code unless the CL is handling an emergency.

変更に適切に応じて、ユニットテスト、統合テスト、またはエンドツーエンドテストを要求してください。一般的に、CLが緊急事態を取り扱っていない限り、テストは本番コードと同じCLに追加されるべきです。
  • 頑張ってテスト書きます！

Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing.

通常、コメントはコードが存在する理由を説明するときに役立ち、コードが何をしているのかを説明すべきではありません。
  • コードが何をしているかは、相手もエンジニアなのでわかるはず、何故そうしたか？を伝える努力の方が大事。とはいえ、「何をしているか」が伝わらないコードは、それはそれでおかしいので素直に対応するべきだろう。
  • 著名なエンジニアも同様のことを言っている。

The author of the CL should not include major style changes combined with other changes. It makes it hard to see what is being changed in the CL, makes merges and rollbacks more complex, and causes other problems. For example, if the author wants to reformat the whole file, have them send you just the reformatting as one CL, and then send another CL with their functional changes after that.

CLの作者は、他の変更と一緒に大きなスタイルの変更を含めるべきではありません。これにより、CLで何が変更されているのかを見るのが難しくなり、マージやロールバックがより複雑になり、他の問題も生じます。例えば、作者がファイル全体をリファクタリングしたい場合、リファクタリングだけを1つのCLとして送ってもらい、その後に機能的な変更を含む別のCLを送ってもらうようにしてください。
  • 例えば、バグ FIX とリファクタリングを一つの PR に含めるのは NG 。このような PR を通してしまう習慣ができると、バグ FIX が新たなバグを生むような流れが発生しやすくなるだろう。

Speed of Code Reviews(コードレビューのスピード)

Speed of Code Reviewsより

At Google, we optimize for the speed at which a team of developers can produce a product together, as opposed to optimizing for the speed at which an individual developer can write code. The speed of individual development is important, it’s just not as important as the velocity of the entire team.

Googleでは、個々の開発者がコードを書く速度に注力するのではなく、開発者チーム全体で製品を完成させるまでのリードタイムを最適化しています。個人の開発速度も確かに重要ですが、チーム全体としてのリードタイムの短縮がより優先される点を重視しています。
  • 一般的には適切なレビューができる人はそれなりのスキルを持った人であり、多忙である。レビューイの方が配慮しないとレビュープロセスがうまく回らないのでは？
    • それでもレビュアーにスピードアップを求めないと、権限委譲などのボトルネック解消の策が進まないのでは？
  • 「リードタイム1を重要視している」という観点が新しい発見。製品を作っていると、実際にそれが重要だと感じるタイミングは複数回あった。

How to Write Code Review Comments(コードレビューコメントの書き方)

How to Write Code Review Commentsより

In general, it is important to be courteous and respectful while also being very clear and helpful to the developer whose code you are reviewing.

一般的に、コードをレビューしている開発者に対して、明確かつ有益なフィードバックを提供する際は、礼儀正しく尊重する態度を保つことが重要です。
  • 忘れてはいけない重要なこと。レビューは人と人のコミュニケーションである。

Handling pushback in code reviews(コードレビューにおけるプッシュバックへの対応)

Handling pushback in code reviewsより

In particular, when the reviewer believes their suggestion will improve code health, they should continue to advocate for the change, if they believe the resulting code quality improvement justifies the additional work requested. Improving code health tends to happen in small steps.

特に、レビュアーが提案によりコードがより健全になると考える場合、その変更が追加の労力を正当化するほどの品質向上をもたらすと信じるなら、変更の推進を続けるべきです。コードの品質は、少しずつの改善で向上します。
  • 「労力に見合うだけの価値があるか？」の視点が大事。

Usually, if you are polite in your comments, developers actually don’t become upset at all, and the worry is just in the reviewer’s mind. Upsets are usually more about the way comments are written than about the reviewer’s insistence on code quality.

通常、コメントが丁寧であれば、開発者は実際には怒ることはまったくなく、その心配はレビュアーの心の中だけのことです。不機嫌な反応は、ほとんどの場合、コメントの書き方に関するもので、コードの品質に対するレビュアーの主張に関するものではありません。
  • 人間は感情があるので、お互いが丁寧であるべき

A common source of push back is that developers (understandably) want to get things done. They don’t want to go through another round of review just to get this CL in. So they say they will clean something up in a later CL, and thus you should LGTM this CL now. Some developers are very good about this, and will immediately write a follow-up CL that fixes the issue. However, experience shows that as more time passes after a developer writes the original CL, the less likely this clean up is to happen. In fact, usually unless the developer does the clean up immediately after the present CL, it never happens. This isn’t because developers are irresponsible, but because they have a lot of work to do and the cleanup gets lost or forgotten in the press of other work. Thus, it is usually best to insist that the developer clean up their CL now, before the code is in the codebase and “done.” Letting people “clean things up later” is a common way for codebases to degenerate.

反発の一般的な原因は、開発者が（理解できるように）物事を成し遂げたいと思っていることです。彼らはこのCLを取り込むためだけに再度のレビューを受けたくありません。そこで彼らは、後のCLで何かを整理すると言って、今のCLを承認してもらいたいと言います。一部の開発者はこれに非常に真摯で、問題を修正する追跡CLをすぐに書きます。しかし、経験から言うと、開発者がオリジナルのCLを書いた後、時間が経つにつれて、その整理が行われる可能性は低くなります。実際、開発者が現在のCLの直後にすぐに整理を行わない限り、それは決して実行されません。これは、開発者が無責任だからではなく、彼らが多くの仕事を抱えており、他の仕事のプレッシャーで整理作業が失われたり忘れられたりするためです。したがって、コードがコードベースに取り込まれて「完成」とされる前に、開発者に今すぐそのCLを整理するように求めるのが、通常は最善です。人々に「後で整理する」ことを許すのは、コードベースが退化する一般的な方法です。
  • チケット化しておけば後回しにしておいても良いのでは？
    • チケット化しておいてもそれが PR に至るとは限らないので、やはりすぐ完了させておくのがベストだろう

Writing good CL descriptions(良い CL 説明文の書き方)

Writing good CL descriptionsより

It can be worthwhile to review a CL description before submitting the CL, to ensure that the description still reflects what the CL does.

CLを提出する前に、CLの説明を見直して、その説明がCLの内容を正確に反映しているか確認することは価値があります。
  • 提出前のセルフレビューは開発者のマナー。

Small CLs(小さな CL)

Small CLsより

Keep in mind that although you have been intimately involved with your code from the moment you started to write it, the reviewer often has no context. What seems like an acceptably-sized CL to you might be overwhelming to your reviewer. When in doubt, write CLs that are smaller than you think you need to write. Reviewers rarely complain about getting CLs that are too small.

あなたはコードの記述を開始した瞬間から密接に関わってきましたが、レビュアーはしばしばその背景を持っていないことを念頭に置いてください。あなたにとって適切なサイズのCL（チェンジリスト）に見えるものが、レビュアーにとっては圧倒的に見えることがあります。疑問のある場合は、あなたが書く必要があると思うよりも小さいCLを書いてください。レビュアーは、小さすぎるCLを受け取ることについてはほとんど不満を言わないものです。
  • PR のコメントは、ステートレスなコミュニケーション（＝相手は隙間時間を利用してちょっと見るてるだけ、伝わる情報量も対面より少ない）であることを意識しなければならない。

It’s usually best to do refactorings in a separate CL from feature changes or bug fixes. For example, moving and renaming a class should be in a different CL from fixing a bug in that class. It is much easier for reviewers to understand the changes introduced by each CL when they are separate.

通常、リファクタリングは機能変更やバグ修正とは別のCLで行うのがベストです。例えば、クラスの移動や名前の変更は、そのクラスのバグ修正とは別のCLで行うべきです。レビュアーにとって、それぞれのCLで導入された変更を理解するのは、別々の方がずっと簡単です。
  • 目的が異なるものを一緒にすると、レビューが難しくなる。
  • リファクタリングが新たなバグを生み出す可能性もあるので、バグフィックスを出したら新たな 2 次バグが生まれることに繋がりかねない。
  • 小さな CL にするのは開発者の意識だけでは難しく、設計段階から考慮しないといけないのでは？

Small cleanups such as fixing a local variable name can be included inside of a feature change or bug fix CL, though. It’s up to the judgment of developers and reviewers to decide when a refactoring is so large that it will make the review more difficult if included in your current CL.

ローカル変数名の修正のような小さなクリーンアップは、機能変更やバグ修正のCLに含めることができます。
  • とはいえリファクタリングを混ぜないという話も 1・0 ではなく、柔軟性はあるべき

Sometimes you will encounter situations where it seems like your CL has to be large. This is very rarely true.

時々、CLが大きくなければならないような状況に遭遇することがある。これは非常にまれなことです。
  • 仕方なく PR が大きくなることもあると思う、時間がないと忌憚のないコメントがしづらいので、小さくならないのならせめて早く議論しておきたい。

How to handle reviewer comments(レビューコメントの対応の仕方)

How to handle reviewer commentsより

Never respond in anger to code review comments. That is a serious breach of professional etiquette that will live forever in the code review tool.

怒りに任せたコメントはプロとしての礼儀作法に違反しますし、それがコードレビューツールに永遠に残ることになります。
  • イライラすることがあるのは致し方ない・・・。
  • 怒りをコメントに残してしまうと、後から見直した時にまた嫌な気持ちになるのを覚えておこう。

Remember, courtesy and respect should always be a first priority. If you disagree with the reviewer, find ways to collaborate: ask for clarifications, discuss pros/cons, and provide explanations of why your method of doing things is better for the codebase, users, and/or Google.

Sometimes, you might know something about the users, codebase, or CL that the reviewer doesn’t know. Fix the code where appropriate, and engage your reviewer in discussion, including giving them more context. Usually you can come to some consensus between yourself and the reviewer based on technical facts.

レビュアーと意見が合わない場合は、協力する方法を見つけましょう：説明を求めたり、賛否両論を議論したり、なぜあなたのやり方がコードベースやユーザー、Googleにとってより良いのかの説明を提供したりしましょう。

ユーザーやコードベース、CLについて、レビュアーが知らないことをあなたが知っていることもあります。適切な場合はコードを修正し、レビュアーにはより多くの背景を伝えるなど、議論に参加してもらいましょう。通常、技術的な事実に基づいて、あなたとレビュアーの間で何らかのコンセンサスを得ることができます。
  • 「議論」という言葉が出てくるのが印象的。実際コードの背景一番知っているのは、実装者たるレビューイの方で、レビュアーのできることは議論を提案することだと思う。
  • レビュアーのコメントは命令ではない。

Picking the Best Reviewers(最高のレビュアーを選ぶ)

Code Review Developer Guide の TOP の後半部分より

In general, you want to find the best reviewers you can who are capable of responding to your review within a reasonable period of time.

一般的に、あなたは、妥当な期間内にあなたのレビューに答えることができる、できる限り最高のレビュアーを見つけたいものです。
  • このガイドは高位のエンジニアが潤沢な組織のガイドに見える。通常の組織において、最高のレビュアーにこだわると、一部のエンジニアの負荷が上昇するのでは？
    • 最高のレビュアーと言っているが、高位のエンジニアとは言っていないのがポイントのように思える。
  • とり急ぎこのガイドには、上級者のコードを初心者が学習も兼ねてレビューをするという概念は入っていないようだ。
    • 初心者がどの程度を指すかによるが、「下に合わせる」現象が発生するとよくないので、初心者のことを気にかけ過ぎるのもいかがなものか

全体的な感想

  • PR を出す前からコミュニケーション取っておけば良いのでは？と思うこともある
  • コミュニケーションツールは一つではない、コードに関して PR でしか議論をしてはいけないというルールは存在しない
  • 大きな変更であれば、事前に design doc2や RFC3というプロセスで大枠を固める、コードレビューも同じようなものだと思う
  • PR で細かい質問や議論が繰り返されると、後で読み返した時にノイズになるので、ある程度認識合わせしてから、PR を出した方が良いだろう
  • つまり、根回しは大事
  • 相手の意図は想像するものではなく、確認するもの！

以上です。
コードレビューのやり方について真剣に議論する機会は多くはありませんので、貴重な体験でした。たまにはコードとコードレビューに対する思いを口にし合うのもいいものです。こういう体験が回り回って品質に繋がるかもしれません。

サーバワークスでは、お客様のシステム内製を支援する活動を行なっています。

www.serverworks.co.jp

帰属情報

本記事中で引用されている内容は以下のソースからのものです。

Code Review Developer Guide by Google, licensed under CC BY 3.0.

脚注

  1. 要件が発生してから製品やサービスが完成・提供されるまでの時間。↩
  2. ソフトウェアやシステムの設計を詳細に説明する文書。この文書には、ソフトウェアやシステムのアーキテクチャ、使用する技術やツール、設計の背景や目的、重要なデータフローやインターフェイスの詳細などが含まれます。↩
  3. 「意見を求める」という意味の文書やプロトコルの仕様書。↩

兼安 聡(執筆記事の一覧)

アプリケーションサービス部 DS1課所属
AWS12冠。
広島在住です。
最近認定スクラムマスターになりました。今日も明日も修行中です。