如何像人类一样进行代码评审（第二部分）

这是我关于如何在代码评审中进行有效沟通并避免陷阱的文章下半部分。在这里，我将重点放在如何在避免不愉快冲突的前提下，让代码评审顺利结束。

我在第一部分中打下了基础，所以我建议先从那里开始阅读。如果你没耐心，这里有简短总结：一个好的代码审查者不仅仅是发现 bug，还要提供细致入微的反馈，帮助团队成员提升能力。

我经历过的最糟糕的代码评审

我有生以来经历过的最糟糕的代码评审，来自一位以前的同事，我们称她为 Mallory。她在我加入公司前好几年就已经入职，但不久前才转到我的团队。

评审过程

当 Mallory 把她的第一个变更集（changelist）发给我评审时，代码显得有些粗糙。她之前从未写过 Python，而且她在我维护的一个复杂、老旧的系统上继续开发。

我尽职尽责地记录下我发现的所有问题，总计 59 处。根据我看过的代码审查资料，这应该是一次很成功的审查——我发现了如此多的错误，因此我一定是个不错的审查者。

几天后，Mallory 发来了更新后的变更集，以及她对我审查意见的回复。她修正了那些简单的问题：拼写错误、变量重命名等。但对于更高层次的问题，她拒绝修复，比如她的代码在处理不合规输入时会有未定义行为，或者她的某个函数有六层嵌套的控制流结构。相反，她用一种不以为然的态度解释，这些问题不值得投入更多的工程时间去修复。

我感到又生气又沮丧，接着发了一轮新的审查意见。虽然我的语气依然保持专业，但已经有了被动攻击（passive-aggressive）的味道：“你能解释一下为什么我们想让不合规的输入产生未定义行为吗？” 你可以想象，Mallory 的回复更是让人难以接受。

苦涩的循环

那是周二，也是一周之后。Mallory 和我依然在为同一个评审互相扯皮。我在前一天晚上给她发了最新的审查意见。我特意等到她下班后才发，因为我不想在她读那些意见时跟她待在同一个房间。

那天一早，我就一直感到心里很沉重，担心下一轮代码评审的到来。午饭回来后，我发现 Mallory 不在她的位置，但她给我发了新的代码更改。我猜她也不想在我看到她回复时待在办公室。

当我看到她的回复，一条条看下去，心跳开始加速，每读一条都更生气。我立刻猛敲键盘开始反驳，指出她既没有按照我的建议去改动，也没有给出能让我同意通过的理由。

我们几乎每天都重复这样的模式，持续了三周。代码几乎没有实质变化。

干预

幸好我们团队里级别最高的同事 Bob 打破了这个循环。他度完一个长假回来后，看到我们正陷在没完没了的代码评审争执之中，感到很惊讶。他立刻认定这是一个僵局，并提出由他来接手评审。我们都同意了。

Bob 的第一步是让 Mallory 把一些小的库文件拆成新的变更集，分别是 30～50 行左右的两块代码。这些并不是我们之前争论的重点。Mallory 把它们单独拆分后，Bob 立刻通过了这两个变更集。

接着，Bob 回过头来处理主要的变更集，缩小到大约 200 行的代码。他提了几条小建议，Mallory 也做了修改，然后他就通过了这个变更集。

Bob 的整个评审只花了两天。

沟通才是关键

你可能已经看出来，这场冲突其实根本不是因为代码本身。确实存在一些问题，但这些问题完全可以通过有效沟通的团队来解决。

虽然这是一次不愉快的经历，但现在回想，我还是很感谢它。它让我重新审视自己在评审时的做法，并找到了需要改进的地方。

下面我会分享一些技巧，以减少你遇到类似糟糕情况的风险。稍后我还会回到 Mallory 的故事，解释为什么我最初的做法是错误的，以及为什么 Bob 的方法堪称“默默的高明”。

技巧

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

虽然从理论上说，你的团队成员也许愿意听到所有可能改进的地方，但他们的耐心是有限的。如果你因为不断冒出的“新灵感”而在一轮又一轮评审中拒绝通过，最终他们会对这个流程深感厌烦。

我在脑子里会用 A 到 F 来给代码打分。如果我收到一个起点在 D 的变更集，我会帮助作者把它提高到 C 或者 B-。这不完美，但已经够好了。

当然，理论上也可以把一个 D 提升到 A+，但可能需要来回评审 8 轮以上。到了那时，作者会恨透你，再也不想让你评审他们的代码。

你可能会想：“如果我接受了 C 级别的代码，那整个代码库不就变成 C 级别了吗？” 其实不会。当我帮助某位同事把 D 提升到 C，下次他们交给我的变更集就会从 C 开始。几个月后，他们交来的变更集可能一开始就是 B，评审完就是 A 了。

而 F 级代码通常指功能上不正确，或者结构过度复杂到无法保证其正确性。只有当在几个来回之后，代码依然停留在 F 的水平，你才有必要拒绝通过。详见下面关于僵局的部分。

不要对重复出现的模式做过多反馈

如果你注意到某些问题反复出现，不要给每一个实例都提出相同的意见。你自己也不想写 25 条一模一样的评论，作者也不会愿意读 25 条重复的内容。

指出两到三个具体实例就够了，随后让作者去修改整类问题，而不是对每一次出现都做指出。

尊重评审的范围

我常见到的一个反面例子是：审查者发现变更集附近的代码有问题，就让作者一起修复。作者修复后，审查者意识到代码虽然变好了，但和其他部分又不一致，于是再提出一些小改动，再改动，再改动……一个小范围的变更集被迫扩展得面目全非。

如果一个饥饿的小老鼠出现在你家门口，你也许想给它一块饼干。如果你给了它饼干，它就会想要一杯牛奶。接着它会想照镜子，看看自己有没有牛奶胡子，然后还要找把剪刀来修理一下自己的毛发……
—— 摘自 Laura Joffe Numeroff 的 If You Give a Mouse a Cookie

总的原则是：如果变更集中没有改到那一行，就不属于这次评审的范围。

看个示例：

即使你会因为这行魔数和荒谬的变量名而夜不能寐，也依然不在本次评审范围内。即使作者本人就是写这段代码的人，也依然不在范围。如果觉得它很糟糕，可以提个 bug 或者自己去修，而不是强塞给作者。

但有一种例外：当变更集的改动直接影响了邻近的代码，即使没有改到那一行，例如：

在这种情况下，作者需要把这个函数的名字从 ValidateAndSerialize 改成 Serialize，因为他们对函数行为进行了改变，导致原函数名不再准确。

如果我的审查意见很少，但附近某行有一个小问题很容易修复，我会“轻度”打破上述原则。并且会明确告诉作者，如果他们不想改也没关系。

寻找机会拆分大型评审

如果你收到一个超过约 400 行代码的变更集，应该鼓励作者将其拆分成更小的部分。超过这个行数越多，就越要强烈要求拆分。我个人会直接拒绝审查任何超过 1000 行的变更集。

作者可能会抱怨拆分变更集是个麻烦事。你可以通过帮他们找出逻辑拆分点来减轻他们的负担。如果变更集独立改动了多个文件，那么最简单的方式就是把这些文件分成几个小变更集。如果情况复杂，就先找到最低层抽象的函数或类，把它们放到单独的变更集里，然后等这个小变更集合并后，再回过头来审查剩余代码。

如果代码质量较差，那么你应该更加强调需要拆分。糟糕代码的体量越大，评审难度就会呈指数级上升。审查两个各 300 行的糟糕变更集，要远比审查一个 600 行的“巨无霸”更容易。

真诚地给予赞扬

大多数评审者只会关注代码里有什么“不好”的地方，但评审其实也是一个强化积极行为的好机会。

举个例子，如果你在评审时看到一个一直文档写得很差的人，这次却写出了一段又清晰又简洁的函数注释，那就告诉他做得好。比起等他们犯错再指出来，让他们知道自己什么时候做对了，通常能更快地帮助他们改进。

你也不一定需要有什么明确目的，只要在评审中看到让你感到“不错”的地方，就可以告诉作者：

“我之前不知道有这个 API，这真是太有用了！”
“这是个很优雅的解决方案，我完全没想到。”
“把这个函数拆开写是个好主意，现在简洁多了。”

如果对方是初级工程师或者刚加入团队，他们在评审里通常会感到紧张或容易防御。真诚的赞扬能让对方意识到你是支持他们的队友，而不是一个冷酷的“守门人”。

当剩余问题很小或微不足道时，给予通过

有些评审者会误以为必须等作者修复了最后一个问题才能给予通过，哪怕只是少了句号。这会让双方浪费时间。

如果满足以下任一情况，你都可以给予通过：

你已经没有更多意见要提了。
剩下的问题是微不足道的小问题。
- 比如变量命名、拼写错误等。
剩下的是可选的建议。
- 你应明确注明这些建议是可选的，这样作者不会以为必须改了才能得到通过。

我曾见过评审者因为作者少打了一个句号就不给通过，请千万别这样。这会让作者觉得，除非你盯着看，否则他们连一个简单的标点都不会加。

当你在还有少量未修复问题的情况下就给通过，确实有一定的风险。我估计大约有 5% 的情况下，作者要么误解了最后一条意见，要么完全忽略了。为此，我会在通过后再稍微检查一下作者提交的最终改动。如果真有沟通失误，我会直接找作者沟通，或者自己提交一个修复。只在 5% 的情况下多花点功夫，远比把额外的工作量和延时加在剩余 95% 的情况下要好得多。

主动处理评审僵局

评审中最糟糕的结果就是僵局：你拒绝通过，除非作者再做修改；作者则拒绝修改。

以下是一些僵局即将到来的征兆：

讨论的语气开始变得紧张或敌对。
每轮评审的意见数量没有减少的趋势。
你提出的许多意见都遭到强烈抵触。

面对面交流

当面或者开视频会议聊一聊。纯文字的沟通很容易让人忘记对方也是有血有肉的人，你会觉得对方只是顽固或无能。见面可以打破这样的想象，对彼此都会有好处。

考虑做一次设计评审

如果在代码层面争执不下，可能是更早阶段的流程出了问题。你们是否在设计阶段讨论过这些问题？有没有进行过设计评审？

如果根源在高层次的设计决策上，那么就不应只让评审者和作者两个人争执，而是应该让更广泛的团队参与进来。你可以建议作者把讨论升级到团队层面的设计评审。

让步或升级问题

拖延僵局只会让你和队友之间的关系进一步恶化。如果其它手段都无法打破僵局，那就只剩下让步或升级。

衡量一下直接同意通过的成本如何。如果你随便放行低质量代码，就没法做出高质量软件，但如果你们因为争执而彻底闹僵，也无法做出好软件。如果批准这个变更集的后果其实没那么严重，最多在出现问题时需要某位开发者去排查，比如只是一个后台任务，出了错也不会造成核心数据损坏，那么你可以考虑做出让步，这样至少能保持你们之间的良好合作关系。

如果让步不行，那就建议作者与自己一起把问题提交给团队的经理或技术负责人。或者尝试换一个评审者。如果上级的决定不支持你，那你就得接受结果继续前进；如果你仍然纠缠不放，只会延长这个糟糕的局面，还会显得你不够专业。

僵局之后，如何善后

糟糕的评审争执往往不是源于代码本身，而是源于评审者与作者之间的关系。如果你们已经走到僵局或接近僵局，不解决人际层面的问题，类似情况将会不断重复。

和你的经理讨论一下。
- 如果团队内部存在冲突，你的经理应该知道。也许这个作者本身就难相处，也或许你有一些自己意识不到的沟通方式问题。一个好的经理会帮助双方解决这些矛盾。
彼此间先隔离一段时间。
- 如果有可能，尽量在接下来的几周内不要相互评审代码，先让气氛缓和下来。
学习冲突解决的技巧。
- 我个人觉得 Crucial Conversations 这本书很有帮助。它的建议听上去也许很常识，但在不带情绪地分析冲突时，你能学到很多。

再看那次糟糕的评审

还记得我和 Mallory 那次代码评审吗？为什么同样的代码，我评了三周，满是被动攻击，Bob 却在两天内就通过了？

我哪里做错了

那是 Mallory 在新团队的第一次评审，我忽略了她可能会有被审视或被评判的焦虑。我本应该先从高层次的问题开始，而不是立刻抛给她一大堆问题。

我应该更多地展示我是如何帮助她推进工作，而不是在设置路障。我也可以提供一些代码示例，或者指出她变更集里做得好的部分。

我让自己的自尊心影响了评审。那套老系统是我花了一年时间维护起来的，如今来了个新人，把它搞得一团糟，还不愿意认真听我的建议？我把这当成对我工作的冒犯，但其实这种心态毫无益处。我应该保持评审时的客观态度。

最后，我让僵局拖得太久。审查来回几轮后就该看出我们毫无进展了。我应该采取更激烈的措施，比如线下面谈或找到我们的经理来解决深层次的冲突。

Bob 为什么做得好

Bob 第一步就拆分了评审，这非常高明。想想我们已经在那个评审里僵持了三周，突然有两个小的库文件被独立拆分并快速合并，这让双方都感到进度在前进。即使剩下的主变更集依然存在问题，但它被缩小到了 200 行，更容易处理了。

Bob 并没有逼着大家把评审改到十全十美。他大概也看到了那些让我抓狂的问题，但他知道 Mallory 还会在这个团队里继续干下去。在短期里保持一定的灵活度，才能在长期帮助 Mallory 提升代码质量。

结论

在我发表这篇文章的第一部分后，有些读者对我推荐的沟通风格不太认同。有人觉得这套方法听起来有点居高临下；也有人担心这太迂回，可能造成理解偏差。

这些担忧都合情合理。对有些人来说，简短的评审意见显得生硬甚至粗鲁；对另一些人来说，这同样的话却可能显得简洁高效。

在做代码评审时，你会面临很多选择：关注哪些问题，用什么口吻反馈，什么时候给通过。不必一定采用我的做法，但你需要意识到你的做法会带来什么结果。

没有人能给你一份“完美评审”的标准食谱。最合适的技巧取决于作者的性格、你与作者的关系，以及你们团队的文化。你可以通过对评审结果的反思来不断改进。当你感觉到有紧张气氛时，不妨停下来想想为什么。关注评审的整体质量。如果你觉得无法让代码达到你的质量标准，看看是哪方面的评审过程阻碍了你，又该如何改进。

祝你好运，也愿你的代码评审更有人情味。