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

2017-11-09

Michael Lynch

如何像人一样做代码评审（下）

原文由 Michael Lynch 于 2017-11-09 发布，订阅该博客

本文是我关于如何在代码评审中有效沟通、避免陷阱的文章的下半部分。在这里，我将重点介绍如何让代码评审顺利收尾，同时避免难看冲突的技巧。

我在上篇中已经打下了基础，所以建议从那里开始看。如果你等不及，这里是精简版：优秀的代码评审者不仅能找出 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 条重复的意见。

指出两三个该模式的典型例子就够了。更多的情况下，只需让作者去修正这一整类问题，而不是逐个去改每一处。

尊重评审的范围

我经常看到一种反模式：评审者在 changelist 附近发现了某个问题，就让作者顺手修掉。作者照做后，评审者又会发现代码虽然变好了，但风格不一致，还需要再做几处小改。接着又是几处，就这样没完没了，原本范围很小的 changelist 被不断扩大，掺进了大量无关的改动。

如果一只饥饿的小老鼠出现在你家门口，你可能会想给他一块饼干。而如果你给了他一块饼干，他就会想要一杯牛奶。他会想照照镜子，看看自己有没有留下牛奶胡子，然后他又会想要一把剪刀给自己修修剪剪……
-劳拉·乔菲·努梅罗夫，《如果给老鼠一块饼干》

经验法则是：如果 changelist 没有碰到那一行代码，它就不在本次评审范围内。

举个例子：

即使代码库里那个魔法数字和可笑的变量名让你夜不能寐、耿耿于怀，它也不在范围内。即使作者就是写下附近那几行代码的人，依然不在范围内。如果问题实在严重到离谱，请单独提个 bug 或自己提交一个修复，但不要在这次评审中强加给作者。

例外情况是，当 changelist 虽然没有直接改动周围的代码，却影响了它的正确性，例如：

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

如果我没什么其他意见，却在范围之外发现一个很容易顺手修复的问题，我也会柔性地打破这条规则。这种时候，我会明确表示作者完全可以忽略这条意见。

寻找机会拆分大型评审

如果你收到的 changelist 超过大约 400 行代码，就鼓励作者把它拆成更小的几块。超出越多，就越要坚决地要求拆分。我个人会直接拒绝评审任何超过 1000 行的 changelist。

作者可能会抱怨拆分 changelist 很繁琐。帮他们减轻负担的办法，是为拆分找出合乎逻辑的边界。最简单的情况是 changelist 独立地改动了多个文件，这时只需按文件拆成几个更小的集合就行。在更复杂的情况下，可以找出最底层的函数或类，让作者先把它们移到单独的 changelist 中，等第一个 changelist 合入后再回头处理其余代码。

当代码质量较低时，更要强烈要求拆分。评审糟糕代码的难度会随着规模呈指数级增长。与其去审一个 600 行的糟糕大块，不如审两个各 300 行的马虎小块要轻松得多。

给出真诚的赞美

大多数评审者只关注代码中错的地方，但评审其实也是强化积极行为的宝贵机会。

例如，假设你在为一位不太擅长写文档的作者做评审，却看到了一段清晰、简洁的函数注释，那就告诉他这次写得很棒。如果你能在他做对时及时肯定，而不是只在他搞砸时才扣分，他会进步得更快。

赞美不一定非要有明确的目的。只要在 changelist 中看到让我眼前一亮的地方，我都会告诉作者：

“我还不知道有这个 API，太有用了！”
“这个解法很优雅，我根本想不到。”
“把这个函数拆开真是好主意，现在简洁多了。”

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

当剩余修改都是小事时就予以通过

有些评审者误以为必须亲眼看到每一条意见都被修复后才能批准通过。这会平白增加评审轮次，浪费作者和评审者双方的时间。

在以下任一情况成立时，就可以批准通过：

你已经没有更多意见了。
剩下的意见都是些小问题。
- 例如，重命名变量、修正拼写错误
剩下的意见只是可选建议。
- 请明确标注这些是可选的，以免队友误以为你的通过是以采纳它们为条件的。

我见过有评审者因为作者在代码注释末尾漏了一个句号就扣住不放。请不要这样做。这会向作者传递一个信号：你认为如果没人盯着，他们连加个标点都做不到。

在仍有未处理意见的情况下就批准通过，确实有一定风险。我估计大约有 5% 的情况下，作者会误解最后一轮的意见或完全遗漏。为了规避这一点，我会在批准后简单检查一下作者后续的改动。在极少数出现沟通偏差时，我要么再去找作者跟进，要么自己提一个 changelist 来修复。给这 5% 的情况增加一点点工作量，远比给另外 95% 增加不必要的负担和延迟要好。

主动化解僵局

代码评审最糟糕的结果就是陷入僵局：你拒绝在作者进一步修改之前签字通过，而作者又拒绝做出这些修改。

以下是一些表明你正走向僵局的信号：

讨论的语气变得越来越紧张或充满敌意。
每一轮提出的意见数量没有呈下降趋势。
你的意见中有异常多的一部分遭到了反驳。

当面沟通

约个时间当面聊或视频沟通。文字交流很容易让人忘记对话的另一端是一个活生生的人，你会不自觉地以为队友是出于固执或无能才那样回应。一次会面能为你和作者同时打破这种错觉。

考虑进行设计评审

一场充满争议的代码评审，可能暴露了更早环节的薄弱之处。你们争论的问题，本该在设计评审阶段就讨论清楚吗？到底有没有做过设计评审？

如果分歧的根源可以追溯到某个高层的设计决策，就应该让更广泛的团队来参与评判，而不是把它留给恰好参与这次代码评审的两个人。可以和作者商量，以设计评审的形式把讨论开放给团队其他人。

让步或升级

你和队友在僵局中僵持得越久，对彼此关系的伤害就越大。如果其他方法都无法让你们摆脱困境，你的选择就只剩下让步或升级。

权衡一下直接批准这些改动的代价。如果轻易接受低质量代码，你无法构建出高质量的软件；但如果你和队友争得不可开交、以至于无法再合作，也同样无法实现高质量。如果批准了这个 changelist，真的会有多糟？它是那种可能毁掉关键数据的代码吗？还是一个后台任务，最坏情况不过是任务失败、需要开发者去调试？如果更接近后者，不妨考虑直接让步，以便与队友保持良好的合作关系。

如果让步不可行，就和作者商量将讨论升级到团队的管理者或技术负责人。也可以主动提出换一位评审者。如果升级后的决定对你不利，就接受它并继续前进。继续纠缠只会让糟糕的局面拖得更久，也会让你显得不够专业。

从僵局中恢复

一团糟的评审争论，往往与其说是关于代码，不如说是关于相关人员之间的关系。如果你已经陷入或接近僵局，若不解决底层的冲突，这一模式还会重演。

和你的管理者谈谈情况。
- 如果团队中存在冲突，你的管理者应该知情。也许作者本身就很难合作，也许你在不自知的情况下也在助长矛盾。一位好的管理者会帮助你们双方解决这些问题。
彼此暂时保持距离。
- 如果可能，几周内尽量避免互审代码，等气氛缓和下来再说。
学习冲突解决方法。
- 我发现《关键对话》这本书很有帮助。它的建议听起来像是常识，但在没有身处争执漩涡时，分析自己处理冲突的方式具有巨大的价值。

回顾我最糟糕的那次代码评审

还记得和 Mallory 的那次代码评审吗？为什么我的评审会变成一场在阴阳怪气泥潭中跋涉三周的苦差，而 Bob 的却在两天内就轻松搞定？

我哪里做错了

那是 Mallory 在团队中的第一次评审。我没有考虑到她可能会感到被评判或产生防御心理。我本应该一开始只提宏观层面的意见，这样她就不会因为大量意见而感到被突袭。

我本应多做一些来表明，我的职责不是阻碍她的工作，而是帮助其推进。我本可以提供代码示例，或指出她 changelist 中的亮点。

我让自尊心影响了评审。过去一年里，我一直在悉心维护、让这个陈旧的系统恢复健康。突然来了个新人对它修修补补，却连认真对待我的关切都懒得做？我把这当成了冒犯，但这种态度只会适得其反。我本应保持我在所有评审中都努力秉持的客观心态。

最后，我让僵局拖得太久了。几轮过后，我就应该清楚地意识到我们并没有取得实质性进展。我本应该做出果断的改变，比如当面沟通以解决更深层的冲突，或是升级到我们的管理者那里。

Bob 做对了什么

Bob 首先拆分评审的那一步非常有效。回想一下，那个已经痛苦地停滞了三周的评审，突然间就有两块代码被合入了。这让 Mallory 和 Bob 都感觉很好，因为它建立了前进的势头。剩下的部分仍有问题，但它已经变成了一个更小、更易于管理的 changelist。

Bob 没有试图把评审逼到完美。他很可能也看到了那些让我大呼小叫的问题，但他意识到 Mallory 还会在团队待很久。他在短期内的灵活变通，为他长期帮助 Mallory 提升质量创造了条件。

结语

在我发表本文的上半部分后，有几位读者对我推荐的沟通方式提出了异议。有人觉得它有居高临下的味道，有人则担心它过于迂回，有造成误解的风险。

这些反馈是合理且在意料之中的。同样的简短评审意见，有人可能觉得生硬、粗鲁，另一个人却可能认为它简洁、高效。

在评审代码时，你要做很多选择：关注什么、如何组织反馈、何时批准通过。重要的不是你一定要选择我的方案，而是要认识到存在多种选择。

没有人能给你一份完美评审的配方。最有效的技巧取决于代码作者的个性、你与他们的关系以及团队的文化。通过批判性地思考代码评审的结果来磨练你的方法。当你遇到紧张局面时，退一步评估其原因。关注你评审的质量。如果你觉得无法让代码达到自己的质量标准，就想想评审流程中的哪些方面在阻碍你，以及该如何解决它们。

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