关于代码评审（Code Review）

[berwin]

关于代码评审（Code Review）

前不久Google开源了一份文档（谷歌工程实践文档），里面包含了他们的代码评审（Code Review）指南，通读之后我发现这份文档非常有价值，所以决定写一篇文章将一些入门的，科普性质的部分翻译并总结成一篇文章帮助大家了解下Google的代码评审。

在Google开源的文档中，有两个内部的专业术语：CL和LGTM：

• CL，全称为“Change List”，表示已提交到版本控制或正在进行代码评审的一个独立更改。

  我理解和Github上的PR有点类似。

• LGTM，表示“Looks Good to Me”，当某个CL被批准后，评审者会说LGTM。

1. 什么是代码评审

代码评审是评审者（某段代码作者之外的人），对某段代码进行检查（审核）的一个过程。

我们可以使用代码评审来维持代码和产品的质量，代码评审的主要目的是确保代码库的整体健康状况会随着时间不短改善。

代码评审的另一个重要功能是，它可以教给开发者一些关于语言，框架，常用的设计原则等知识。

1.1 评审什么

当进行代码评审时，评审者应该评审：

• 设计：代码是否拥有良好的设计，并适用于项目整体设计。
• 功能：代码的行为是否是作者想要的？这种行为对用户是否友好。
• 复杂度：能否让代码更简单？如果未来某一天，其他开发者遇到这段代码时，他们能否很容易理解并使用这段代码？
• 测试：代码是否拥有正确，且设计良好的测试。
• 命名：是否为变量，类，方法等选择了清晰的名字？
• 注释：注释是否清晰且有用
• 风格：代码风格是否符合项目标准
• 文档：开发者是否更新了相关的文档

本文的后面会针对上面提到的几点进行更详细的介绍。

1.2 选择合适的审核者

通常，开发者希望找到可以在合理的时间内响应自己审核需求的最合适地审核者。

最合适的审核者是能够为代码片段提供最彻底，最正确地审核的人，通常是代码的主人。有时可能会请求不同的人帮自己评审CL的不同部分。

如果与自己“结对编程”的人有资格作为这段代码的审核者，那么该代码将被视为已审核。

1.3 代码评审标准

评审者有责任确保每个CL的质量都使得代码库的整体健康状况不会随着时间而减少。这很困难，通常随着时间推移，代码库的健康会缓慢下降，尤其是团队处在时间限制下，为了快速迭代功能时。

通常，只要CL可以改善整体代码的健康，评审者就应该批准，即便CL并不完美。

这是所有代码评审指南中的最高原则。

当然，这是有局限性的。例如，如果CL添加了评审者不希望出现在系统中的功能时，那么可以拒绝批准，即便代码拥有良好的设计。

这里的关键点是，没有“完美”的代码，只有更好的代码。评审者追求的是持续改进，而不是追求完美。

总体而言，只要一个CL能对整个系统的维护性，可读性和可理解性起到改善的作用，评审者就不应该因为它不是“完美”的而被延迟几天或几周再批准。

评审者应该随时发表评论，表示某些代码可以变得更好，但是如果不是很重要，那么可以加上前缀“Nit:”让作者知道这仅仅是一个建议，可以选择忽略。

留下一些可以帮助开发者学习新东西的评论总是好的，随着时间的推移，分享知识是改善代码健康的一部分。但请记住，如果评论仅仅是出于教育目的，请在评论前面加上“Nit:”前缀，表明不是强制要求作者在此CL中对其进行解决。

1.3.1 原则

• 技术事实和数据要优先于个人喜好和意见
• 关于风格问题，团队的风格指南是绝对的权威。不在风格指南中的任何代码习惯都属于个人喜好问题。风格应该与现有代码保持一致。如果之前没有规定这样的代码风格，则接受CL作者的风格。
• 软件设计方面从来不是纯粹的风格问题或个人喜好问题，它们基于基本原则，而不是简单的个人偏好。有时，会有几个不同的可行方案，如果作者能够通过数据或基于可靠的工程原则证明几种方案同样有效，那么评审者应该接受作者的偏好。否则，还是根据软件设计的标准原则进行选择。
• 如果没有其他适用规则，则评审者可以要求作者的偏好于当前代码库保持一致，只要这不会影响整体代码的健康情况。

2. 代码评审

注意：在考虑以下要点时，始终确保考虑到前面提到的“代码评审标准”。

2.1 评审“设计”

代码评审中最重要的事情是CL的总体设计。代码中的各个部分之间的交互是否有意义？本次修改应该放在代码仓库中吗？它与系统中的其他部分可以完美结合在一起吗？现在是添加这个功能的好时机吗？

2.2 评审“功能”

CL的功能是否符合开发者的预期？开发者想为用户提供哪些功能？

通常我们希望开发者会为CL提供良好的测试，但是作为评审者，仍然应该考虑一些极端情况，寻找并发现问题。尝试像用户一样思考，来确保这个功能没有bug，而不仅仅是通过阅读代码就确定没有bug。

当CL是对用户有影响的改动（例如：修改了UI上某个功能），最重要的事是检查CL的行为。因为评审者只是阅读代码很难理解这个代码修改会对用户产生什么影响，对于这样的修改，如果评审者检查CL的行为过于麻烦，可以让开发者提供该功能的演示。

2.3 评审“复杂度”

CL是否可以实现的更简单？对CL的所有“级别”进行检查。某行代码是否过于复杂？功能是否过于复杂？类是否过于复杂？“过于复杂”通常意味着代码的阅读者不能快速理解。还意味着开发者在尝试调用或修改此代码时，可能会引入bug。

关于复杂，有一种特殊的类型，叫做“过度设计”，开发者让代码过分通用，或添加了一些暂时还不需要的功能。评审者应该特别警惕过度设计。鼓励开发者解决他们现在需要解决的已知问题，而不是开发者推测的将来可能需要解决的问题。未来的问题应该在问题来临时解决。

2.4 评审“测试”

根据修改的内容进行单元测试，集成测试或端到端测试。确保CL中的测试正确，合理且有用，开发者必须确保测试有效。

测试也是必须维护的代码，不要因为代码是测试代码就接受代码的复杂性。

2.5 评审“命名”

开发者是否为所有命名都选择了一个好名字？好名字应该足够长，以充分表达含义和作用，而又不会太长而难以阅读。

2.6 评审“注释”

开发人员是否写下了清晰的注释？所有注释都是必要的吗？通常当注释解释了代码为什么存在时非常有用，不应该解释代码做了什么，如果代码不能清晰的解释自身，则应该让代码更简单。有一些例外的情况（正则表达式与复杂的算法通常会从解释他们作用的注释中受益匪浅），但大多数注释是针对代码本身无法包含的信息，例如决策背后的原因。

2.7 评审“风格”

请确保CL的风格与团队的风格指南保持一致。

如果想改善风格指南中没有提到的部分，请在注释前面加上“ Nit：”，以使开发者知道这是可以改善但不是强制性的选择。

CL不应该将风格改动和其他改动结合在一起提交，这会导致查看CL中有哪些改动变得非常困难，还会使merge与回滚也更加复杂。例如，如果开发者想重新格式化整个文件，则需要将重新格式化的改动作为一个CL提交，然后再发送另一个具有功能改动的CL。

2.8 评审“文档”

如果CL改变了用户构建，测试，交互或发布代码的方式，请检查是否更新了相关文档。如果CL删除或弃用了代码，请考虑是否应该删除该文档。

3. 总结

本文提到的内容只是谷歌工程实践文档中的一小部分，感兴趣的可以看原文了解更多。

通读了一遍谷歌工程实践文档后，我发现代码评审在Google内部是开发流程的一部分，和在Github上为开源项目贡献PR类似，开发者提交了PR后，项目作者肯定会先Review一遍代码，然后再决定是否将代码合并到仓库中，没有经过Review的代码是无法合并到仓库里的。

但大部分公司不是这么开发项目的，我觉得这也是代码评审普遍做得不够好的原因。

可能很多人会对代码评审有很多疑问，例如，将代码评审作为项目开发必须要走的一环，是否会降低开发效率。代码评审是否会消耗评审者很多时间。开发者和评审者之间发生了冲突怎么办等问题。

这些问题在谷歌工程实践文档中有清晰的解释，感兴趣的可以去看原文。原文地址：https://github.com/google/eng-practices