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

2017-11-09

Michael Lynch

像人一样做代码审查（第二部分）

这是我关于如何在代码审查中进行良好沟通、避免踩坑的文章的下半部分。在这里，我重点介绍如何让代码审查顺利收尾，同时避免难堪的冲突。

我在第一部分中打好了基础，因此建议从那里开始阅读。如果你等不及了，这里是简短版：优秀的代码审查者不仅能发现 bug，还会提供认真负责的反馈，帮助队友进步。

我最糟糕的一次代码审查

我人生中最糟糕的一次代码审查，是针对一位前队友进行的。下文称她为 Mallory（马洛里）。她在我加入公司前几年就已经入职，但直到最近才转到我的团队。

审查

当 Mallory 把她的第一个 changelist（变更列表）发给我审查时，代码有些粗糙。她以前从没写过 Python，而且她是在一个笨重的遗留系统之上开发的，而这个系统一直由我维护。

我尽职尽责地记录下了发现的所有问题，总共 59 个。根据我读过的代码审查相关文章，我做得非常出色。我发现了这么多错误。因此，我一定是个优秀的审查者。

几天后，Mallory 把更新后的 changelist 以及对我所留评论的回复发了过来。她修复了简单的问题：拼写错误、变量重命名之类。但她拒绝处理更高层次的问题，例如她的代码在输入格式错误时会产生未定义行为，或者她的某个函数把控制流结构嵌套了六层。相反，她不屑地解释说，修复这些问题不值得投入工程时间。

我又生气又沮丧，于是发起了新一轮评论。我的语气很专业，但逐渐滑向了消极攻击式的表达。“你能解释一下，我们为什么希望在输入格式错误时出现未定义行为吗？”你可以猜到，Mallory 的回复变得更加顽固。

苦涩的循环

那是一个星期后的星期二。Mallory 和我仍在围绕同一次审查来回争论。我在前一天晚上发给了她最新的评论。我故意等到她下班后才发，因为不想在同一个房间里看到她阅读这些评论。

整个上午，我都因害怕下一轮审查而感到胃部深处沉甸甸的。午饭回来后，我看到 Mallory 不在座位上，但她发来了新的修改。我想，她大概也不想在我阅读她的回复时待在附近。

她的每条回复都让我更加恼火，我的心开始在胸腔里砰砰直跳。我立刻开始在键盘上猛烈敲击反驳，指出她既没有做出我建议的修改，也没有给出足以让我批准的理由。

我们每天重复这一套流程，持续了三周。代码几乎没有变化。

介入

幸好，我们最资深的队友 Bob（鲍勃）打破了这个循环。他结束长假回来后，惊讶地发现我们正苦涩地把代码审查评论来回扔给对方。他立刻看出了事情的本质：僵局。他请求接手审查，我们都同意了。

Bob 开始审查时，先让 Mallory 创建新的 changelist，把两个我们从未真正争论过的小型库拆出来，每个大约 30—50 行。Mallory 做完后，Bob 立即批准了它们。

然后，Bob 回到主 changelist。此时它已经精简到大约 200 行代码。他提出了几条小建议，Mallory 做了相应处理。随后，他批准了这个 changelist。

Bob 整个审查过程只用了两天。

沟通很重要

你可能已经推断出，这场冲突其实并不是关于代码本身。代码确实存在合理的问题，但善于沟通的队友显然能够解决这些问题。

这是一段令人不快的经历，但现在回想起来，我很庆幸经历过它。它促使我重新评估自己的审查方式，并找出可以改进的地方。

下面我会分享一些技巧，帮助你降低遭遇类似糟糕结果的风险。稍后我还会回到 Mallory 的故事，解释为什么我最初的做法本末倒置，以及 Bob 的做法为何看似平静却极其高明。

技巧

目标是让代码提升一到两个等级

理论上，你的队友可能希望抓住每个机会来改进代码，但他们的耐心是有限的。如果你因为不断想到新的、绝妙的方式让他们打磨 changelist，而一轮又一轮地拒绝批准，他们很快就会感到沮丧。

我私下里会用从 A 到 F 的字母等级来衡量代码。当我收到一个起始等级为 D 的 changelist 时，我会努力帮助作者把它提升到 C 或 B−。不完美，但足够好。

理论上，可以把 D 提升到 A+，但这很可能需要八轮以上的审查。到最后，作者会恨你，再也不想把代码发给你审查。

你可能会想：“如果我接受 C 级代码，最后得到的代码库岂不是也是 C 级？”幸运的是，不会。我发现，当我帮助队友把代码从 D 提升到 C 后，他们下一次发给我的 changelist 起始等级就会是 C。几个月之内，他们发来的审查会以 B 开始，并在审查结束时变成 A。

F 级留给功能不正确，或者复杂混乱到让你无法确信其正确性的代码。几轮审查之后，如果代码仍然是 F 级，你才应该拒绝批准。请参阅下面关于僵局的章节。

限制对重复模式的反馈

当你注意到作者的多个错误属于同一种模式时，不要把每一个实例都标出来。你不想花时间把同一条评论写 25 遍，作者当然也不想阅读 25 条重复评论。

指出两三个不同的模式实例没有问题。超过这个数量后，只需让作者修复这种模式，而不是修复每个具体出现的位置。

尊重审查范围

我经常看到一种反模式（anti-pattern）：审查者发现 changelist 附近的某段代码有问题，要求作者修复。作者照做后，审查者通常又会意识到代码虽然更好了，但风格不一致，因此还需要再做几处小修改。然后又是几处。如此不断，直到一个范围原本很窄的 changelist 扩大到包含大量无关改动。

如果一只饥饿的小老鼠出现在你家门口，你可能会想给它一块饼干。如果你给它一块饼干，它就会要一杯牛奶。它会想照照镜子，确认自己没有留着牛奶胡子，然后会要一把剪刀给自己修剪一下……
——Laura Joffe Numeroff（劳拉·乔夫·努梅罗夫），《如果你给老鼠一块饼干》

一条经验法则是：如果 changelist 没有触及某一行，那一行就不在范围内。

下面是一个例子：

即使代码库里的魔法数字和荒谬的变量名让你整晚无法入睡，它们也不在审查范围内。即使附近代码的作者就是当前作者，也仍然不在范围内。如果糟糕得离谱，就提交一个 bug 或者自己提交修复，但不要在这次审查中把它强行塞到作者的待办事项里。

例外情况是，changelist 虽然没有直接触及周围代码，却影响了它，例如：

在这种情况下，应指出作者需要把函数从 ValidateAndSerialize 重命名为 Serialize。他们没有修改包含函数签名的那一行，但他们仍然导致该签名变得不正确。

如果我的评论不多，但注意到范围外有一个很容易修复的问题，我会适度打破这条规则。此时我会明确说明，如果作者愿意，完全可以忽略这条评论。

寻找拆分大型审查的机会

如果你收到的 changelist 超过约 400 行代码，就应该鼓励作者把它拆成更小的部分。超出这一限制越多，你就应该越强硬地要求拆分。我个人拒绝审查超过 1,000 行的 changelist。

作者可能会抱怨拆分 changelist 很麻烦，因为这确实是一项乏味的工作。你可以通过找出合理的拆分边界来减轻他们的负担。最简单的情况是 changelist 独立修改了多个文件。此时，他们只需把 changelist 拆成更小的文件集合。更困难的情况下，找出抽象层次最低的函数或类。让作者把这些内容移到单独的 changelist 中，等第一个 changelist 合并后，再回头处理其余代码。

代码质量较低时，要明确而强烈地要求拆分。随着规模增大，审查糟糕代码的难度会呈指数增长。与其审查一个 600 行的单一怪物，不如审查几个 300 行的草率 changelist，后者要好得多。

给予真诚的赞扬

大多数审查者只关注代码有什么问题，但审查也是强化正面行为的宝贵机会。

例如，假设你正在审查一位不擅长写文档的作者的代码，却看到了一条清晰、简洁的函数注释。让对方知道这次做得很棒。如果你在他们做对时告诉他们，而不是只等到他们犯错时再挑毛病，他们会进步得更快。

你不需要为了某个具体目标才给予赞扬。每当我在 changelist 中看到令我欣喜的内容，都会告诉作者：

“我之前不知道这个 API，真的很有用！”
“这是个优雅的解决方案。我从来没想到过这样做。”
“拆分这个函数是个好主意。现在简单多了。”

如果作者是初级开发者，或者刚加入团队，他们在审查过程中很可能会感到紧张或产生防御心理。真诚的赞美能够缓解这种紧张，让他们知道你是支持他们的队友，而不是残酷的守门人。

剩余修改很琐碎时就批准

一些审查者误以为，必须等看到最后一条评论都修复后才能批准。这会平白增加代码审查轮次，浪费作者和审查者双方的时间。

在以下任一情况成立时，都可以批准：

你没有更多评论。
你剩下的评论只涉及琐碎问题。
- 例如，重命名变量、修正拼写错误
你剩下的评论只是可选建议。
- 请明确标记为可选，这样你的队友就不会以为必须完成这些建议你才会批准。

我见过审查者因为作者漏掉了代码注释末尾的句号而拒绝批准。请不要这样做。这会向作者传达一种信号：你认为除非有人监督，否则他们连简单的标点都加不好。

在仍有未处理评论时批准，确实存在一些风险。我估计，大约 5% 的情况下，作者会误解最后一轮评论，或者完全漏掉它。为降低这种风险，我只需检查作者获批后的修改。在少数发生沟通误会的情况下，我要么跟作者跟进，要么自己创建一个 changelist 来修复。与其给另外 95% 的情况增加不必要的工作和延误，不如只在这 5% 的情况下增加少量工作。

主动处理僵局

代码审查最糟糕的结果就是陷入僵局：你拒绝在没有进一步修改的情况下签字批准 changelist，而作者也拒绝做出这些修改。

以下迹象表明你可能正走向僵局：

讨论的语气越来越紧张或充满敌意。
每轮审查的评论数量没有呈下降趋势。
你提出的评论中，有异常多的内容遭到了反驳。

把话说开

当面见面，或者进行视频聊天。文字交流很容易让你忘记，对话的另一端是真实的人。你会变得很容易把队友想象成一个顽固或无能的人。一次会面会帮你和作者双方打破这种错觉。

考虑进行设计审查

充满争议的代码审查可能说明流程前期存在不足。你们争论的事情，是不是本应在设计审查中讨论？到底有没有进行过设计审查？

如果分歧的根源可以追溯到高层设计选择，那么应该让更广泛的团队参与讨论，而不是把它交给碰巧参与这次代码审查的两个人。和作者谈谈，以设计审查的形式把讨论扩展到团队其他成员。

让步或升级

你和队友在僵局中纠缠得越久，对你们关系的伤害就越大。如果其他办法都没能打破僵局，你们的选择就是让步或升级处理。

权衡一下直接批准这些修改的代价。你不能随意接受低质量代码，然后还指望构建出高质量软件；但如果你和队友争吵得如此激烈，以至于再也无法共事，同样不可能实现高质量。批准这个 changelist，真的会有多糟？它是否可能摧毁关键数据？还是说它只是一个后台进程，最坏的结果也不过是任务失败，需要开发者调试？如果情况更接近后者，不妨直接让步，这样你们还能保持良好关系，继续与队友合作。

如果无法让步，就和作者讨论把问题升级给团队经理或技术负责人。主动提出改派另一位审查者。如果升级处理的结果对你不利，接受决定然后继续前进。继续争执只会拖延糟糕的局面，让你显得不专业。

从僵局中恢复

混乱的审查争论往往与其说是代码问题，不如说是相关人员之间关系的问题。如果你们已经陷入或接近僵局，却不处理潜在冲突，这种模式还会重演。

和你的经理讨论情况。
- 如果团队中存在冲突，你的经理应该知道。也许作者只是难以共事。也许你也在以自己没有意识到的方式促成这种局面。优秀的经理会帮助你们双方处理这些问题。
暂时离彼此远一点。
- 如果可能，几周内避免互相发送代码审查请求，直到双方冷静下来。
学习冲突解决。
- 我发现《Crucial Conversations（《关键对话》）》这本书很有帮助。书中的建议听起来可能只是常识，但在你没有处于争论的激烈情绪中时，分析自己处理冲突的方式，价值非常大。

重新审视我最糟糕的一次代码审查

还记得我和 Mallory 的那次代码审查吗？为什么我的审查变成了在消极攻击式泥潭中苦熬三周，而 Bob 的审查却轻松地在两天内完成？

我做错了什么

这是 Mallory 加入团队后的第一次审查。我没有考虑到她可能会觉得自己正在受到评判，或者产生防御心理。我本应先从高层次评论开始，逐步深入，这样她就不会因为大量评论而觉得自己遭到了突袭。

我本应更努力地表明，我的工作不是阻碍她，而是帮助她推进工作。我可以提供代码示例，或者指出她的 changelist 中做得好的地方。

我让自我影响了审查。我花了一年时间努力修复这个老系统，让它恢复健康。突然来了一个新人在上面胡乱折腾，而她却懒得认真对待我的担忧？我把这视为一种冒犯，但这种态度适得其反。我本应保持自己努力带入每次审查的客观心态。

最后，我让僵局持续得太久了。几轮之后，我本应清楚地意识到我们没有取得有意义的进展。我本应做出重大改变，比如当面会谈，处理更深层的冲突，或者把问题升级给我们的经理。

Bob 做对了什么

Bob 一开始就拆分审查，这一招非常有效。记住，这次审查已经痛苦地停滞了三周。突然之间，两部分代码被合并进去了。这让 Mallory 和 Bob 都感觉不错，因为它确立了向前推进的势头。剩下的部分仍然存在问题，但它变成了一个更小、更容易管理的 changelist。

Bob没有试图把审查扼杀在追求完美的过程中。他可能也发现了我大声指责的那些问题，但意识到 Mallory 还会在团队里待很久。他在短期内表现出的灵活性，为长期帮助 Mallory 提高质量创造了条件。

结语

我发表这篇文章的上半部分后，有几位读者对我推荐的沟通方式提出了异议。有些人觉得那种方式居高临下，另一些人担心它过于间接，可能导致沟通误会。

这些反馈合理且在意料之中。有人可能会觉得简短的审查评论生硬或无礼，另一个人却可能认为同样的评论简洁高效。

在审查代码时，你会做出许多选择：关注什么、如何组织反馈、何时批准。重要的不是你选择我的选项，而是要意识到，选择确实存在。

没有人能给你一份完美审查的配方。最有效的技巧取决于代码作者的个性、你与对方的关系，以及团队文化。认真思考代码审查的结果，不断改进自己的方法。当你遇到紧张局面时，退一步想想它为什么会发生。关注审查的质量。如果你觉得自己无法让代码达到质量标准，就想想审查流程中的哪些方面妨碍了你，以及如何解决这些问题。

祝你好运，也愿你的代码审查都充满人性。