【译】如何用人类的方式进行Code Review(二)

2,998 阅读17分钟

正文

这是我文章的后半部分,关于如何在 Code review 中进行良好的沟通,避免陷入一些潜在的陷阱。这里,我会着重于介绍一些技巧,让你的 Code review 能够顺利完成,避免磕磕碰碰。

第一篇文章中,我介绍了很多基础的东西,所以我更推荐从那里开始读。当然如果你没有耐心,那么这里用一句话概括一些:一个好的代码评审者,不仅需要找出代码中的 bug,还需要提供认真负责的反馈来让团队伙伴得到能力上的提升。


我最糟糕的一次 Code review

我最糟糕的一次 Code review 经历,是和一个叫 Mallory 的前同事有关。她早我几年加入公司,但最近才转到我的团队来。

Review 的经历

当我收到 Mallory 的第一份变更列表,里面的代码是很粗糙的。她之前完全没有写过 Python 代码,而且她的代码是基于一个我维护的笨重的老系统上开发的。

我很尽职地记录下了所有我找到的问题,一共有 59 个。按照其它 Review 文章的标准来看,我做得很不错。我找到了如此多的错误,所以我肯定是个优秀的评审者。

过了几天后,Mallory 给了我一份更新过的变更列表,已经针对我之前的 Review 修改过了。她修复了一些简单的问题,比如拼写错误、变量重命名等等,但她拒绝修改任何更高层次的问题,比如她的代码对于错误的输入会产生不确定的行为,还有她的一个函数嵌套了 6 层逻辑。她拒绝修改这些东西,轻蔑地解释说,这些东西不值得花工作时间去修复。

我看到这些,心情有些恼怒,所以又写了一轮新的评论。我的语气很专业,但是不可避免的有些火药味。“你能解释一下,为什么我们需要对于错误的输入会产生不确定的行为吗?”正如你所猜的那样,Mallory 的回复比之前更固执了。


恶性循环

一周后的星期二,Mallory 和我依然停留在第四轮相同的 Review 上。我前一天晚上提交了我的新一轮评论,但实际上我是故意拖了一天才提交的,因为当她读这些评论时,我不想跟她呆在一个屋子里。

整个早上,我一直觉得胃不舒服,因为我恐惧又要开始新一轮 Review。当我吃完午饭回来时,发现 Mallory 已经提交了新的代码,但她已经不在位置上了,我估计她也不想呆在我身边看着我审查这些更新。

我读了代码之后,我的心剧烈地跳动,因为真的被她惹怒了。我立刻开始奋力敲击我的键盘,指出她既没有按照我的要求作出修改,也没有用任何理由说服我批准这个变更列表。

我们就这样毅种循环(误)了整整三个星期,而代码几乎没怎么改动过。


介入

幸运的是,我们团队里最高级别的工程师,Bob,帮我们打破了这个循环。他刚刚休完长假回来,惊讶地发现我们两个正在把 Code review 相互扔来扔去。他意识到这是一个僵局,所以请求我们让他接管这个 Review,我们都同意了。

Bob 在最开始时,先让 Mallory 创建了几个新的变更列表,把我们俩之前争吵从来没有提到过的两个小型库分隔开,每个大概 30 - 50 行。Mallory 做完之后,Bob 立刻批准了。

然后,Bob 回到主要的变更列表上来,这个列表现在已经被裁剪成 200 行左右的代码。他给了一些小建议,Mallory 响应修改了。再然后,他批准了这个变更列表。

Bob 的整个 Review 就花了两天。


沟通很重要

你或许已经发现,这儿产生的冲突其实并不是关于代码的。Code review 里提出的问题都是合情合理的,它们也明显可以被沟通能力强的团队成员解决。

这是一个很不愉快的经历,但我很高兴能回想起来,因为它让我重新思考了我的 review 方法是否恰当,并从中找到可以改进的地方。

下面,我会介绍一些技巧,以避免类似的不愉快经历。然后我会回到 Mallory 这个例子上,解释为什么我之前的方法是不好的,而为什么 Bob 做得那么好。


一些技巧

旨在把代码改进一到两个级别

即使你的团队伙伴,理论上,想找到任何机会来让他们的代码变得更好,但他们的耐心也依然是有限的。当你经过一轮又一轮的 review 之后,依然不肯批准变更,他们的心情会变得很沮丧,因为你总是在不停地想各种方法来改进他们的代码。

我个人会把代码分几个级别,从 A 到 F。当我收到一份评分为 D 的变更列表,我会尝试帮助作者把它改进到 C 或者 B-。不是完美的,但足够好了。

当然,把一份 D 评分的代码改进到 A+ 在理论上是可能的,但它可能会需要八轮 review。最后,代码作者会怨恨你,并且未来再也不给你任何 review 的机会了。

你可能会想,“如果我批准了 C 等级的代码,那最后代码库里不就全都是 C 等级的代码了吗?” 其实不然,我发现,如果我帮助团队成员从 D 改进到 C,那么他们的下一份变更列表就会从 C 等级开始。几个月后,他们给我的 review 都是从 B 等级开始了,这些改进之后就会变成 A 等级。

当然有一个特例,那就是 F 等级,这个等级是保留给一些写得太烂的代码的,一般是功能不正确,或者写得太错综复杂,以致于你无法判断代码的正确性。你驳回它的唯一理由应该是,它经过了几轮 review 之后,依然没有任何改进。这时,请参考下面的关于僵局的部分。


对于相同的问题,有限度的反馈

当你发现了作者很多相同的问题,不需要把每一处都标明,你肯定不想把同样的评论重复 25 次,代码的作者也不想读 25 次相同的评论。

对于同样的问题,只需要重复注明 2- 3 次,然后剩下的,就直接评论说让作者修复类似的问题就好了,而不是去注明每一处问题。


遵守 review 的范围

有一个我经常见到的反模式,那就是评审者会评论变更列表附近的一些代码,并要求作者修改它们。作者修改后,评审者通常会觉得代码确实更好了,但和别处的代码有一些不一致,于是又要求进一步的更改,以及再进一步的更改。最后让变更列表扩展得很大,包括了很多不相关的东西。

如果一只饥饿的小老鼠出现在你的家门口,你可能会想给他一块饼干。如果你给他一块饼干,它又会想要一杯牛奶。然后它会想要一面镜子,以确保它的胡子上没有粘上牛奶,然后它会要一把剪刀给自己剪剪胡子...

—— Laura Joffe Numeroff, If You Give a Mouse a Cookie

所以有一条准则:如果变更列表没有涉及到这一行,那它就是超出 review 范围的。

下面就是一个例子:

即使你失眠了一整晚,被这个魔术数字和荒唐的变量名所困扰,它也是超出范围的。即使写下这一行代码的人就是本次 review 的作者,它也是超出范围的。如果它真的非常糟糕,那也请你提一个 bug,或者提交你的修复,但不要把它放到本次 review 的范畴中来。

当然有个例外,那就是当变更列表影响了周围的代码时,比如:

这个例子里,需要让作者把 ValidateAndSerialize 重命名为 Serialize。虽然这超出了变更列表的范围,但它会导致不正确的命名。

当我没发现问题,但发现了范围外的某个很容易修复的问题时,我会不遵守这个规则。在这种情况下,我会明确地表示,作者可以完全无视它。


尝试切分体积很大的 review

当你收到了一个约 400 行代码的变更列表,你应该鼓励作者把它切分到更小的块。超过得越多,你就越应该打回这个变更列表。我个人拒绝 review 任何超过 1000 行的变更列表。

作者对于 “切分变更列表” 这件事可能会有些怨言,因为这是个很乏味的任务,所以作为评审者的我们最好为作者划出要切分边界,以减轻他们的负担。最简单的一种情况是,变更列表涉及到多个文件,这时便可以按文件切分为多个更小的变更列表。当然也会有更复杂一些的情况,这时也许需要找到抽象层次比较低的函数或者类,然后要求作者把它们移动到另一个变更列表里,等另一个变更列表合并之后,再回来看现在的这一个。

当代码写得很糟糕时,就更应该要求切分。因为这时 review 的难度随着代码的长度呈指数级增长。Review 多个 300 行的变更,比 review 单个 600 行的变更,要好得多。


真诚的表扬

大多数评审者都只关注代码中的错误,但忘记了 review 也是一个促进积极行为的好机会

比如,你正在 review 的这个作者是个文档写得很挣扎的人,但你却看到了一条清晰、简洁的注释,这时请表扬他一下。当你告诉他们什么做得对时,他们会进步得更快,而不是等着他们犯错才告诉他们。

你不需要刻意地去表扬作者,每当我看到变更列表中有任何亮点,我都会告诉作者:

  • “我没想到还有这个 API,它用处很大!”
  • “这是个很优雅的解决方法,我之前还没想到可以这样做。”
  • “这个函数重构得很不错,它现在简单得多了。”

如果作者是个最近才加入团队的萌新,他们在 code review 的时候可能会感到紧张和焦虑。为了缓解这种情绪,你需要真诚地表扬他们,证明你是支持他们的同伴,而不是残酷无情的代码守门员。


当剩下的都是些小问题时,直接批准

有些评审者会有一个错误的观念,他们总会一直不肯给出批准,直到他们看到每一条评论都得到了修复。这增加了不必要的工作,也浪费了作者和评审者的时间。

当有下面这些情况时,可以直接批准变更:

  • 你没有更多的评论想写了
  • 剩下的评论都是些无关紧要的小问题,比如变量命名、拼写错误
  • 剩下的评论都是 “可选的”(记得明确地标明这些评论不是一定要修复,这样你的小伙伴才不会认为一定要改掉这里才能让你批准)

我之前看到过有些评审者一直不肯给出批准,因为作者在最后的评论后就没再有任何回应了。请不要这样做,这会让作者觉得你认为他们连加一个标点符号都应该被监视着。

当还剩一些很重要的问题没修改时,直接给出批准可能会很危险。根据我的经验,大约有 5% 的概率会发生这样的事,要么作者误解了最后一轮评论的意思,要么他完全没看到。为了解决这种情况,我会去检查一遍要批准的变更。如果真的是因为罕见的沟通不畅,我要么会继续跟进作者,要么会自己创建一个新的变更来修复问题。在 5% 的情况下增加少量的工作,要好过于,在另外 95% 的情况下加入更多不必要的精力和拖延。


主动处理僵持状态

在 code review 中,最糟糕的情况就是僵持:如果没有进一步更改的话,你不想批准这个变更列表,而代码的作者拒绝修改它。

下面这些指标,表示你正在走向僵持状态:

  • 讨论的语气变得越来越有敌意和火药味
  • 你的每一轮评论都不是自上而下的了
  • 你回复的评论异常之多

说出来

直接面对面交流或者通过网络视频。文字交流最后会让你忘了你对话的是一个真正的人,很容易让你想象你的同伴是一个来自无能与固执之地的人。面对面的交流将会打破这个魔咒。

考虑设计阶段的审查(design review)

可能会进入僵持状态的 code review 可能在更早的时候就有一些征兆了。你们现在是不是在一些事情上争吵,而这些事情本应该在设计审查时讨论的?你们有设计审查吗?

如果分歧的根源是高层次的设计问题,那就应该是由更大的团队来权衡利弊,而不是交给 code review 的两个人。你应该和作者在 review 中讨论,同时开放给团队其他成员,以设计审查的形式让他们也进来讨论。

让步或者升级

你和你的同伴在 review 中僵持得越久,就越有害于你们之间的关系。如果问题一直没有得到解决,那么你最好选择让步,或者把问题升级。

学会衡量给出批准的成本。当你总是接受低质量代码时,你的软件质量当然不可能好,但当你和你的团队伙伴痛苦地工作,以至于你们无法在一起愉快地合作时,软件质量同样也不会有任何提高。这个时候应该想想,如果你批准了变更列表,结果会有多糟?这些代码会破坏重要的数据吗?它是一个后台进程,并且只有当最坏的情况发生时才需要开发人员对它进行 debug 吗?如果更偏向于后者,那么可以考虑批准变更,以便让你和你的团队伙伴继续工作。

如果你不想做出让步,请告诉作者,将这里的争议升级到团队的管理者或者技术负责人那里,把 review 重新分配给另外的评审者。即使之后得出的结果和你之前的意见相悖,也要接受它。继续固执下去只会导致更不好的结果,让你看起来很不专业。

脱离僵局之后

一次棘手的 review 中的争吵,真正的重点很可能并不是代码,而是人和人的关系。如果你陷入了僵局或者即将陷入僵局,并且不去解决那些潜在的冲突,那么这种情况会一直发生下去。

  • 与你的上级讨论这种情况
    • 如果团队中产生了冲突,你的上级应该会知道。也许是因为代码作者本来就是个很难合作的人,也许是因为你们只是用了相互了解的方式在解决当前的状况。一个好的上级会帮你们解决这些情况。
  • 休息一下
    • 如果可能的话,相互之间停止代码评论几个星期,直到两边都冷静下来。
  • 学会解决冲突
    • 有一本书叫 "Crucial Conversations" 很不错。虽然它给出的建议似乎都是些常识性的东西,但当你作为一个旁观者来看冲突时,就会发现它分析冲突的方法真的很棒。

重新思考我最糟糕的那次 Code review

还记得我和 Mallory 的那次 code review 吗?为什么我的 review 花了整整三周,并且成效甚微,而 Bob 仅花了两天就搞定了?

我做错了什么

这是 Mallory 在我们团队的第一次 review。我之前并没有考虑到她可能会感觉自己受到评判,产生戒备心。我应该在最开始的时候给出一些高层次的评论,让她不会因为大量的评论而感到挫折。

我应该更好地向她表明,我的工作不是去阻碍她的工作,而是帮助她变得更好。我应该给出更多的代码示例,以及在她的变更列表中给出更多的积极因素

我的自尊心也过多地影响了这次 review。要知道我花了一整年的时间,才把这个老系统修复到健康状态,突然有了一个新人开始在这个系统上做些事情,而她就不再需要考担心这些我曾经担忧的问题了吗?我认为这是一种冒犯,也就是这种态度产生了副作用。我应该在所有的评论上都保持一个客观的态度。

最后一点,我让这个僵局持续太久了。在几轮 review 过后,我们都应该很清楚,我们没有任何有意义的进展。这时我应该主动做出改变,比如主动当面交流,以解决深层次的冲突,或者把它升级到我们的上级那里。

Bob 哪些地方是对的

Bob 第一步把 review 切分的做法非常有效。这个 review 已经花费了痛苦的三周时间,忽然,其中的两块代码被合并了。这让 Mallory 和 Bob 都感觉到激励,因为它产生了向前的动力。尽管剩下的代码里依然存在问题,但它变成了一个更小的,更好管理的变更列表。

Bob 没有尝试在 review 的过程中把代码改造成完美无缺的,他可能已经意识到那些代码里我觉得无法忍受的问题,但他也同样意识到 Mallory 会待在我们团队一段时间。对于某些特殊情况的灵活处理,让他可以在长时间内帮助 Mallory 提升质量。


结论

在我发表这篇文章的前半部分之后,有些读者对我鼓励的沟通风格产生了一些质疑。有人觉得它太屈尊卑微了,有人觉得这种风格太委婉了,会产生误解。

这样的反馈是合情合理的。对于同样的一条评论,有人可能会觉得它很粗鲁无礼,而有的人却会觉得它很简洁高效。

当你在 review 代码时,你会面临很多选择:该关注什么,怎么写反馈,什么时候给出批准。对于这些选择,你是否遵循我的建议其实不重要,只要你记得有这些选择就好。

没有人可以给你一个完美的 code review 指南。什么是最好的技巧,取决于代码作者的性格、你和他们的关系还有你们团队的文化。不断思考你的 code review 会产出什么结果。当你遇到任何紧张形势时,先退一步思考为什么会发生。关注你 review 的质量。如果你觉得代码质量很难提升到符合你的标准,应该思考 review 的那些过程阻碍了你,以及如何解决它们。

最后祝好运,希望你能用人类的方式进行 code review。