codereview

发表于 更新于 分类于 Waline：

就在大家 happy 的这几天期间，Google 公开了两个文档，我感觉作为程序员和工程师非常有必要了解一下。作者在阅读之后截取部分原文并精简了其他一些部分总结下来。

The CL author’s guide to getting through code review

简单来讲就是 CL 作者指南去指导通过代码审查。那么 CL 是什么意思呢？就是 change list ，变更记录和日志的意思。

这个指南可帮助您更快地完成代码审核并获得更高质量的结果。适用于每个 Google 开发人员，阅读这份指南非常有帮助。

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

Writing good CL descriptions

First Line

  • Short summary of what is being done.
  • Complete sentence, written as though it was an order.
  • Follow by empty line.

首先，CL的第一行最好是一个做了什么的简短的描述，并且后面跟着一个空行，其次应该将其写成命令式的，当然接下来的部分可以没有必要写成这样。

Body is informative

其余的描述应提供信息。它可能包括对要解决的问题的简要说明，以及为什么这是最好的方法。如果该方法有任何缺点，则应予以提及。如果相关，请包括背景信息，例如错误号，基准测试结果以及设计文档的链接.。

Bad CL Description

“Fix bug” is an inadequate CL description. What bug? What did you do to fix it? Other similarly bad descriptions include:

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

Some of those are real CL descriptions. Their authors may believe they are providing useful information, but they are not serving the purpose of a CL description.

Good CL Descriptions

Here are some examples of good descriptions.

Functionality change

rpc: remove size limit on RPC server message freelist.

Servers like FizzBuzz have very large messages and would benefit from reuse. Make the freelist larger, and add a goroutine that frees the freelist entries slowly over time, so that idle servers eventually release all freelist entries.

The first few words describe what the CL actually does. The rest of the description talks about the problem being solved, why this is a good solution, and a bit more information about the specific implementation.

文档中还有很多好的CL的举例说明，大家可以自己去看一下。同时记住，CL最好是全英的。

Small CLs

Why write small CLs?

  • Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small CLs than to set aside a 30 minute block to review one large CL.
  • Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
  • Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the CL and see if a bug has been introduced.
  • Less wasted work if they are rejected. If you write a huge CL and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.
  • Easier to merge. Working on a large CL takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
  • Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
  • Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current CL in review.
  • Simpler to roll back. A large CL will more likely touch files that get updated between the initial CL submission and a rollback CL, complicating the rollback (the intermediate CLs will probably need to be rolled back too).

简单来讲，就是小步提交可以更容易地知道你每一次提交在做什么，这样一来，当你有什么问题的时候，可以非常快速地定位问题，如果该问题需要回滚时，也不会对现有代码产生什么影响。

What is small?

In general, the right size for a CL is one self-contained change. This means that:

  • The CL makes a minimal change that addresses just one thing. This is usually just one part of a feature, rather than a whole feature at once. In general it’s better to err on the side of writing CLs that are too small vs. CLs that are too large. Work with your reviewer to find out what an acceptable size is.
  • Everything the reviewer needs to understand about the CL (except future development) is in the CL, the CL’s description, the existing codebase, or a CL they’ve already reviewed.
  • The system will continue to work well for its users and for the developers after the CL is checked in.
  • The CL is not so small that its implications are difficult to understand. If you add a new API, you should include a usage of the API in the same CL so that reviewers can better understand how the API will be used. This also prevents checking in unused APIs.

对于每次提交的大小，每次100行左右的是一个比较合理的大小，而1000行就太大了。但这也不是绝对的判断标准，如果100行代码分布在50个文件中，那也算一次很大的修改。

When are large CLs okay?

There are a few situations in which large changes aren’t as bad:

  • You can usually count deletion of an entire file as being just one line of change, because it doesn’t take the reviewer very long to review.
  • Sometimes a large CL has been generated by an automatic refactoring tool that you trust completely, and the reviewer’s job is just to sanity check and say that they really do want the change. These CLs can be larger, although some of the caveats from above (such as merging and testing) still apply.
Splitting by Files

可以将需要不同reviewer的相对独立的更改拆分为不同的CL

Separate Out Refactoring

通常最好是在单独的CL中进行重构，而不包含功能更改或错误修复。例如，修改某个类以及调用该类应该放到不同的两个CL中。

尽量不要把测试代码放到不同的CL中，因为测试会保证你的这段代码的正确性，即使测试代码可能会使你的提交变得庞大。

但是，我们可以将独立的测试放到单独的CL中，比如：

  • validating pre-existing, submitted code with new tests.
  • refactoring the test code (e.g. introduce helper functions).
  • introducing larger test framework code (e.g. an integration test).
Don’t break the build

如果多个CL会相互依赖，最好是能够保证在每个CL被提交修改之后，系统能够正常运行。

Can’t Make it small enough

How to handle reviewer comments

When you’ve sent a CL out for review, it’s likely that your reviewer will respond with several comments on your CL. Here are some useful things to know about handling reviewer comments.

Don’t take it personally

The goal of review is to maintain the quality of our codebase and our products. When a reviewer provides a critique of your code, think of it as their attempt to help you, the codebase, and Google, rather than as a personal attack on you or your abilities.

也就是说，尽量不要再代码审查中加入自己的负面情绪，如果有人在你的代码审查过程中加入了负面情绪，首先考虑一下他们真正想说的是什么，然后再想办法和他们私下交流，比如面谈或者邮件。

Fix the code

如果审阅者说不明白你的代码，那你应该想办法解释清楚你的代码在做什么。因为你的审阅者看不懂，将来其他的同事也可能看不懂。

Think for yourself

因为我们每次提交可能是花费了大量的时间，所以当我们看到审阅者给我们提出意见的时候，我们的第一反应是拒绝。

但是我们最好先考虑一下，审阅者的意见是否是正确的，如果我们没办法回答这个问题，我们就可能需要去找到审阅者让他说清楚自己的观点，然后再去判断。

Resolving conflicts

Your first step in resolving conflicts should always be to try to come to consensus with your reviewer. If you can’t achieve consensus, see The Standard of Code Review, which gives principles to follow in such a situation.

How to do a code review

这份文档中包含有关进行代码审查的最佳方式的建议。是一个非常完整的文档，分为许多单独的部分。阅读这个文档，对于大家来看，肯定会非常有帮助，绝对让大家受益匪浅的一件事。

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

The standard of code review

代码审查的主要目的是确保Google代码库的整体代码运行状况随着时间的推移而不断改善。

首先，开发人员必须能够在其任务上取得进展。如果您从未向代码库提交过改进，那么代码库将永远不会得到改进。另外，如果审阅者很难进行任何更改，那么开发人员就没有动力在将来进行改进。

另一方面，审阅者有责任确保每个CL的质量都使得其代码库的整体代码运行状况不会随着时间的流逝而减少。

Thus, we get the following rule as the standard we expect in code reviews:

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.

Mentoring

代码审查具有重要的功能，可以教给开发人员有关语言，框架或通用软件设计原理的新知识。留下有助于开发人员学习新知识的评论总是很好的。共享知识是随着时间的推移改善系统代码运行状况的一部分。

Principles

  • 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.

What to look for in a code review

Design

审查中最重要的内容是CL的总体设计。CL中各种代码的交互是否有意义？

Functionality

此CL是否达到开发人员的预期目的？开发人员打算为该代码的用户带来什么好处？

“用户”通常既是最终用户（当他们受到更改影响时）又是开发人员（将来他们将不得不“使用”此代码）。通常，我们希望开发人员能够对CL进行良好的测试，以确保它们在进行代码审查时能够正常工作。

但是，作为审阅者，您仍然应该考虑边缘情况，寻找并发性问题，尝试像用户一样思考，并确保没有仅仅通过阅读代码就能看到的错误。

您可以根据需要验证CL，对于审阅者而言，检查CL的行为最重要的时间是对用户的影响（如UI更改）。

当您仅阅读代码时，很难理解某些更改将如何影响用户。进行此类更改时，如果过于麻烦以致无法在CL中打补丁并自己尝试，则可以让开发人员向您演示该功能。

另外一次在代码审查期间考虑功能特别重要的时候是，CL中是否正在进行某种并行编程，从理论上讲可能会导致死锁或竞争状况。仅通过运行代码很难检测到这类问题，通常需要有人（开发人员和审阅者）仔细考虑它们，以确保不会引入问题。

Complex

简单来说就是不要过度设计。

Tests

根据更改要求进行单元测试，集成测试或端到端测试。通常，除非CL处理紧急情况，否则应在与生产代码相同的CL中添加测试。确保CL中的测试正确，合理且有用。测试不会自我测试，我们很少为测试编写测试-人员必须确保测试有效。

Naming

名字要足够长，直到他可以完全表达清楚它是什么或者它想做什么。

Comments

开发人员是否用可理解的英语写下清晰的评论？所有评论实际上都是必要的吗？通常，当注释解释了为什么存在某些代码，而不应该解释某些代码在做什么时，它们很有用。如果代码不够清晰，无法说明自身，则应简化代码。有一些例外情况（例如，正则表达式和复杂算法通常会从注释中解释它们的作用而受益匪浅），但大多数注释是针对代码本身可能无法包含的信息，例如决策背后的原因。

Style

确保CL遵循适当的样式指南。如果您想改善样式指南中没有的样式点，请在注释前面加上“ Nit：”，以使开发人员知道这是您认为可以改善代码但不是强制性的选择。

CL的作者不应将主要样式更改与其他更改结合在一起。这使得很难看到CL中的更改，使合并和回滚更加复杂，并导致其他问题。

Documentation

如果CL改变了用户构建，测试，与代码交互或释放代码的方式，请检查其是否还更新了相关文档，包括自述文件

Every Line

查看已分配给您检查的每一行代码。有时您可以扫描数据文件，生成的代码或大型数据结构之类的东西，但不扫描人工编写的类，函数或代码块，并认为其中的内容是可以的。显然，某些代码比其他代码更需要仔细检查-这是您必须做出的判断调用-但您至少应确保您了解所有代码在做什么。如果您阅读代码太困难，并且使审查速度变慢，那么您应该让开发人员知道这一点，并等待他们澄清，然后再尝试审查

Context

通常，代码查看工具只会向您显示围绕要更改的部分的几行代码。有时，您必须查看整个文件，以确保更改确实有意义。

Good Things

如果您在CL中看到不错的东西，请告诉开发人员，尤其是当他们以出色的方式回答了您的评论之一时。

Summary

In doing a code review, you should make sure that:

  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any UI changes are sensible and look good.
  • Any parallel programming is done safely.
  • The code isn’t more complex than it needs to be.
  • The developer isn’t implementing things they might need in the future but don’t know they need now.
  • Code has appropriate unit tests.
  • Tests are well-designed.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (generally in g3doc).
  • The code conforms to our style guides.

Now that you know what to look for, what’s the most efficient way to manage a review that’s spread across multiple files?

  1. Does the change make sense? Does it have a good description?
  2. Look at the most important part of the change first. Is it well-designed overall?
  3. Look at the rest of the CL in an appropriate sequence.

Speed of Code Reviews

Why should code reviews be fase?

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.

When code reviews are slow, several things happen:

  • The velocity of the team as a whole is decreased. Yes, the individual, who doesn’t respond quickly to the review, gets other work done. However, new features and bug fixes for the rest of the team are delayed by days, weeks, or months as each CL waits for review and re-review.
  • Developers start to protest the code review process. If a reviewer only responds every few days, but requests major changes to the CL each time, that can be frustrating and difficult for developers. Often, this is expressed as complaints about how “strict” the reviewer is being. If the reviewer requests the same substantial changes (changes which really do improve code health) but responds quickly every time the developer makes an update, the complaints tend to disappear. Most complaints about the code review process are actually resolved by making the process faster.
  • Code health can be impacted. When reviews are slow, there is increased pressure to allow developers to submit CLs that are not as good as they could be. Slow reviews also discourage code cleanups, refactorings, and further improvements to existing CLs.

How to write code revieww comment

  • Be kind.
  • Explain your reasoning.
  • Balance giving explicit directions with just pointing out problems and letting the developer decide.
  • Encourage developers to simplify code or add code comments instead of just explaining the complexity to you.