良好代码审查的艺术

这篇文章是我最近在公司演讲的延伸。由于时间仓促，听众又都是经验丰富的工程师，所以我讲得很简短，尽量避免喧宾夺主。但事后的反馈相当积极，也有一些问题，所以这里是扩展版本，供有兴趣的人参考。请注意，我们团队进行的是合并前审查，其中一些建议是针对合并前审查提出的。实际上，我更喜欢合并后审查，但我还没有说服我的同事们相信合并后审查更好。哦，触发警告：本文章包含观点。

我们为什么要审查代码？

首先，让我们确定一下代码审查的意义何在，以及它的不足之处。

审查代码的首要、最重要的原因是共享所有权。用代码术语来定义 “所有权 “可能很棘手，但主要是一种感觉。它意味着您了解代码，您有权修改代码，并有责任维护代码。

共享所有权非常重要，因为个人会以各种方式失败。他们会生病、换工作或升职。当然，代码也会失败，因此，只要重要的代码由个人拥有，那就意味着一颗定时炸弹正在倒计时破坏你的组织（通常是在周五深夜）。审查代码（以及结对编程等其他技术）是打破知识孤岛、降低紧急情况发生可能性的一种方法。

我们审查代码的第二个最重要的原因是 “大局观”。工程师天生就是注重细节的人。为了把工作做好，这项工作实际上迫使你关注细节。当你把一天的大部分时间都花在思考和处理小细节上时，就很容易忽略更广泛的问题。代码审查就是一个解决这个问题的机会，让我们退后一步，从更广阔的视角来考虑问题。你孤立构建的那个小组件在更广泛的系统中是否有意义？它是否会影响安全性、可观察性或性能？是否稳健？有没有更简单的方法？

排在这两个问题后面的是一系列不太重要的审查代码的理由：提高代码质量、坚持约定俗成、格式正确、捕捉错误、发现测试覆盖率中的漏洞等。这些都很好，但却不那么重要，因为代码审查是一种不可靠的捕捉方法。有些事情会被发现，但很多事情不会。如果代码审查是解决这些问题的唯一机制，那么你肯定会失败。这就是为什么我们要使用像编译器、编译器、类型检查器、格式化器和各种自动测试等工具的原因。如果你想检查的东西没有工具，那就写一个能检查的工具。当机器为你完成这些工作时，你的人类也会更快乐。

糟糕的代码审查是什么样的？

lgtm👍

当然，并不是所有的 “lgtm “代码审查都是糟糕的。有些修改是微不足道的，或者显然是正确的，”lgtm “对这些修改是没有问题的。但对于更复杂的修改，如果唯一的注释就是 “lgtm”，那就表明审查者还没有正确理解它。由于共享所有权，理解是首要目标。代码审查不是橡皮图章。

代码审查不力的另一个表现是审查出现得太快。阅读代码的速度有一个下限，超过这个下限就应该敲响警钟。不过也有例外。也许某项修改是合作完成的，而审阅者对其已经非常熟悉。

当对琐碎的修改进行了大量讨论，但对复杂的部分却没有提出任何意见时，也会出现糟糕的代码审查。这种情况属于 “bikeshed“的范畴，如果不加以纠正，可能会发展到令人发指的地步。

良好的代码评审需要良好的公关

良好的代码审查并不完全是审查者的责任。最初的责任在于被审查者，使他们的修改易于理解。

这首先要从描述开始，描述应清晰、详细、准确。描述应介绍变更，并包含理解变更所需的所有上下文。如果上下文太长，无法详细描述，则应加以总结，并链接到原始信息来源。说明应涵盖变化是什么、为什么需要变化以及你是如何处理变化的。如果有你选择反对的方法，也要提及这些方法以及你决定反对的原因。

然后，变更本身应只包含实现描述所需的最小差异。如果有切题的重构，应将其提取到一个新的分支中，并在主要变更之前单独审核。避免无意义的改动，这些改动无助于改进你正在处理的事情。改动的每一部分都应该有其目的，当被要求这样做时，你应该能够自圆其说。

在要求审核之前，应确保所有自动检查都是绿色的。审核员的注意力是有限的，不要浪费在尚未完成的事情上。

所有的行为变化都应附带对测试的相应修改。新功能应该有新的测试，行为修改应该修改现有的测试。如果没有现存的测试需要修改，那就趁机编写一些新的测试，这样将来就不会有人意外地破坏你的修改了。

如果修改本身是自动化的，则应包含足够的信息，以便审核人员审查调用，而不是数千行的生成输出。当然，他们也会想亲眼看看输出结果，但您使用的命令或编写的脚本才是这里要审查的真正内容。

最后，在请别人审查之前，自己先进行一次自我审查。这不会花很长时间，而且你可能会惊讶于它能发现多少错误。错别字、遗留的 TODO 注释、调试日志、跳过的测试、注释过的代码等等。如果这些错误没有出现在你的拉取请求中，你在同行眼中就会显得更有能力。

好的代码审查是什么样的？

作为审查员，您需要做好五件事。

0.描述

在查看代码之前，先阅读代码说明。有描述吗？是否合理？是否包含理解代码所需的所有信息？

这不是迂腐。你需要先理清所有这些问题，以确保你理解你要审核的内容。如果你能坚持不懈，工程师很快就会适应你的期望，写出好的描述也会成为一种习惯。

1.代码

阅读代码不仅仅是看一眼代码大纲。您需要真正阅读每一行的每一个标记。这需要时间。你不可能在 5 分钟内读完 500 行代码，所以要提前留出足够的时间，这样你才不会觉得匆匆忙忙就完成了，然后继续做下一件事。

您不必马上理解所有内容。对不清楚的部分可以提问。不要担心自己听起来很蠢，没有人会这么想，而且最快的正确方式就是当众说自己错了。对于你认为自己理解的部分，可以写下来检查自己的理解，并请被审查者确认（如果是错的）。如果你发现有些东西看起来很滑稽、奇怪、令人惊讶或复杂，就提出来。

逐步建立代码的心智模型，然后问自己一个关键问题，这个问题是共享所有权概念的基础：

“我乐意维护它吗？

2.测试

测试只是代码的另一部分，但我把它们单独拿出来是因为有时人们在审查时会完全跳过它们。理解测试与理解代码同样重要。难以理解的测试将来不太可能得到维护。好的测试可以作为有用的文档，说明系统能做什么，不能做什么。

确保应该测试的东西确实在测试。这不仅仅是覆盖率的问题，还需要考虑断言。如果断言没有执行正确的不变式，100% 的覆盖率就毫无意义。

在审查测试的同时，回过头来再次审查相关代码也会有所帮助。并排比较这两段代码，寻找似乎遗漏或不恰当的地方。对任何不符合您期望的地方提出问题。

3.批评评论

代码审查中有三条讨论规则：体贴、诚实和虚心。

要考虑周到，因为审查的另一端是有感情的人。当团队中每个人都相互尊重、和睦相处时，团队的工作会更好。因此，任何批评都应该针对代码，而不是人。应该冷静地提出批评，并附上理由。如果可能的话，也应提及其他建议，但应避免过于规范，尽量给被审查者留出一些空间，让他们自己找出解决方案。不要让编程失去乐趣。

要诚实，因为代码审查是改进整个团队实践的一种方式。如果你错误地试图 “友善 “而隐瞒意见，那么就无法实现这一目标。诚实也会让你自己的错误偶尔得到纠正，这意味着你个人也会从中受益。

要心胸开阔，因为所有方面的错误都是不可避免的，包括你自己的错误。不要过于拘泥于自己的观点，花点时间考虑其他观点。如果你发现自己陷入了分歧，双方都不愿让步，那就让步吧。有分歧没关系，继续前进，代码审查不是一项竞技运动。如果你真的对某件事情有强烈的感觉，可以把你的想法作为后续建议提出来。代码库很少是完整的，通常还有机会进一步改进。

4.审批

鉴于代码审查的主要目的是共享所有权，因此在您理解变更之前，您不能批准它。其他所有事情都是次要的，因此在您确信完全理解变更之前，请暂缓批准。

这一原则的反面是，只要你理解了，批准你不同意的变更也是可以的。代码审查不是独裁。

如果您对一些细微的修改提出了意见，无论如何也要在修改登陆之前预先批准。我们应该相信你的同事会妥善处理这些修改，即使他们做错了，也可以稍后再修正。预先批准可以确保在修改完成后，没有人被阻止合并修改。

代码审查清单

在代码审查中应注意哪些问题？有时，人们会密切关注少数几个重点领域，而对其他方面却听之任之。如果这听起来像您的情况，您可以使用这份（不完整的）清单来启发自己：

• 安全性
• 可观察性
• 性能
• 健壮性
• 复杂性
• 可读性
• 可维护性
• 过度工程（YAGNI）
• 正确使用现有抽象
• 命名
• 评论注释
• 测试
• 一次只做一件事（单独的 PR）
• “我想维护这个吗？

如果上述内容有什么是您不确定的，请先花点时间学习相关知识，然后在代码审查时提出相关问题。你很快就会在这方面提高水平。

本文文字及图片出自 The art of good code review