代码评审要注意哪些事情？

设计

代码评审中最重要的部分是CL的总体设计。CL中不同代码段之间的交互是有意义的吗？这个变更应该属于代码库，还是属于某个包？它与系统的其他部分可以良好地集成吗？现在是引入这个变更的好时机吗？

功能

这个CL是否达到了开发人员的目的？开发人员的意图对代码用户来说有好处吗？代码“用户”可以是指最终用户（他们受代码变更的影响）和开发人员（将来要“使用”这些代码）。

大多数情况下，我们希望开发人员先测试好CL，确保它们能够正确运行。但作为评审人员，你仍然要考虑一些边缘情况，比如查找并发问题，尝试像用户一样思考问题，并找出只是通过阅读代码无法看到的错误。

如果愿意，你也可以验证一下CL。如果一个CL会影响用户，比如做出了UI变更，那么这是验证CL的好时机。如果只是看代码，很难理解一些变更将如何影响用户。对于这样的更改，如果不方便自己运行，可以让开发人员提供功能演示。

另一个重要的考虑点是CL中是否存在可能导致死锁或竞态条件的并发问题。只是简单地运行代码很难发现这类问题，通常需要有人（开发人员和评审人员）仔细思考这些问题，确保不会把它们引入到系统中。

复杂性

CL比实际需要的更复杂吗？从每一层面检查CL，细到每一行代码，它们是不是太复杂了？函数是否过于复杂？类复杂吗？“太复杂”通常意味着“阅读代码的人难以很快理解它们”，也意味着“开发人员在调用或修改这些代码时可能会引入bug”。

过度设计是一种特殊的复杂性，开发人员把代码写得比实际需要的更通用，或者增加了系统当前不需要的功能。评审人员要警惕过度设计，鼓励开发人员只解决现在需要解决的问题，而不是将来可能需要解决的问题。未来的问题应该在它们出现之后再去解决，因为到了那个时候我们可以看到它们的实际状况和需求。

测试

要求开发人员进行单元测试、集成测试或端到端测试。一般来说， CL中应该包含测试，除非这个CL只是为了处理紧急情况。

确保CL中的测试是正确、合理和有用的。因为测试本身无法测试自己，而且我们很少会为测试编写测试，所以必须确保测试是有效的。

如果代码出了问题，测试会失败吗？如果代码发生改动，它们会误报吗？每一个测试都有断言吗？是否按照不同的测试方法对测试进行分类？

请记住，测试代码也是需要维护的。

命名

开发人员是否使用了良好的命名方式？好的命名要能够充分表达一个项（变量、类名等）是什么或者用来做什么，但又不至于让人难以阅读。

注释

开发人员有没有用自然语言写出清晰的注释？他们所写的注释都是必需的吗？通常，注释应该用于解释代码的用处，而不是解释它们在干什么。如果代码不够清晰，无法自解释，那就应该简化代码。当然也有一些例外（例如，正则表达式和复杂的算法，如果能够解释它们在做什么，会让阅读代码的人受益匪浅），但大多数注释都应该指出代码中不可能包含的信息，比如这些代码背后的缘由。

CL附带的其他注解也很重要，比如告知一个可以移除的待办事项，或者一个不要做出代码变更的建议，等等。

注意，注释不同于类、模块或函数文档。文档的目的是为了说明代码的用途、用法和行为。

代码风格

谷歌为主要编程语言和大多数次要编程语言提供了代码风格指南，所以要确保CL遵循了适当的指南。

如果你想对指南中没有提及的风格做出改进，可以在注释前面加上“Nit:”，让开发人员知道这是一个你认为可以改进的地方，但不是强制性的。但请不要只是基于个人偏好来阻止CL的提交。

开发人员不应该将风格变更与其他变更放在一起，这样很难看出CL发生了哪些变化，导致合并和回滚变得更加复杂。如果开发人员想要重新格式化整个文件，让他们将重新格式化后的文件作为单独的CL，并将功能变更作为另一个CL。

文档

如果CL导致用户构建、测试、交互或发布代码的方式发生了变化，请确保相关的文档也得到了更新，包括README、g3doc页和其他生成的参考文档。如果CL有移除或弃用代码，请考虑一下是否也应该删除相关的文档。如果文档缺失，要向开发人员索要。

查看每一行代码

查看每一行代码。有些东西可以看一看，比如数据文件、生成的代码或大型数据结构，但不要只是粗略地扫一下类、函数或代码块，并假定它们都能正常运行。显然，有些代码需要仔细检查，至于是哪些代码完全取决于你，但你至少应该要理解这些代码都在做些什么。

如果代码很复杂或者你难以快速看懂它们，导致评审速度变慢，你要让开发人员知道，并在进行进一步评审之前让他们做一些澄清。如果你看不懂这些代码，其他开发人员很可能也看不懂。因此，要求开发人员澄清代码其实也是在帮助未来的开发人员更好地理解代码。

如果你理解代码，但又觉得没有资格做代码评审，可以确保有资格的CL评审人员在代码评审时考虑到了安全性、并发性、可访问性、国际化等问题。

上下文

代码评审工具通常只显示被修改的代码，但有时候你需要查看整个文件，确保代码变更是有意义的。例如，你可能只看到新添加了四行代码，但如果你看一下整个文件，会发现这四行代码位于一个50多行的方法中，这个时候需要将这个方法拆分为更小的方法。

你需要基于整个系统来考量CL。这个CL是提升了系统的代码质量，还是让整个系统变得更复杂、更不可测？不要接受导致系统代码质量退化的CL。大多数系统都是因为累积了很多小的变更而变复杂的，所以要尽量避免小的变更带来的复杂性。

好的一面

如果你在CL中看到一些不错的东西，要让开发人员知道，特别是当他们以一种很好的方式解决了问题。代码评审通常只关注错误的东西，但其实也应该鼓励和赞赏好的代码实践。有时候，让开发人员知道他们做对了事情比让他们知道做错了事情更有价值。

总结

在进行代码评审时，你要确保：

· 良好的代码设计。

· 功能对代码用户来说是有用的。

· UI变更应该是合理的。

·并行编程是安全的。

· 代码复杂性不要超过应有的程度。

· 不需要实现可能会在未来出现的需求。

· 有适当的单元测试。

· 精心设计的测试用例。

· 使用了清晰的命名方式。

· 清晰而有用的代码注释，要解释“为什么”，而不是“什么”。

· 恰如其分的代码文档化。

· 代码要遵循风格指南。

检查每一行代码，查看上下文，确保你正在改进代码质量，并为表现不错的开发人员点赞。