Google's Engineering Practices documentationはGoogleのコードレビュープロセスとポリシーの標準的な説明のドキュメントです。
https://google.github.io/eng-practices/review/
開発の仕事する中でコードレビューにかける時間は長いですよね？でも雰囲気でレビューしていませんか？
何をどのようにコードレビューするのかは、ちゃんと知っておきたい部分でもあり、チームで共有したいものでもあります。
いろいろ書いてあるのですが、個人的に刺さる部分を書いていきます。なので、結構端折ってます。意味違うのではみたいなのがあれば修正するので教えて下さい。 
以下のような構成になっており、今回はコードレビューの方法の コードレビューの見方(Navigating a CL in Review)まで見ていきます。
- コードレビューの方法
-
コードレビューの基準(The Standard of Code Review) -
コードレビューで何を探すか(What to Look For In a Code Review)
-
コードレビューの見方(Navigating a CL in Review)
- Speed of Code Reviews
- How to Write Code Review Comments
- Handling Pushback in Code Reviews
-
- CL作成者のガイド
- Writing Good CL Descriptions
- Small CLs
- How to Handle Reviewer Comments
頻繁に出てくるCL = Change List = ほぼPull requestと同じもののようです。
コードレビューの基準
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.
That is the senior principle among all of the code review guidelines.
CLが完璧でなくても、その変更がシステムの全体的なコードの健康状態を確実に改善する状態になれば、レビュアーは承認を優先する。
これはコードレビューの全ガイドラインの上位の原則。 (この理由についてはドキュメントに書いてありますが、端折っています。)
There are limitations to this, of course. For example, if a CL adds a feature that the reviewer doesn’t want in their system, then the reviewer can certainly deny approval even if the code is well-designed.
ただこれには制限があり、例えば、よく設計されていたとしても、レビュワーがシステムに入れたくない機能の場合は拒否できる。
Instead of seeking perfection, what a reviewer should seek is continuous improvement. A CL that, as a whole, improves the maintainability, readability, and understandability of the system shouldn’t be delayed for days or weeks because it isn’t “perfect.”
レビュワーが求めるべきものは完璧ではなく、継続的な改善。"完全"ではないという理由で全体として保守性、可読性、理解性を高めるCLを数日または数週間遅らせるべきではない。
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: "のようなプレフィックスをつけて、それがただ改善するポイントであることを知らせ、無視することを選べるようにする。
メンタリング
Sharing knowledge is part of improving the code health of a system over time.
知識の共有は時間の経過とともにシステムの健康状態を改善する一部。
Just keep in mind that if your comment is purely educational, but not critical to meeting the standards described in this document, prefix it with “Nit: “ or otherwise indicate that it’s not mandatory for the author to resolve it in this CL.
教育的だが、このドキュメントの"コードレビューの基準"的に重要ではない場合は"Nit:"をつけるなどしてこのCLで解決する必要がないことを示す。
原則
Technical facts and data overrule opinions and personal preferences.
技術的事実とデータは意見や個人的な好みを覆す。
On matters of style, the style guide is the absolute authority. Any purely style point (whitespace, etc.) that is not in the style guide is a matter of personal preference. The style should be consistent with what is there. If there is no previous style, accept the author’s.
コードスタイルについてはスタイルガイドが絶対的な権威だが、載っていないものはコードを書いた人に従う。
Aspects of software design are almost never a pure style issue or just a personal preference. They are based on underlying principles and should be weighed on those principles, not simply by personal opinion. Sometimes there are a few valid options. If the author can demonstrate (either through data or based on solid engineering principles) that several approaches are equally valid, then the reviewer should accept the preference of the author. Otherwise the choice is dictated by standard principles of software design.
ソフトウェア設計の側面はコードスタイルや個人的な好みではない。個人的な意見ではなく、原則に基づいて検討する。同等な場合は著者を優先するが、そうでなければ基本的には原則によって決まる。
If no other rule applies, then the reviewer may ask the author to be consistent with what is in the current codebase, as long as that doesn’t worsen the overall code health of the system.
他のルールが適応されない場合で、コードの健康状態を悪くしない場合は、レビュワーが現在のコードベースにあるものと同じように書くことを依頼できる。
対立の解消
In any conflict on a code review, the first step should always be for the developer and reviewer to try to come to consensus, based on the contents of this document and the other documents in The CL Author’s Guide and this Reviewer Guide.
コードレビューで対立が起こった場合、最初のステップはドキュメントとCL作成者のガイドとCLレビュワーのガイドに基づいて合意すること。
https://google.github.io/eng-practices/review/developer/
https://google.github.io/eng-practices/review/reviewer/
When coming to consensus becomes especially difficult, it can help to have a face-to-face meeting or a video conference between the reviewer and the author, instead of just trying to resolve the conflict through code review comments. (If you do this, though, make sure to record the results of the discussion as a comment on the CL, for future readers.)
難しい場合は、コメントで解決するのではなく、対面やビデオ会議が役立つ。
If that doesn’t resolve the situation, the most common way to resolve it would be to escalate. Often the escalation path is to a broader team discussion, having a Technical Lead weigh in, asking for a decision from a maintainer of the code, or asking an Eng Manager to help out. Don’t let a CL sit around because the author and the reviewer can’t come to an agreement.
それでも難しい場合はエスカレする。 合意できないからといってCLをそのままにしない。
コードレビューで何を探すか
設計
The most important thing to cover in a review is the overall design of the CL. Do the interactions of various pieces of code in the CL make sense? Does this change belong in your codebase, or in a library? Does it integrate well with the rest of your system? Is now a good time to add this functionality?
レビューでカバーするべき最も重要なものはCLの全体的な設計。コードの相互作用に意味があるか？コードがコードベースに有るのかライブラリにあるのか？他の部分とうまく統合されるか？この機能を追加する良い機会か？
機能性
Does this CL do what the developer intended? Is what the developer intended good for the users of this code? The “users” are usually both end-users (when they are affected by the change) and developers (who will have to “use” this code in the future).
開発者の意図したことを実行しているか？このコードの"ユーザー"にとって、開発者の意図は良いものか？この"ユーザー"とは通常、実際に利用するエンドユーザー(影響するのであれば)とコードを使う開発者両方を指す。
Mostly, we expect developers to test CLs well-enough that they work correctly by the time they get to code review. However, as the reviewer you should still be thinking about edge cases, looking for concurrency problems, trying to think like a user, and making sure that there are no bugs that you see just by reading the code.
開発者としては正しく動くように十分にテストすることを期待するが、
レビュワーとしてはエッジケースを考え、非同期処理について考え、ユーザーのように考え、読んだだけでわかるバグがないことを確認する必要がある。
You can validate the CL if you want—the time when it’s most important for a reviewer to check a CL’s behavior is when it has a user-facing impact, such as a UI change. It’s hard to understand how some changes will impact a user when you’re just reading the code. For changes like that, you can have the developer give you a demo of the functionality if it’s too inconvenient to patch in the CL and try it yourself.
UIの変更でどんな影響がユーザーに与えられるのかを確認するときに変更の動作を確認するのは大切。自分でパッチを当てて確認するのが面倒な場合はデモを見せてもらうことができる。
Another time when it’s particularly important to think about functionality during a code review is if there is some sort of parallel programming going on in the CL that could theoretically cause deadlocks or race conditions. These sorts of issues are very hard to detect by just running the code and usually need somebody (both the developer and the reviewer) to think through them carefully to be sure that problems aren’t being introduced. (Note that this is also a good reason not to use concurrency models where race conditions or deadlocks are possible—it can make it very complex to do code reviews or understand the code.)
デッドロックやレースコンディションを起こし得るときに特に機能性について考えることが重要。この問題は発見するのが非常に難しい。そのため、デッドロックやレースコンディションが起こり得ない非同期モデルを使う良い理由にもなる。
複雑さ
Is the CL more complex than it should be? Check this at every level of the CL—are individual lines too complex? Are functions too complex? Are classes too complex? “Too complex” usually means “can’t be understood quickly by code readers.” It can also mean “developers are likely to introduce bugs when they try to call or modify this code.”
関数、クラス、すべてのレベルで複雑さを確認する。"複雑すぎる"の意味は通常コードを読む人がすぐに理解できないことを指す。そしてそれはコードを呼び出したり変更するときにバグを仕込みやすいということも意味する。
A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn’t presently needed by the system. Reviewers should be especially vigilant about over-engineering. Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future. The future problem should be solved once it arrives and you can see its actual shape and requirements in the physical universe.
レビュワーはオーバーエンジニアリングについて特に注意する必要がある。レビュワーは将来の問題ではなく、今解決しなくてはならない問題を解決するように推奨しよう。未来の問題は現実になってから解決すべき。
テスト
Tests
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.
変更に応じて、ユニットテスト、統合テスト、またはE2Eテストを要求してください。一般的に、テストは緊急事態を処理している場合を除き、プロダクションのコードと同じCLに追加されるべき。
Tests do not test themselves, and we rarely write tests for our tests—a human must ensure that tests are valid.
テストのためにテストを書くことはまれなので、人間がテストの有効性を確かめる必要がある。
Will the tests actually fail when the code is broken? If the code changes beneath them, will they start producing false positives? Does each test make simple and useful assertions? Are the tests separated appropriately between different test methods?
コードが壊れたときにテストが失敗するか？コードが変更されたときにコードは正しいのにテストが壊れるないか？それぞれのテストはシンプルで有用なアサーションをしているか？テストのメソッド間でテストがちゃんと分離されているか？
Remember that tests are also code that has to be maintained. Don’t accept complexity in tests just because they aren’t part of the main binary.
テストは保守するコードなので、メインのバイナリに入らなくても複雑さを受け入れないように。
ネーミング
Did the developer pick good names for everything? A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read.
いい名前はそのアイテムが何をしているのか伝えるのに十分な長さで、読みにくくなるほど長くはない。
コメント
Usually comments are useful when they explain why some code exists, and should not be explaining what some code is doing. If the code isn’t clear enough to explain itself, then the code should be made simpler. There are some exceptions (regular expressions and complex algorithms often benefit greatly from comments that explain what they’re doing, for example) but mostly comments are for information that the code itself can’t possibly contain, like the reasoning behind a decision.
通常コメントにはコードが存在する理由を書き、コードが何をしているのかを書かない。もしコードが明白でない場合はコードをシンプルにする。正規表現や複雑なアルゴリズムなど例外はあるが、殆どのコメントはコードに書けない背景にある理由などを書くべき。
Note that comments are different from documentation of classes, modules, or functions, which should instead express the purpose of a piece of code, how it should be used, and how it behaves when used.
コメントとクラスなどにつけるドキュメントとは違うので注意。クラスなどにつけるドキュメントはコードの目的、使用方法、使用したときの動作を書く。
スタイル
We have style guides at Google for all of our major languages, and even for most of the minor languages. Make sure the CL follows the appropriate style guides.
スタイルガイドに則る。
If you want to improve some style point that isn’t in the style guide, prefix your comment with “Nit:” to let the developer know that it’s a nitpick that you think would improve the code but isn’t mandatory. Don’t block CLs from being submitted based only on personal style preferences.
スタイルについて指摘するときは"Nit:"をつけて修正が義務ではないことを示す。個人的なスタイルの好みによってブロックしない。
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.
スタイルの変更と主な変更を一緒にするべきではない。そのため例えばフォーマットの変更を行ってから、機能の変更を行うなど分けて行うように依頼する。
一貫性
What if the existing code is inconsistent with the style guide? Per our code review principles, the style guide is the absolute authority: if something is required by the style guide, the CL should follow the guidelines.
スタイルガイドと一貫性が矛盾した場合はスタイルガイドに従う。
ドキュメンテーション
If a CL changes how users build, test, interact with, or release code, check to see that it also updates associated documentation, including READMEs, g3doc pages, and any generated reference docs. If the CL deletes or deprecates code, consider whether the documentation should also be deleted. If documentation is missing, ask for it.
変更に伴って更新しよう。更新するように訪ねよう。
すべての行
you should at least be sure that you understand what all the code is doing.
すくなくともコードが何をしているのかを理解している必要がある。
If it’s too hard for you to read the code and this is slowing down the review, then you should let the developer know that and wait for them to clarify it before you try to review it. At Google, we hire great software engineers, and you are one of them. If you can’t understand the code, it’s very likely that other developers won’t either. So you’re also helping future developers understand this code, when you ask the developer to clarify it.
コードが難しすぎる場合は知らせて、わかりやすくするのを待つこともできる。他の開発者のためになる可能性がある。
If you understand the code but you don’t feel qualified to do some part of the review, make sure there is a reviewer on the CL who is qualified, particularly for complex issues such as security, concurrency, accessibility, internationalization, etc.
一部のコードに関してレビューする資格がないと感じたときは資格がある人をアサインする。
文脈(Context)
It is often helpful to look at the CL in a broad context. Usually the code review tool will only show you a few lines of code around the parts that are being changed. Sometimes you have to look at the whole file to be sure that the change actually makes sense. For example, you might see only four new lines being added, but when you look at the whole file, you see those four lines are in a 50-line method that now really needs to be broken up into smaller methods.
文脈を広げて見てみると良い。例えば4行を見ているが、50行のメソッドにいるとき、メソッドを分割したほうが良いなど。
It’s also useful to think about the CL in the context of the system as a whole. Is this CL improving the code health of the system or is it making the whole system more complex, less tested, etc.? Don’t accept CLs that degrade the code health of the system. Most systems become complex through many small changes that add up, so it’s important to prevent even small complexities in new changes.
またシステム全体で見ることも良い。コードの状態を改善するか？より複雑にしてテストを減らしたりするか？コードの健康状態を悪化させる変更を受け入れないでください。ほとんどのシステムは小さな変更の追加で複雑になる。
良いこと
If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way. Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well. It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
CL内で良いポイントを見つけたら開発者に教える。指摘に焦点が当たるが、Good Placticeに対する励ましと感謝も必要。メンタリングの観点から開発者に何が間違っているかを伝えるよりも何が正しいのかを伝えるほうが価値がある場合がある。
まとめ
レビューするすべてのコードを確認し、文脈を見て、コードの健康状態を向上させることを確認し、良いことを称賛しよう。
コードレビューの見方
概要
Now that you know what to look for, what’s the most efficient way to manage a review that’s spread across multiple files?
Does the change make sense? Does it have a good description?
Look at the most important part of the change first. Is it well-designed overall?
Look at the rest of the CL in an appropriate sequence.
複数ファイルあるときに効率的に見るにはどうすればよいか？
- ステップ1: 変更に意味があるか、良いdescriptionがあるか。
- ステップ2: 最も重要な部分を見る。全体的な設計がよくされているか？
- ステップ3: 残りの変更を見る。
ステップ1 変更の全体像を把握する
When you reject a change like this, it’s also a good idea to suggest to the developer what they should have done instead.
For example, you might say “Looks like you put some good work into this, thanks! However, we’re actually going in the direction of removing the FooWidget system that you’re modifying here, and so we don’t want to make any new modifications to it right now. How about instead you refactor our new BarWidget class?”
もし変更を拒否する場合は別の変更をするのをおすすめすると良いかも。例えば"変更ありがとう！今はFooWidgetシステムを消す方向に進んでいるから、代わりにBarWidgetシステムをリファクタリングするのはどう？"みたいにいう。
Note that not only did the reviewer reject the current CL and provide an alternative suggestion, but they did it courteously. This kind of courtesy is important because we want to show that we respect each other as developers even when we disagree.
開発者として尊重していることを示すようにする。
ステップ2 主要部分を調べる
Look at these major parts first. This helps give context to all of the smaller parts of the CL, and generally accelerates doing the code review. If the CL is too large for you to figure out which parts are the major parts, ask the developer what you should look at first, or ask them to split up the CL into multiple CLs.
はじめに主要部分を見る。それによって細かい部分の文脈が分かり、コードレビューを加速させる。 大きすぎる場合ははじめに見る場所を聞くか、CLを分割するように言ってください。
If you see some major design problems with this part of the CL, you should send those comments immediately, even if you don’t have time to review the rest of the CL right now. In fact, reviewing the rest of the CL might be a waste of time, because if the design problems are significant enough, a lot of the other code under review is going to disappear and not matter anyway.
There are two major reasons it’s so important to send these major design comments out immediately:
Developers often mail a CL and then immediately start new work based on that CL while they wait for review. If there are major design problems in the CL you’re reviewing, they’re also going to have to re-work their later CL. You want to catch them before they’ve done too much extra work on top of the problematic design.
Major design changes take longer to do than small changes. Developers nearly all have deadlines; in order to make those deadlines and still have quality code in the codebase, the developer needs to start on any major re-work of the CL as soon as possible.
大きな設計の問題を見つけたらそれをすぐにコメントすること。他の部分を見たとしても変更される可能性がある。また開発者はこの設計を元に他の開発をしてしまっている場合もある。設計の変更は大きな作業であるため開発者の再作業をできるだけ早くするためにすぐにコメントする。
ステップ3：CLの残りの部分を確認
Once you’ve confirmed there are no major design problems with the CL as a whole, try to figure out a logical sequence to look through the files while also making sure you don’t miss reviewing any file. Usually after you’ve looked through the major files, it’s simplest to just go through each file in the order that the code review tool presents them to you. Sometimes it’s also helpful to read the tests first before you read the main code, because then you have an idea of what the change is supposed to be doing.
レビューツールが表示する順番などで順番に見ていく。
テストコードを先に読むと変更によって何が行われるのかがわかるので役立つ場合がある。
続きについて
次はコードレビューの速度というめっちゃ大切なところですが、一旦ここまで読みました。
続きのドキュメント (後で読みます)
https://google.github.io/eng-practices/review/reviewer/speed.html