Code Review应该Review什么

注意:在考虑这些要点时，一定要考虑到Code Review标准

设计

在评审中最重要的是CL的整体设计。CL中不同代码段之间的交互有意义吗?这个更改是属于您的代码基，还是属于库?它能很好地与系统的其他部分集成吗?现在是添加此功能的好时机吗?

功能性

这个CL是否达到了开发人员的目的?开发人员想要为这段代码的用户提供什么?“用户”通常是终端用户(当他们受到更改的影响时)和开发人员(将来必须“使用”这些代码)。

大多数情况下，我们希望开发人员能够很好地测试CL，以便在进行代码评审时能够正确地工作。但是，作为评审员，您仍然应该考虑边缘情况，寻找并发性问题，尝试像用户一样思考，并确保通过阅读代码不会看到任何bug。

如果需要，您可以验证CL—当CL具有面向用户的影响(例如UI更改)时，检查CL的行为才是最重要的。当您仅仅阅读代码时，很难理解一些更改将如何影响用户。对于这样的更改，如果在CL中进行修补太不方便，您可以让开发人员给您一个功能演示，然后自己尝试一下。

在代码评审期间考虑功能特别重要的另一个问题是，在CL中是否存在某种并行编程，这种并行编程理论上可能会导致死锁或竞争条件。这类问题很难通过运行代码来发现，通常需要有人(开发人员和审查人员)仔细考虑这些问题，以确保没有引入问题。(注意，这也是一个不使用可能存在竞争条件或死锁的并发模型的好理由——这会使代码评审或理解代码变得非常复杂。)

复杂性

CL比它应该的实现复杂吗?在CL的每一层都检查复杂性--某一行代码是不是太复杂了?函数是不是太复杂了吗?类太复杂了吗?“太复杂”通常意味着“代码读者不能很快理解”。这也意味着“当开发人员试图调用或修改这段代码时，他们可能会引入bug。”

一种特殊类型的复杂性是过度设计，开发人员使代码比需要的更通用，或者增加了系统目前不需要的功能。评审员应该特别警惕过度设计。鼓励开发人员解决他们知道现在需要解决的问题，而不是开发人员推测将来可能需要解决的问题。未来的问题应该在它到来时解决，你可以看到它与需求相符的实际的样子。

测试

要求为变更进行单元测试、集成测试或端到端测试。通常，除非CL正在处理紧急情况，否则应该在与生产代码一样为CL添加测试。

确保CL中的测试是正确的、合理的和有用的。测试本身不进行测试，而且我们很少为我们的测试编写测试——我们必须确保测试是有效的。

当代码被破坏时，测试实际上会失败吗?如果下面的代码发生变化，它们会开始产生主动的错误信息吗?每个测试都做出简单而有用的断言吗?测试是否在不同的测试方法之间适当地隔离吗?

请记住，测试也是必须维护的代码。不要仅仅因为它们不是主要的部分就接受测试中的复杂性。

命名

开发人员是否为每样东西都选择了好名字?一个好的名字足够长，可以完整地传达物品是什么或做了什么，但又不会长到让人难以阅读。

开发人员是否用可理解的语言写出了清晰的注释?所有的注释都是必要的吗?通常，注释在解释某些代码为什么存在时是有用的，而不应该解释某些代码在做什么。如果代码不够清晰，无法解释自己，那么应该简化代码。有一些例外情况(例如，正则表达式和复杂算法通常会从解释它们在做什么的注释中获益良多)，但是大多数注释是针对代码本身不可能包含的信息，比如决策背后的推理。

Review本次CL之前,看看本次CL的注释也很有帮助。也许现在有一个可以删除TODO，一个反对这个变更的建议的评论，等等。

请注意，注释不同于类、模块或函数的文档，这些文档应该表达一段代码的用途、应该如何使用它以及使用时它的行为。

格式

我们有针对所有主要语言的风格指南，甚至包括大多数次要语言。确保CL遵循适当的样式指南。

如果你想改进一些风格指南中没有的地方，在你的注释前面加上“Nit:”，让开发人员知道这是你认为可以改进代码但不是强制性的。不要仅仅根据个人的风格偏好来阻止CLs的提交。

CL的作者不应该将主要的样式更改与其他更改结合在一起。这使得很难看到CL中发生了什么变化，使得合并和回滚更加复杂，并导致其他问题。例如，如果作者想要重新格式化整个文件，让他们只将重新格式化后的文件作为一个CL发送给您，然后再发送另一个CL，其中包含其功能更改。

文档

如果CL更改了用户构建、测试、交互或发布代码的方式，请检查它是否也更新了相关文档，包括READMEs、g3doc页面和任何生成的参考文档。如果CL删除或弃用代码，请考虑是否也应该删除文档。如果缺少文档，就要求补充它。

每一行

查看分配给您的每一行代码。有些东西，比如数据文件、生成的代码或大型数据结构，您有时可以扫描它们，但不要扫描一个人编写的类、函数或代码块，并假定其中的内容是正确的。显然，有些代码需要比其他代码更仔细的检查—这是您必须做出的判断—但是您至少应该确保了解所有代码在做什么。

如果阅读代码对您来说太困难，并且这会减慢审查的速度，那么您应该让开发人员知道这一点，并等待他们澄清之后再进行审查。我们雇佣优秀的软件工程师，而你就是其中之一。如果您不能理解代码，其他开发人员也很可能无法理解。因此，当您要求开发人员澄清这些代码时，您也在帮助未来的开发人员理解这些代码。

如果您理解了代码，但是您觉得没有资格做部分检查，那么请确保CL上有一位合格的检查人员，特别是在安全性、并发性、可访问性、国际化等复杂问题上。

上下文

在广泛的背景下研究CL通常是有帮助的。通常，代码审查工具只会向您显示修改部分的几行代码。有时您必须查看整个文件，以确保更改实际上是有意义的。例如，您可能只看到添加了四行，但是当您查看整个文件时，您会看到这四行位于一个50行的方法中，现在确实需要将其分解为更小的方法。

在整个系统的上下文中考虑CL也是有用的。这个CL是改善了系统的代码健康状况，还是使整个系统更复杂、测试更少等等?不要接受降低系统代码健康状况的CL。大多数系统都是通过许多累积起来的小变更而变得复杂的，因此在新变更中防止小的复杂性非常重要。

好东西

如果您在CL中看到一些不错的东西，请告诉开发人员，特别是当他们以一种很好的方式处理您的一个注释时。代码评审通常只关注错误，但是它们也应该提供对良好实践的鼓励和赞赏。在指导方面，有时告诉开发人员他们做对了什么比告诉他们做错了什么更有价值

小结

在进行代码评审时，您应该确保:

• 代码设计良好。
• 这个功能对代码的用户来说是很好的。
• 任何UI更改都是合理的，看起来也不错。
• 任何并行编程都是安全的。
• 代码并不比它需要的实现更复杂。
• 开发人员没有实现他们将来可能需要但现在不知道需要的东西。
• 代码有适当的单元测试。
• 测试是精心设计的。
• 开发人员对所有东西都使用了清晰的名称。
• 注释是清晰和有用的，主要解释为什么而不是什么。
• 代码被适当地文档化(通常在g3doc中)。
• 代码符合我们的风格指南。

确保检查你被要求检查的每一行代码，查看上下文，确保你在改善代码的健康状况，并赞扬开发人员所做的好事。
下一章：查看CL