良好代码审查的艺术

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

我们为什么要审查代码?

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

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

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

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

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

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

lgtm👍

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

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

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

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

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

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

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

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

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

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

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

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

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

0.描述

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

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

1.代码

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

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

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

“我乐意维护它吗?

2.测试

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

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

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

3.批评评论

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

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

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

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

4.审批

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

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

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

代码审查清单

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

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

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

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

阅读余下内容
 

发表回复

您的电子邮箱地址不会被公开。 必填项已用*标注


京ICP备12002735号