Google Code Review Guidelines

Google积累了很多最佳实践，涉及不同的开发语言、项目，这些文档，将Google工程师多年来积攒的一些最佳实践经验进行了总结并分享给众开发者。学习下这里的经验，我们在进行项目开发、开源协同的过程中，相信也可以从中受益。

Google目前公开的最佳实践相关文档，目前包括：

• Google’s Code Review Guidelines，Google代码review指引，包含以下两个系列的内容：
  • The Code Reviewer’s Guide
  • The Change Author’s Guide

这了涉及到Google内部使用的一些术语，先提下：

• CL: 代表changelist，表示一个提交到VCS的修改，或者等待review的修改，也有组织称之为change或patch；
• LGTM：代表Looks Good to ME，负责代码review的开发者对没有问题的CL进行的评论，表明代码看上去OK；

The Code Reviewer’s Guide

从代码reviewer的角度出发，介绍下Google内部积累的一些good practices。

The Code Reviewer’s Guide

The Change Author’s Guide

这里先介绍下CR过程中应该做什么，或者CR的目标是什么。

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

The Standard of Code Review

代码review的主要目的就是为了保证代码质量、产品质量，另外Google的大部分代码都是内部公开的，一个统一的大仓库，通过代码review的方式来保证未来Google代码仓库的质量，Google设计的代码review工具以及一系列的review规范也都是为了这个目的。

为了实现这个目标，某些方面需要做一些权衡和取舍。

首先，开发者必须能够持续优化。如果开发者从来不对代码做优化，那么最终代码仓库一定会烂掉。如果一个reviewer进行代码review时很难快速投入，如不知道做了哪些变更，那么代码reviewer也会变得很沮丧。这样就不利于整体代码质量的提高。

另外，代码reviewer有责任维护CL中涉及到的修改的质量，要保证代码质量不会出现下降，时间久了也不至于烂尾。有的时候，某些团队可能由于时间有限、赶项目，代码质量就可能会出现一定的下降。

还有，代码reviewer对自己review过的代码拥有owner权限，并要为代码后续出现的问题承担责任。通过这种方式来进一步强化代码质量、一致性、可维护性。

基于上述考虑，Google制定了如下规定来作为Code review的内部标准：

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.

一般，reviewers对CL进行review的时候，达到approved的条件是，CL本身可能不是完美的，但是它至少应保证不会导致系统整体代码质量的下降。

当然，也有一些限制，例如，如果一个CL添加了一个新特性，reviewer目前不想将其添加到系统中，尽管这个CL设计良好、编码良好，reviewer可能也会拒绝掉。

值得一提的是，没有所谓的perfect code，只有better code。

• 代码reviewers应该要求开发者对CL中每一行代码进行精雕细琢，然后再予以通过，这个要求并不过分。
• 或者，代码reviewers需要权衡下他们建议的“精雕细琢”的必要性和重要性。代码reviewers应该追求代码的持续优化，不能一味地追求完美。对于提升系统可维护性、可读性、可理解性的代码CL，reviewers应该尽快给出答复，不能因为一味追求完美主义将其搁置几天或者几周。

What to Look For In a Code Review

结合前面提到的一些Code review标准，将Code review中应该关注的点进一步细化，主要以下内容。

代码规范，这里的规范涵盖了多种编程语言的规范，请确认CL遵循此规范。

如果某些地方没有明确的代码规范指引，而你有觉得开发者这种写法不太好，你想提出点建议的话，review的时候请在意见里面加个前缀“Nit:”，Not Import Pick，以表明这是个非强制性的建议。不要因为个人偏好问题，阻塞CL的代码review过程。

CL里面不要既做代码逻辑修改，又做大范围格式化的操作，这可能让review、merge、rollback等变得复杂，如果确实有必要做这样的调整，请提供两个CL。第一个用来格式化，第二个在此基础上再进行代码逻辑的修改。反过来也可以，但是不要一次做两件“变动巨大”的事情。

Navigating a CL in Review

现在我们知道了What to look for，如果review涉及到多个文件，如何最高效地进行review呢？

• review之前，查看CL的整体表述，确认是否有意义
• 首先，看CL中最有价值的部分，整体设计是否良好
• 然后，再根据合理顺序看CL中剩余的部分

Speed of Code Reviews

What is An Emergency?，区分什么是紧急场景，什么不是，避免滥用。

How to Write Code Review Comments

Handling Pushback in Code Reviews

The Change Author’s Guide

从代码CL开发者的角度出发，介绍下Google内部积累的一些good practices。

Writing Good CL Descriptions

CL描述用来记录做了什么改变、为什么做这个改变，它会作为VCS中的提交历史记录下来，之后可能会有更多的开发者阅读到这里的CL描述。

将来，开发者也可能根据一些关键词来搜索这里的CL描述，如果CL相关的关键信息只记录在代码中，在CL描述中不能予以体现的话，那么别人定位你的CL就会异常困难。

First Line

• 需要对修改进行一个精炼的总结
• 描述应该是一个完整的句子
• 描述后跟一个空行

CL描述的首行应该对做的修改进行一个精炼的总结、概括，然后后面跟一个空行。大部分情况下，开发者查看、检索CL描述信息（log信息）是看的这些内容，所以CL描述的首行信息是至关重要的。

通常，首行信息应该是一个完整的句子，例如，一个好的首行表述：”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

CL描述的剩余部分应该详细一点，可以包含对要解决问题的描述，以及为什么CL中的实现是比较好的或者是最优的方案。如果该方法有什么不足，也应该提一下。如果有一些bug编号、性能结果、设计文档之类的背景信息，也应该包含进来，方便后来者查看。

即使CLs涉及到的改动不多，有必要的话，也需要在body部分描述下。

Bad CL Descriptions

“Fix bug”不是一个足够充分的CL描述，修的什么bug？为了修bug做了哪些修改？其他类似的不良CL表述包括：

• Fix build
• Add patch
• Moving code from A to B
• Add convennience functions
• kill weired URLs

上面这些是Google代码仓库中捞出来的真实CL描述，作者可能认为他们的描述比较清晰了，但是实际上这样的CL描述没有任何意义，完全没有起到CL描述应有的作用。

Good CL Descriptions

下面是一些比较好的CL描述示例。

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.

这个CL描述的首行信息概述了做的修改，后面body部分又解释了为什么做这个修改，以及自己是怎么做的，有什么好处，还提供了实现相关的一些信息。

Refactoring

Construct a Task with a TimeKeeper to use its TimeStr and Now methods.

Add a Now method to Task, so the borglet() getter method can be removed (which was only used by OOMCandidate to call borglet’s Now method). This replaces the methods on Borglet that delegate to a TimeKeeper.

Allowing Tasks to supply Now is a step toward eliminating the dependency on Borglet. Eventually, collaborators that depend on getting Now from the Task should be changed to use a TimeKeeper directly, but this has been an accommodation to refactoring in small steps.

Continuing the long-range goal of refactoring the Borglet Hierarchy.

这个CL描述的首行信息指出了CL做了什么，相对于以前的实现做了哪些修改。body部分介绍了实现细节、CL的context，也指出了虽然这个方案没有那么理想，但是也指出了未来的改进方向。同时也解释了为什么需要做这里的重构。

Small CL that needs some context

Create a Python3 build rule for status.py.

This allows consumers who are already using this as in Python3 to depend on a rule that is next to the original status build rule instead of somewhere in their own tree. It encourages new consumers to use Python3 if they can, instead of Python2, and significantly simplifies some automated build file refactoring tools being worked on currently.

这个CL的首行信息描述了做了什么，body部分解释了为什么要做这里的修改，并且给reviewer提供了充分的context（领域相关知识）信息。

Review the description before submitting the CL

CLs在review过程中可能会再次修改，再次review的时候，CL描述信息可能会与最新的修改不符，在提交CL之前review一下描述信息是否与代码修改仍然一致，这个是有必要的。

Small CLs

Why Write Small CLs?

Small, simple CLs有这样的好处：

• Review更快。如果提交一个大的CL，reviewer可能要专门抽30分钟才能完成review过程，这个可能比较困难，但是如果CL拆的都比较小，reviewer每次抽个5分钟完成一次review，多次review，回比前者更快完成。
• Review更充分。如果提交一个大的CL，改动东西很多情况下，reviewer需要游走在更多代码中，心理上会倾向于关注更加重要的代码，难免某些代码会被降权，review可能不那么充分。如果CL比较小，很容易就能看懂，也不需要来回翻代码，review的会更充分。
• 不易引入bug。因为每次改动都比较小，CL作者不容易引入bug，reviewer也更容易发现bug。
• 如果被拒绝，可减少无谓的工作。CL可能会被拒绝，如果一次做大量修改，被拒绝的话，CL中的所有修改动作可能都要重新来过，不管是合理的、不合理的，相当于做了更多无谓的工作。
• 更易merge。如果提交一个大的CL，涉及修改比较多，冲突可能也会比较多，那么解决冲突花费的时间会更多，不容易merge。小的CL在merge的时候就简单多了。
• 设计不至于出大问题。如果CL涉及修改比较少，那么很容易把修改优化到最佳，对于reviewer的建议也更容易完成，如果CL比较大，reviewer意见、建议比较多，就很难一次完成了。
• 后续工作不至于阻塞在review上。CL作者可能希望基于这个CL做更多工作，如果CL比较小那么review可以很快通过，但是如果CL比较大，那么CL作者将不得不等待更多的时间
• merge后有问题，更易回退。merge之后有时候也会意识到merge的代码有问题，如果要回退的话，小的CL更容易回退，如果CL很大，哇，涉及到的代码多，回退就比较痛苦、复杂。

reviewer不会因为某个CL很大，我不review了，然后给你拒绝掉，通常他们会表现的比较礼貌，告知你将这个大的CL拆分成几个小的CLs。当你已经完成了一个CL时，再将其拆分成几个小的CLs，其实这里的工作量可能不少的，当然和reviewer争辩为什么要求拆分成多个CLs花的时间可能也会很长。最省力的做法就是，一开始就坚持写小的CL，多次CLs。

What is Small?

一般，CL一般建议的大小：

• CL包含最少内容，一次只做一件事情，比如新增feature，CL只包含feature的一个部分，而不是所有的部分。也可以与reviewer进行沟通，确定合适的CL大小，不同的reviewer习惯也不一样，对reviewer而言，CL太大太小都是负担。我们的建议是CL一次只做一件事情。
• reviewer需要看到的任何东西都已经包含在了CL中，这些信息可能位于CL代码中、描述中、已经存在的代码中、reviewer之前review过的CL中。
• 当把这个CL合入之后，系统仍然能够工作的很好。
• CL也不小到隐晦难懂的程度。比如新增了一个API，应该同时包含API的使用示例，这样reviewer能够更好地理解API的使用方式，这样也可以避免引入没有用的API。

也没有硬性的规定指明CL多大才算大，一般100行代码是一个比较合理的CL尺寸，1000行有点太大了，主要还是要看reviewer的态度。CL中涉及到的文件的数量也是CL大小的一个度量维度，如果CL中包含了一个文件，文件修改了200行代码，这个感觉还是OK的，但是如果同样是修改了200行代码但是散落在50个文件中，那CL有点太大了。

体会一下，我们写代码的时候是有了解对应的背景的，但是reviewer可能了解比较少或者完全不知道。一个可接受的CL大小，对reviewer而言是很重要的。如果你不确定reviewer期待的CL大小是怎样的，那就尽量写一个比自己预期中的合理大小更小点的，很少有reviewer会嫌弃CL太小。

When are Large CLs OK?

下面这些场景，如果CL比较大也还OK，不算坏：

• 删除整个文件的内容，或者删除大段内容，可以看做是一行修改，因为它不会花费reviewer太多时间review。
• 有时一个大的CL是有自动化refactor工具生成的，而我们信任这些工具，reviewer的工作就是检查并确认这里的修改没问题。这样的CL很大，但是对reviewer而言仍然不会花太多时间。

Splitting by Files

另一种拆分大的CL的方式是，将修改的文件进行适当分组，并将不同分组的文件做成CL并提交不同的reviewer进行review。

例如：你发送了一个CL修改，同时包含了对protocol buffer文件的修改，另一个CL是业务代码修改但是使用了这里修改后的protocol buffer文件。首先我们要先提交proto文件对应的CL，然后再提交引用它的业务代码CL。虽然提交的时候有先后，但是review过程可以是同时进行的。这么做的同时，最好也知会下reviewer另一个CL中的存在以及与当前CL的关系。

Separate Out Refactorings

一般将CL中的重构之类的工作和其中包含的feature开发、bug修改区分开是比较好的做法。例如，将CL中包含的移动、重命名类名之类的操作单独放在一个CL中，这样reviewer能够更容易理解每个CL中的修改。

不过，一个小的变量名修改的也可以包含在另一个feature change或者bugfix相关的CL中。如果一个CL中包含了一些重构之类的修改，让开发者、reviewer判断这部分重构相关的修改是否对当前CL来说太大了，太大了会让review变得更加困难，开发者应该据此作出一些调整。

Keep related test code in the same CL

避免将相关的测试代码单独放在另一个CL中。测试代码验证代码修改的正确性，所以和代码修改相关的测试代码，应该放置在一个相同的CL中，尽管测试代码增加了修改代码的行数。

不相关的测试代码修改，可以放到另一个CL中，类似于 Separate Out Refactorings， 包含：

• 验证已经存在的、提交过的代码，当前只是新增、完善测试代码；
• 重构测试代码（如引入了新的helper函数）
• 引入了更大的测试框架（如集成测试）

Don’t Break the Build

如果多个CLs互相依赖，需要找一个办法来确保这几个CL每提交一个，系统整体仍然能够正常工作，不能因为提交了一个CL就导致系统构建失败或者工作不正常。比如，可以考虑将几个依赖的小的CL修改合并。

Can’t Make it Small Enough

有时会遇到这种情况，看上去CL会无可避免地变得很大，其实，这种情况几乎是不存在的。只要开发者尝试练习去写小的CLs多次code review，习惯了之后开发者总能找到合适的办法来将一个大的修改分解成几个小的修改。

在开发者动手进行一个很大的CL之前，开发者应考虑下是否先来一次只包含重构（只修改设计，不变动功能）的CL然后在此基础上再做修改，这样是不是会更好一点。如果一个CL中既包含重构代码，又包含逻辑变动代码，这个修改就很重。多数情况下，先来一次只包含重构的CL会为后续的修改扫清障碍，后续的修改会更clean。

如果因为某些客观原因，无法做到上述这些，那就先提前向reviewer说明情况，让reviewer做好心理准备，准备好review一个大的CL，review可能花费比较长的时间，请务必小心引入bug、务必添加好测试用例并通过测试，reviewer在这么大的CL中review的效果可能会大打折扣。

How to Handle Reviewer Comments

当开发者针对CL发起Code review时，reviewer一般会通过comments写一些意见、建议，那开发者该如何处理reviewer comments呢？

Don’t Take it Personally

Code review的初衷是为了维护代码的质量、产品的质量，当一个reviewer对开发者代码写了些意见时，开发者应该将其看作是来自reviewer的帮助，不要将其看做是对开发者个人、个人能力的人身攻击。

有时，reviewers会变得很沮丧，并且可能将沮丧、不满情绪在评论中体现出来。一个好的reviewer是不会这么做的，但是作为一个好的开发者，应该做好面对这种情况的准备。开发者可以反问下自己，当reviewer的评论到底是在描述一个什么代码问题，然后尝试去修改就可以了。

对于别人的review意见，永远不要用愤怒去回应！这违反了Code review或者开源协同的精神，并且这样的负面信息可能会永远留存在review历史中。如果你心里面很愤怒，并且想愤怒地做出回应，请尝试离开你的电脑、键盘，或者先干点别的，直到你内心平静下来再礼貌地做出回应。

一般，如果reviewer回复的评审意见不是建设性的，或者没有礼貌，可以试着向reviewer私下里诉说下你的看法。如果能面对面交流最好，不能就尝试发一封私人邮件，向reviewer说下你不喜欢他的这种review方式，你期望他能做出好的改变。如果对方还是拒绝做出改变，那就不要浪费时间了，升级一下知会你的项目经理。

Fix the Code

如果reviewer说他看不懂你的代码中的某个部分，首先你应该尝试简化这里的代码实现。如果代码已经无法再精简实现，那就在代码中添加合适的注释来解释。如果添加额外的注释没有什么太大意义，只有这种情况下，可以尝试通过code review工具添加一些comments来解释。

如果某个reviewer看不懂这里的代码，那么将来可能其他开发者阅读代码的时候，也是看不懂的。但是通过code review工具添加comments进行解释，对将来的开发者是没有帮助的，但是精简代码实现、代码添加注释是有帮助的。

Think for Yourself

完成一个CL是需要很多工作量的，也需要投入不少精力，开发者发起Code review的时候，内心里面可能都是人为我写的代码是没问题的，简直无懈可击，但是当看到reviewer回复的修改建议时，内心里面可能会有抵触，开发者可能只想让reviewer通过这个CL。而实际上，开发者应该首先思考下，reviewer的意见是不是正确的？

如果你不能回答这个问题，可能需要和reviewer沟通下了解下他评论的具体意思。

如果你仍然坚持认为自己是对的，那就跟reviewer解释下为什么你选择这种方案，这种方案好在哪里？通常，reviewer会提出建议、意见，但是往往也更希望开发者能够自己决策到底哪种方案更好。这个时候可以列举技术细节、事实等任何有助于双方达成一致的信息，促使review顺利完成。

Resolving Conflicts

如果感觉与reviewer之间产生了冲突，先尝试达成一致，如果仍然不能，可以参考下 The Standard of Code Review中列出的一些指引。