代码评审应该关注什么？

注意:当我们考虑以下点时，应当始终遵循代码评审标准。

设计

代码评审中最重要的一个点就是把握住变更中的整体设计。变更中各个部分的代码交互是否正常？整个改动是否属于你负责的代码库？是否和你系统中其他部分交互正常？现在是否是添加整个功能的恰当时间？

功能性

开发者在这个变更中想做什么？ 开发人员打算为该代码的用户带来什么好处？（这里”用户“是指受变更影响到的实际用户，和将来会使用到这些代码的开发者）

重要的是，我们希望开发者能充分测试代码，以确保代码在代码评审期间正常运行。但无论如何，你作为审查者也要考虑一些特殊情况，像是有些并发问题。站在用户的角度，确保你正在看的代码没有bug。

你可以验证变更，尤其是在有面向用户的影响时，评审者应该仔细检查整个变更。有时候单纯看代码很难理解这个变更如何影响到用户，这种情况下如果你不方便自己在CL中打补丁并亲自尝试，你可以让开发者为你提供一个功能性的demo。

另一个在代码评审时需要特别关注的点是，CL中是否有 并发编程，并发理论上可能会导致死锁或资源争抢，这种问题在代码运行时很难被检测出来，所以需要有人（开发者和评审者）仔细考虑整个逻辑，以确保不会引入线程安全的问题。（所以除了死锁和资源争抢之外，增加代码评审复杂度也可以成为拒绝使用多线程模型的一个理由。）

复杂性

变更是否比预期的更复杂？检测变更的每个级别，是否个别行太复杂？功能是否太复杂？类是否太复杂？”复杂“意味着代码阅读者很难快速理解代码，也可意味着开发者尝试调用或修改此代码时可能会引入bug。。

一个典型的复杂性问题就是 过度设计，当开发者让代码变得更通用，或者增加了许多当前不需要的功能时，评审者应该额外注意是否过度设计。鼓励开发者解决现在遇到的问题，而不是揣测未来可能遇到的问题。未来的问题应当在遇到时解决，到那个时候你才能看到问题本质和实际需求。

测试

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

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

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

记住，不要以为测试不是二进制中的一部分就不关注其复杂度。

命名

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

注释

开发者有没有写出清晰易懂的注释？所有的注释都是必要的吗？ 通常注释应该解释清楚为什么这么做，而不是做了什么。如果代码不清晰，不能清楚地解释自己，那么代码可以写的更简单。当然也有些例外（比如正则表达式和复杂的算法，如果能够解释清楚他们在做什么，肯定会让阅读代码的人受益良多），但大多数注释应该指代码中没有包含的信息，比如代码背后的决策。

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

注意，注释不同于类、模块或函数文档，文档是为了说明代码片段如何使用和使用时代码的行为。

风格

在谷歌内部，主流语言甚至部分非主流语言都有编码风格指南 ，确保变更遵循了这些风格规范。

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

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

文档

如果变更改变了用户构建、测试、交互或者发布代码相关的逻辑，检测是否也更新了相关文档，包括READMEs, g3doc页面，以及任何相关文档。如果开发者删除或者弃用某些代码，考虑是否也应该删除相关文档。如果文档有确实，让开发者补充。

代码细节

查看你整个代码评审中的每一行代码，比如你可以扫到的数据文件，生成的代码或大型数据结构，但不要只扫一遍手写的类，函数或者代码块，然后假设它们都能正常运行。显然，有些代码需要仔细检查，这完全取决于你，但你至少应该理解所有的代码都在做什么。

如果你很难看懂代码，导致代码评审的速度慢了下来，你要让开发者知道，并且在Review前澄清原因。在谷歌，我们有最优秀的工程师，你也是其中之一。如果你都看不懂，很可能其他人也看不懂。所以要求开发者理清代码逻辑也是在帮助未来的开发者。

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

上下文

看改动上下文代码对代码评审很有帮助，因为通常代码评审工具只会显示改动部分上下几行代码，但有时你必须查看整个文件确保这次变更可以正常运行。例如，你可能看到加了4行代码，但是看到整个文件时才会发现这加的4行代码是在一个50多行的方法里，这个方法现在应该被拆分为多个小的方法了。

代码评审时考虑到整个系统的上下文也很重要，这次改动提升了系统健康度？或者增加了系统复杂性？少了测试用例？…… 不要通过哪些会损害系统健康的代码。 很多系统变复杂都是因为一点一点的小改动日积月累造成的，所以千万不要放进来任何增加复杂性的改动。

亮点

如果你看到变更中做的好的地方，告诉开发者他们做的很棒，尤其是当他们用某种很精巧的方式解决你评论中提到的问题时更应不吝赞美。 代码评审过多关注于错误，但你也应该为开发者展现出好的一面点赞。在指导他人的时候，有时候告诉开发者正确的做法比告诉他们错误的做法更有价值。

总结

在做代码评审时，你需要注意以下：

• 代码设计良好。

• 代码功能对用户有用。

• 任何UI改动应当是深思熟虑过而且看起来不错的。

• 保证线程安全。

• 代码没有增加不必要的复杂性。

• 开发者没有写有些将来需要但现在不知道是否需要的东西。

• 代码有适当的单元测试。

• 测试逻辑设计良好。

• 开发者使用了清晰明了的命名。

• 注释清晰明了实用，通常解释清楚了为什么这么做，而不是做了啥。

• 代码又相应完善的文档。

• 代码风格符合规范。

确保你review了要求你看的每一行代码，确保你正在提升代码质量，并且为开发者做的提升点赞。

下一篇：代码评审的步骤