How to Do Code Reviews Like a Human (Part One)

Michael Lynch

像人一样做代码审查(第一部分)

最近,我一直在读一些关于代码审查最佳实践的文章。我注意到,这些文章都把重点放在找 bug 上,几乎完全忽略了审查的其他方面。以建设性、专业的方式沟通你发现的问题?不重要!只要找出所有 bug,其他事情自然会解决。

于是我突然想到:如果这套方法对代码有效,为什么不拿来谈恋爱呢?因此,我要宣布推出我的新电子书,帮助开发者改善自己的爱情生活:

电子书封面

我的革命性电子书将教你经过验证的技巧,帮助你最大限度地找出伴侣的缺点。本书不会涉及:

  • 如何带着同理心和理解与伴侣沟通问题。
  • 如何帮助伴侣改正弱点。

根据我读过的代码审查文献,这些关系方面的内容都是显而易见的不值得讨论

你觉得这是一本好电子书吗?我猜你刚才已经喊了:“不不不不不!”

那么,为什么我们谈论代码审查时却是这个样子?

我只能假设,我读到的那些文章来自未来——在那个未来,所有开发者都是机器人。在那个世界里,你的队友会欢迎那些不假思索措辞的代码批评,因为处理这些信息会温暖它们冰冷的机器人心。

我大胆假设,你想改善的是当下的代码审查,而你的队友都是人类。我再大胆一点假设:与同事保持积极的关系本身就是目的,而不只是一个用来调节、以降低每个缺陷成本的变量。在这种情况下,你的审查实践会如何改变?

本文讨论的技巧,不仅把代码审查视为一个技术流程,也把它视为一个社交流程。

什么是代码审查?

“代码审查”这个术语可以指一系列活动,从站在队友身后随手读几眼代码,到召开由 20 个人参加、逐行剖析代码的会议。我这里所说的,是一种正式且以书面形式进行的流程,但又没有繁重到需要一连串面对面的代码检查会议。

代码审查流程

代码审查的参与者包括作者审查者。作者编写代码并将其提交审查;审查者阅读代码,并决定何时可以将其合并到团队的代码库中。一次审查可以有多名审查者,但为简单起见,我假设你是唯一的审查者。

代码审查开始前,作者必须创建一个changelist(变更列表)。这是作者希望合并到团队代码库中的一组源代码变更。

作者将其 changelist 发送给审查者时,审查便开始了。代码审查以轮次进行。每一轮都是作者与审查者之间的一次完整往返:作者发送变更,审查者针对这些变更给出书面反馈。每次代码审查都会有一轮或多轮。

审查者批准这些变更后,审查便结束。这通常称为给出 LGTM,即“looks good to me(在我看来没问题)”的缩写。

为什么这很难?

如果一个程序员把自己认为很棒的 changelist 发给你,而你写下一长串理由说明它为什么并不棒,那么要把这个信息传达出去可不容易。

这就是我不怀念 IT 行业的原因之一,因为程序员都是很不讨人喜欢的人……比如在航空业,严重高估自己技能水平的人全都死了。

——ArsDigita 联合创始人 Philip Greenspun(菲利普·格林斯彭),引自《Founders at Work(创业者访谈录)》

作者很容易把对代码的批评理解为你在暗示他们是无能的程序员。代码审查是分享知识、做出明智工程决策的机会。但如果作者把这场讨论看成是人身攻击,这些事情就不可能发生。

仿佛这还不够难似的,你还要面对以书面形式表达想法的挑战,因为这会带来更高的误解风险。作者听不到你的语气,也看不到你的肢体语言,因此你更需要谨慎地组织反馈。对于一个心存防备的作者来说,一句无害的“你忘了关闭文件句柄”,可能会被读成:“我真不敢相信你居然忘了关闭文件句柄!你真是个蠢货。”

技巧

  1. 让计算机处理无聊的部分
  2. 用风格指南解决风格争论
  3. 立即开始审查
  4. 先从高层次开始,再逐步深入
  5. 慷慨地提供代码示例
  6. 永远不要说“你”
  7. 把反馈表述为请求,而不是命令
  8. 让意见基于原则,而不是个人看法

让计算机处理无聊的部分

在会议和电子邮件等打断之间,你能用于专心写代码的时间非常有限。你的脑力耐力更是供不应求。阅读队友的代码会带来很高的认知负担,需要高度集中注意力。不要把这些资源浪费在计算机能够完成的任务上,尤其是计算机还能做得更好的任务。

空白字符错误就是一个明显的例子。比较一下,人类审查者找出缩进错误并与作者协作修正它所需的工作量,和直接使用自动格式化工具相比有多大差别:

由人工审查者完成所需的工作使用格式化工具所需的工作
  1. 审查者搜索空白字符问题,并发现缩进不正确。
  2. 审查者写下意见,指出缩进不正确。
  3. 审查者重新阅读自己的意见,确保措辞清楚且不带指责意味。
  4. 作者阅读意见。
  5. 作者修正代码缩进。
  6. 审查者确认作者已正确处理自己的意见。
什么都不用做!

右侧之所以是空的,是因为作者使用的代码编辑器会在每次点击“Save”时自动格式化空白字符。最差的情况是,作者把代码提交审查后,continuous integration(持续集成)方案报告空白字符不正确。作者自行修正问题,审查者甚至无需操心。

在代码审查中,寻找可以自动化处理的机械性任务。常见的有以下几种:

任务自动化方案
确认代码能够构建持续集成方案,例如 TravisCircleCI
确认自动化测试通过持续集成方案,例如 TravisCircleCI
确认代码空白字符符合团队风格代码格式化工具,例如 ClangFormat(C/C++ 格式化工具)或 gofmt(Go 格式化工具)。
识别未使用的导入或变量代码 linter,例如 pyflakes(Python linter)或 JSLint(JavaScript linter)。

自动化能帮助你作为审查者做出更有意义的贡献。当你可以忽略整类问题(例如 imports 的排序或源文件的命名约定)时,就能把注意力放在更有趣的事情上,比如功能错误或可读性方面的缺陷。

自动化对作者也有好处。它让作者能在几秒钟内发现粗心造成的错误,而不是花上几个小时。即时反馈让学习更容易、修复成本更低,因为作者脑中还保留着相关上下文。此外,如果作者必须听说自己犯了一个愚蠢的错误,那么由计算机告诉他们,远比由你告诉他们更不伤自尊。

与团队协作,把这些自动检查直接集成到代码审查流程中(例如 Git 中的 pre-commit hooks,或 GitHub 中的 webhooks)。如果审查流程要求作者手动运行这些检查,你就放弃了大部分收益。作者总会偶尔忘记运行,这迫使你继续审查那些本应由自动化处理的简单问题。

用风格指南解决风格争论

在审查中争论风格是在浪费时间。一致的风格当然很重要,但代码审查不是争论大括号该放在哪里的时机。把风格争论从审查中彻底清除的最佳方法,是维护一份风格指南。

典型的风格争论

优秀的风格指南定义的不仅是命名约定或空白字符规则等表面元素,还包括如何使用特定编程语言的功能。例如,JavaScript 和 Perl 都拥有极其丰富的功能——同一套逻辑往往有许多种实现方式。风格指南会定义做事的唯一正确方式,避免团队一半人使用一套语言功能,而另一半人使用完全不同的功能。

有了风格指南后,你就不必浪费审查轮次,和作者争论谁的命名约定更好。只需遵循风格指南,然后继续前进。如果你的风格指南没有规定某个具体问题的约定,那么通常就不值得争论。如果遇到指南未涵盖的风格问题,而且重要到值得讨论,就和团队一起把它解决。然后把决定记录到风格指南中,这样以后就再也不用讨论同一个问题了。

选项 1:采用现有的风格指南

在网上搜索一下,你就能找到许多可以直接采用的公开风格指南。Google 的风格指南最为人熟知,但如果这种风格不适合你,也可以找到其他指南。采用现有指南,就能获得风格指南的好处,而不必承担从零创建指南的巨大成本。

缺点是,各组织会针对自身的特定需求优化风格指南。例如,由于 Google 拥有庞大的代码库,其中的代码必须能够运行在从家用路由器到最新 iPhone 的各种设备上,因此 Google 的风格指南对使用新的语言功能持保守态度。如果你是一家只有四个人、只有一个产品的初创公司,那么你可能会选择更积极地使用最前沿的语言功能或扩展。

选项 2:逐步创建自己的风格指南

如果你不想采用现有指南,也可以创建自己的指南。每当代码审查中出现风格争论时,就把问题提交给整个团队,由团队决定官方约定是什么。达成一致后,把这个决定正式写入风格指南。

我更喜欢把团队的风格指南以 Markdown 的形式存放在源代码管理系统中(例如 GitHub pages)。这样,风格指南的任何变更都会经过正常的审查流程——必须有人明确批准变更,团队中的每个人也都有机会提出疑虑。Wiki 和 Google Docs 也是可以接受的选择。

选项 3:混合方案

结合选项 1 和 2,你可以采用现有风格指南作为基础,然后维护一份本地风格指南,对基础指南进行扩展或覆盖。Chromium C++ 风格指南就是一个很好的例子。它以 Google 的 C++ 风格指南为基础,再在其上做出自己的修改和补充。

立即开始审查

把代码审查视为高优先级事项。真正阅读代码并给出反馈时要慢慢来,但要立即开始审查——最好在几分钟内开始。

代码审查接力赛

如果队友把一个 changelist 发给你,很可能意味着在审查完成前,他们会被其他工作卡住。理论上,源代码管理系统允许作者创建分支、继续工作,然后将审查中的变更前向合并到新分支中。实际上,总共只有大约四名开发者能高效完成这件事。对其他人来说,理清三方差异所需的时间太长,可能抵消等待审查返回所取得的全部进展。

立即开始审查,会形成一个良性循环。你的审查周转时间将完全取决于作者 changelist 的规模和复杂程度。这会促使作者提交小而明确的 changelist。这样的 changelist 更容易审查,审查体验也更好,于是你会审查得更快,循环就这样持续下去。

想象一下,你的队友实现了一个新功能,需要修改 1,000 行代码。如果他们知道你大约能在 2 小时内审查一个 200 行的 changelist,那么他们可以把这个功能拆成若干个约 200 行的 changelist,并在一两天内让整个功能通过检入。然而,如果你无论审查规模大小都要花一天时间,那么这个功能现在需要一周才能检入。你的队友不想干等一周,因此他们会受到激励,提交更大的代码审查,比如每次 500–600 行。这样的审查成本更高,反馈质量也更差,因为相比 200 行的变更,要保持对 600 行变更的上下文更加困难。

一次审查轮次的周转时间,绝对最长不应超过一个工作日。如果你正忙于处理优先级更高的问题,无法在一天内完成一轮审查,请告知队友,并给他们机会将审查重新分配给其他人。如果你被迫拒绝审查的次数每月超过一次左右,这很可能意味着团队需要放慢节奏,才能维持合理的开发实践。

先从高层次开始,再逐步深入

在一轮审查中写的意见越多,作者就越有可能感到不堪重负。具体上限因开发者而异,但通常在单轮审查提出 20–50 条意见时,就开始进入危险区。

如果你担心意见太多会把作者淹没在意见的海洋里,那么在前几轮中限制自己,只提供高层次的反馈。重点关注重新设计类接口或拆分复杂函数等问题。等这些问题解决后,再处理变量命名或代码注释清晰度等较低层次的问题。

作者整合你的高层次意见后,你的低层次意见可能就不再适用了。把它们推迟到后续轮次,你既省去了撰写措辞谨慎、指出这些问题的评论所需的不少工作,也让作者免于处理不必要的意见。这种技巧还会把你在审查中关注的抽象层次分开,帮助你和作者以清晰、系统的方式处理 changelist。

慷慨地提供代码示例

在理想世界里,代码作者会感谢收到的每一次审查。这是他们学习的机会,也能保护他们免于犯错。但现实中,有许多外部因素可能导致作者对审查产生负面看法,并因你给他们提意见而对你心怀不满。也许他们正承受着赶工期的压力,因此除了你立即盖章批准之外的任何反馈,都像是在阻碍他们。也许你们合作不多,所以他们不相信你的反馈是出于善意。

让作者对审查流程感觉良好的一个绝佳方法,就是在审查过程中寻找机会送给他们一些礼物。而所有开发者都喜欢收到的礼物是什么?当然是代码示例。

收到代码这份礼物

如果你通过写出部分建议的修改来减轻作者的负担,就表明你作为审查者愿意慷慨地投入时间。

例如,假设你的同事不熟悉 Python 的 list comprehensions(列表推导式)功能。他们提交了一份代码审查,其中包含以下代码:

urls = []
for path in paths:
  url = 'https://'
  url += domain
  url += path
  urls.append(url)

如果你回复:“我们能用列表推导式简化这段代码吗?”他们会很恼火,因为现在他们不得不花 20 分钟研究一个自己从未用过的东西。

如果收到下面这样的意见,他们会高兴得多:

可以考虑用类似下面的列表推导式来简化:

urls = ['https://' + domain + path for path in paths]

这种技巧并不局限于一行代码。我经常会为代码创建自己的分支,用来向作者演示较大规模的概念验证(例如拆分大型函数,或添加单元测试来覆盖额外的边界情况)。

把这种技巧留给明确且没有争议的改进。在上面的列表推导式例子中,很少有开发者会反对将代码行数减少 83%。相比之下,如果你写了一大段示例,只是为了展示一个基于个人品味而“更好”的改动(例如风格变更),代码示例就会让你显得咄咄逼人,而不是慷慨。

每轮审查将代码示例限制在两三个。如果你开始替作者写完整个 changelist,就会传达出你认为他们没有能力自己写代码的信号。

永远不要说“你”

这一点听起来会很奇怪,但请听我说:代码审查中永远不要使用“你”这个词。

审查中达成的决定,应当基于什么能让代码变得更好,而不是谁提出了这个想法。你的队友为 changelist 付出了大量努力,很可能为自己的成果感到自豪。听到对自己工作的批评时,他们的自然反应就是产生防备心理,并保护自己的成果。

组织反馈措辞时,要尽量降低触发队友防御心理的风险。明确表示你批评的是代码,而不是写代码的人。当作者在评论中看到“你”时,他们的注意力就会从代码转回自己身上。这会增加他们把批评当成针对个人的批评的风险。

看看下面这条无害的评论:

你把“successfully”拼错了。

作者可以用两种截然不同的方式理解这条意见:

  • 理解 1:嘿,老朋友!你把“successfully”拼错了。不过我仍然觉得你很聪明!可能只是个笔误。
  • 理解 2:你把“successfully”拼错了,蠢货。

再看看省略了“你”的意见:

sucessfully -> successfully

后者只是简单的更正,并不是对作者的评判。

幸运的是,改写反馈、避免使用“你”很容易。

选项 1:把“你”替换成“我们”

你能把这个变量重命名为更有描述性的名称,例如 seconds_remaining 吗?

改成:

我们能把这个变量重命名为更有描述性的名称,例如 seconds_remaining 吗?

“我们”强化了团队对代码共同承担责任这一点。作者可能会跳槽到另一家公司,你也可能会,但拥有这段代码的团队会以某种形式继续存在。明明是希望作者亲自完成的事情,却说“我们”可能听起来很傻,但傻一点总好过带有指责意味。

搬沙发漫画

选项 2:从句子中删去主语

避免使用“你”的另一种方法,是使用省略句子主语的简写形式:

建议重命名为更有描述性的名称,例如 seconds_remaining

使用被动语态也能达到类似效果。在技术写作中,我通常像躲避瘟疫一样避免使用被动语态,但它可以帮助你绕开“你”:

这个变量应该重命名为更有描述性的名称,例如 seconds_remaining

还可以把它表述成一个问题,以“what about…”或“how about…”开头:

把这个变量重命名为更有描述性的名称,例如 seconds_remaining,怎么样?

把反馈表述为请求,而不是命令

代码审查比日常沟通更需要策略和谨慎,因为讨论很容易偏离轨道,变成个人争执。你可能会认为审查者会在审查中提高礼貌程度,但奇怪的是,我发现他们往往反其道而行之。大多数人绝不会对同事说:“把那个订书机递给我,然后给我拿瓶汽水。”但我见过许多审查者用类似强硬的命令来表达反馈,例如:“把这个类移到单独的文件中。”

反馈时宁可过分温和,也不要不够温和。把意见表述为请求或建议,而不是命令。

比较一下同一条意见的两种不同表达方式:

以命令形式表达的反馈以请求形式表达的反馈
Foo 类移到单独的文件中。我们能把 Foo 类移到单独的文件中吗?

人们喜欢觉得自己的工作由自己掌控。向作者提出请求,会让他们感到拥有自主权。

请求也让作者更容易礼貌地反驳。也许他们的选择有充分理由。如果你把反馈表述为命令,作者的任何反驳都会显得像是不服从。如果你把反馈表述为请求或问题,作者就可以直接回答你。

比较一下,根据审查者如何表达最初的意见,对话显得有多么好斗:

以命令形式表达的反馈(对抗性)以请求形式表达的反馈(合作性)
审查者:把 Foo 类移到单独的文件中。
作者:我不想这么做,因为那样它就会离 Bar 类很远。客户端几乎总是会一起使用这两个类。
审查者:我们能把 Foo 类移到单独的文件中吗?
作者:可以,但那样它就会离 Bar 类很远,而且客户端通常会一起使用这两个类。你觉得呢?

看到了吗?当你构造虚构的对话来证明自己的观点把意见表述为请求而不是命令时,对话会变得文明得多。

让意见基于原则,而不是个人看法

给作者提意见时,要同时解释你建议的修改以及修改的原因。与其说“我们应该把这个类拆成两个”,不如说:“目前,这个类既负责下载文件,也负责解析文件。根据单一职责原则,我们应该把它拆成一个下载器类和一个解析类。”

让意见以原则为依据,会以建设性的方式框定讨论。当你给出具体理由,例如“我们应该把这个函数设为私有,以尽量缩小类的公共接口”时,作者就不能简单地回答:“不,我更喜欢我的方式。”或者,他们可以这么回答,但会显得很可笑,因为你已经说明了这项修改如何满足某个目标,而他们只是在表达偏好。

软件开发既是艺术,也是科学。你不可能总是用既定原则准确说明一段代码到底哪里有问题。有时代码就是丑陋或不直观,很难明确说出原因。在这种情况下,能解释多少就解释多少,但要保持客观。如果你说“觉得这段代码很难理解”,至少这是一种客观陈述;相比之下,“这段代码令人困惑”是一种价值判断,对每个人来说未必如此。

如果可能,请用链接提供支持证据。团队风格指南中相关章节的链接,是你能提供的最佳链接。你也可以链接到该语言或库的文档。获得大量赞同的 StackOverflow 回答也可以,但你越偏离权威文档,证据就越不可靠。

第二部分

如果你喜欢这篇文章,可以看看本文的后半部分,其中重点介绍如何在不发生难看冲突的情况下顺利结束审查。内容包括以下技巧:

  • 处理规模过大的代码审查;
  • 发现给予赞扬的机会;
  • 尊重审查范围;以及
  • 化解僵局。

像人一样做代码审查(第二部分)


Samantha Mason(萨曼莎·梅森)编辑。插图由 Loraine Yow(洛林·尤)绘制。感谢 @global4g 为本文早期草稿提供宝贵反馈。

原文由 Michael Lynch 发布

本文章由 openai/gpt-5.6-luna 进行翻译