Code Review 要点

Code Review 要点

注意：在考虑这些要点时，请谨记 “Code Review 标准”。

设计

审查中最重要的是 CL 的整体设计。CL 中各种代码的交互是否有意义？此变更是属于您的代码库（codebase）还是属于库（library）？它是否与您系统的其他部分很好地集成？现在是添加此功能的好时机吗？

功能

这个 CL 是否符合开发者的意图？开发者的意图对代码的用户是否是好的？ “用户”通常都是最终用户（当他们受到变更影响时）和开发者（将来必须“使用”此代码）。

大多数情况下，我们希望开发者能够很好地测试 CL，以便在审查时代码能够正常工作。但是，作为审查者，仍然应该考虑边缘情况，寻找并发问题，尝试像用户一样思考，并确保您单纯透过阅读方式审查时，代码没有包含任何 bug。

当要检查 CL 的行为会对用户有重大影响时，验证 CL 的变化将变得十分重要。例如 UI 变更。当您只是阅读代码时，很难理解某些变更会如何影响用户。如果在 CL 中打 patch 或自行尝试这样的变更太不方便，您可以让开发人员为您提供功能演示。

另一个在代码审查期间特别需要考虑功能的时机，就是如果 CL 中存在某种并行编程，理论上可能导致死锁或竞争条件。通过运行代码很难检测到这些类型的问题，并且通常需要某人（开发者和审查者）仔细思考它们以确保不会引入问题。（请注意，这也是在可能出现竞争条件或死锁的情况下，不使用并发模型的一个很好的理由——它会使代码审查或理解代码变得非常复杂。）

复杂度

CL 是否已经超过它原本所必须的复杂度？针对任何层级的 CL 请务必确认这点——每行程序是否过于复杂？功能太复杂了吗？类太复杂了吗？ “太复杂”通常意味着“阅读代码的人无法快速理解。”也可能意味着“开发者在尝试调用或修改此代码时可能会引入错误。”

其中一种复杂性就是过度工程（over-engineering），如开发人员使代码过度通用，超过它原本所需的，或者添加系统当前不需要的功能。审查者应特别警惕过度工程。未来的问题应该在它实际到达后解决，且届时才能更清晰的看到其真实样貌及在现实环境里的需求，鼓励开发人员解决他们现在需要解决的问题，而不是开发人员推测可能需要在未来解决的问题。

测试

将要求单元、集成或端到端测试视为应该做的适当变更。通常，除非 CL 处理紧急情况，否则应在与生产代码相同的 CL 中添加测试。

确保 CL 中的测试正确，合理且有用。测试并非用来测试自己本身，且我们很少为测试编写测试——人类必须确保测试有效。

当代码被破坏时，测试是否真的会失败？如果代码发生变化时，它们会开始产生误报吗？每个测试都会做出简单而有用的断言吗？不同测试方法的测试是否适当分开？

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

命名

开发人员是否为所有内容选择了好名字？一个好名字应该足够长，可以完全传达项目的内容或作用，但又不会太长，以至于难以阅读。

注释

开发者是否用可理解的英语撰写了清晰的注释？所有注释都是必要的吗？通常，注释解释为什么某些代码存在时很有用，且不应该用来解释某些代码正在做什么。如果代码无法清楚到去解释自己时，那么代码应该变得更简单。有一些例外（正则表达式和复杂算法通常会从解释他们正在做什么事情的注释中获益很多），但大多数注释都是针对代码本身可能无法包含的信息，例如决策背后的推理。

查看此 CL 之前的注释也很有帮助。也许有一个 TODO 现在可以删除，一个注释建议不要进行这种改变，等等。

请注意，注释与类、模块或函数的文档不同，它们应该代表一段代码的目的，如何使用它，以及使用时它的行为方式。

风格

Google 提供了所有主要语言的风格指南，甚至包括大多数小众语言。确保 CL 遵循适当的风格指南。

如果您想改进风格指南中没有的一些样式点，请在评论前加上“Nit：”，让开发人员知道这是您认为可改善代码的小瑕疵，但不是强制性的。不要仅根据个人风格偏好阻止提交 CL。

CL 的作者不应在主要风格变更中，包括与其他种类的变更。它会使得很难看到 CL 中的变更了什么，使合并和回滚更复杂，并导致其他问题。例如，如果作者想要重新格式化整个文件，让他们只将重新格式化变为一个 CL，其后再发送另一个包含功能变更的 CL。

文档

如果 CL 变更了用户构建、测试、交互或发布代码的方式，请检查相关文档是否有更新，包括 README、g3doc 页面和任何生成的参考文档。如果 CL 删除或弃用代码，请考虑是否也应删除文档。如果缺少文档，请询问。

每一行

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

如果您觉得这些代码太难以阅读了并减慢您审查的速度，您应该在您尝试继续审核前要让开发者知道这件事，并等待他们为程序做出解释、澄清。在 Google，我们聘请了优秀的软件工程师，您就是其中之一。如果您无法理解代码，那么很可能其他开发人员也不会。因此，当您要求开发人员澄清此代码时，您也会帮助未来的开发人员理解这些代码。

如果您了解代码但觉得没有资格做某些部分的审查，请确保 CL 上有一个合格的审查人，特别是对于安全性、并发性、可访问性、国际化等复杂问题。

上下文

在广泛的上下文下查看 CL 通常很有帮助。通常，代码审查工具只会显示变更的部分的周围的几行。有时您必须查看整个文件以确保变更确实有意义。例如，您可能只看到添加了四行新代码，但是当您查看整个文件时，您会看到这四行是添加在一个 50 行的方法里，现在确实需要将它们分解为更小的方法。

在整个系统的上下文中考虑 CL 也很有用。这个 CL 是否改善了系统的代码健康状况，还是使整个系统更复杂，测试更少等等？不要接受降低系统代码运行状况的 CL。大多数系统通过许多小的变化而变得复杂，因此防止新变更引入即便很小的复杂性也非常重要。

好的事情

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

总结

在进行代码审查时，您应该确保：

代码设计精良。
该功能对代码用户是有好处的。
任何 UI 变更都是合理的且看起来是好的。
其中任何并行编程都是安全的。
代码并不比它需要的复杂。
开发人员没有实现他们将来可能需要，但不知道他们现在是否需要的东西。
代码有适当的单元测试。
测试精心设计。
开发人员使用了清晰的名称。
评论清晰有用，且大多用来解释为什么而不是做什么。
代码有适当记录成文件（通常在 g3doc 中）。
代码符合我们的风格指南。

确保查看您被要求查看的每一行代码，查看上下文，确保您提高代码健康状况，并赞扬开发人员所做的好事。

下一篇：查看 CL 的步骤