开篇 — 同一条意见,不同的结果
有两条评审意见。一条是“这段你为什么这么写”,另一条是“这个函数把校验和存储放在一起做了,以后只想改校验规则的时候,怕是连存储逻辑也得动。拆成两个怎么样”。它们指向的东西完全一样。但收到第一条的人开始准备防守,收到第二条的人开始看代码。
这种差别常被解释成人品或礼貌问题,实际上大多是形式问题。不是好脾气的人才写得出好评审,而是好的形式能从普通人身上榨出好评审。所以这篇文章讲的是形式,不是心态。批准的标准、意见的前缀、句子的顺序、PR的体积 — 全都是团队用一页维基就能达成一致的东西。
评审的三个目的,以及多数人丢掉的那两个
代码评审同时在做三件事:发现缺陷、传播知识、分摊代码的所有权。可多数团队把第一件当成评审的定义,把后两件当成副产品。
有意思的是,数据指向相反的方向。阿尔贝托·巴凯利(Alberto Bacchelli)和克里斯蒂安·伯德(Christian Bird)在2013年于微软开展的研究中报告说,开发者和管理者都把发现缺陷列为评审的首要目的,但实际观测到的产出并非如此。真实意见里的大多数不是缺陷指摘,而是可读性改进、替代方案的提出,以及对代码为什么长成这样的理解共享。凯特琳·萨多夫斯基(Caitlin Sadowski)研究团队在2018年发表的谷歌案例研究也得出了相似结论。谷歌内部被认为是评审首要动机的,更靠近教育和规范维护,而不是抓虫子。
第二个目的,知识传播,是这样运作的。评审者在读代码的过程中拿到了那块领域的地图。半年后这段代码在凌晨三点炸掉时,只要当初参与评审的人还在一个,恢复时间就不一样。从这个角度看,挑评审者时只放“最懂的那个人”并不是优化。一个很懂的加一个不懂的,对整个团队来说往往更划算。
第三,所有权共享会在出事故时显形。经过评审合并进去的代码,不再是作者个人的东西,而是团队的决定。这不是逃避责任的装置,而是把失败当作系统问题而不是个人失误来处理的前提。停止归咎个人的文化为什么是韧性的条件,在把发生过的事变成资产那篇里讲过。
批准的标准 — 不是完美,而是明确变好
评审变成地狱最常见的路径,是评审者问了自己一个错误的问题。只要问“这是我会写出来的代码吗”,批准就永远不会到。
谷歌公开的工程实践文档把这一点讲得很明确。要点是:评审者不该要求代码变得完美,只要这次改动明确改善了系统整体的代码健康度,就应该批准。后面还跟着一句,世上没有完美的代码,只有更好的代码。批准要问的不是这是不是最优解,而是它有没有比合并前更好。
这个标准为什么重要?因为相反标准造成的成本很难被看见。在一个要求完美的评审者手下,人们会用两种方式回应,而且都不是把PR拆得更小。他们开始用能彻底躲开评审的方式工作:推迟重构,把那位评审者从名单里划掉,找“这个很急”的借口跳过批准。把评审质量推到极端,评审本身就被绕过去了。
不过这个标准也有例外。安全、数据一致性、难以回滚的迁移、公开API的契约 — 在这些领域,“变好了所以放行”是不成立的。把哪些必须卡住、哪些可以放过的清单在团队层面先约定好,这类判断就不会被误读成某个评审者性格难搞。
意见的语法 — 强度标记与三段式句子
评审之所以伤人,最常见的原因不是内容,而是强度没有传达到。评审者按“改了更好,不改也行”的分量写下的意见,作者读成的是“不改这个就不给批准”。挂上十条意见,十条全都会被按最高强度来读。
解法简单得出奇:加前缀。谷歌的评审文档里也明确写了给小意见加“nit:”的惯例。可以按团队习惯改成中文,三档就够。
| 前缀 | 含义 | 是否卡住合并 | 示例 |
|---|---|---|---|
| nit | 接近个人偏好的小意见 | 不卡 | nit: 这个变量名改成复数会更好读 |
| 建议 | 现在改了更好,放到下个PR也行 | 不卡 | 建议: 把这段分支逻辑抽成独立函数,测试会更容易 |
| 必须 | 合并前必须解决 | 卡 | 必须: 这条查询漏了用户ID过滤,会泄露其他账号的数据 |
| 问题 | 是在要信息,不是在下判断 | 不卡 | 问题: 这里把重试设成3次,是有什么依据吗 |
前缀带来的真正变化,也发生在评审者这一侧。要标强度,评审者就得先问自己这件事是不是真的该拦住合并,光是这个问题就能让阻塞性意见明显变少。
定好强度之后,接下来是句子结构。按观察、影响、建议的顺序写,评价性的词会自然掉出去。“这代码不太行”没有观察、没有影响、也没有建议,是纯粹的评价。而“这个函数把异常吞掉了(观察),出故障时怕是很难追原因(影响)。记完日志再抛出去怎么样(建议)”,把同样的内容变成了协作。这个结构并非代码评审专属,它本来就是通用的反馈语法。分场景的句式,我整理在在公司可以直接照着说的句子那一篇里。
再补一句:指出做得好的地方,不是客套而是信息。如果评审里只流动负面信号,作者根本无从知道该保留什么。“这个错误处理的写法,我想搬到其他模块去”这样一行,就是团队惯例形成的方式。
为评审者设计PR — 体积和说明
评审质量有相当一部分不是评审者决定的,而是作者决定的。而作者手里最大的变量是体积。
有个被广泛引用的数字:一次超过200到400行,发现缺陷的能力就会下降。出处是2006年SmartBear在思科做的案例研究。他们分析了约50名开发者、320万行代码、2500次评审规模的数据,得出的结论是:评审规模越大,每行发现的缺陷数越低;速度超过每小时500行,或者连续评审超过60分钟,效果会急剧下滑。
这份依据的局限也得一并知道。它是来自一家公司、一个代码库、2006年工具环境的单一案例研究,前提还是GitHub拉取请求出现之前的做法。而且核心指标缺陷密度是按行数除出来的,大改动的数值偏低,有一部分本来就是定义上的必然。它只统计了评审者实际报告的缺陷,这一点也让人在意。所以200到400行更准确的读法,是经验法则而不是物理定律。
即便只是经验法则,方向大体是对的。顺带一提,谷歌案例研究里报告的改动体积中位数只有几十行,比我们想象的小得多。落到实处,这样做就够了:不要把重构和功能改动混进同一个PR;把自动格式化和真正的逻辑改动分开提交;大规模改动先合接口,再把实现挂上去。
体积压下来之后,剩下一半是说明。好的PR说明包含三样东西:为什么做这件事(背景和链接)、考虑过哪些选项以及为什么选了这个(替代方案)、希望对方重点看哪里(评审请求点)。第三样对异步评审的往返次数影响最大。“第3个文件的锁处理请重点看一下,其余都是机械改动”这样一行,能省下评审者两小时和整整一天的往返。
被评审一方的技巧
评审文化不是靠评审者单方面努力就能建成的。被评审的一方也有技巧。
第一,把第一反应往后拖。读完意见立刻写反驳,是最常见的失误。尤其是花了很多时间写出来的代码,别人的指摘越容易被读成人身攻击。半小时后再读一遍,同样一句话往往看起来就不一样了。
第二,不同意的时候要请对方给依据。比起“那样做性能会变差吧”,“我当初选这个方案是因为A,您建议的方案在这一点上有什么好处”能快得多地抵达结论。争论拉长的话,换个形式也是办法。往返超过三轮,一通十五分钟的电话比二十条帖子便宜。
第三,决定不采纳的也要回一句。被沉默略过的意见,在评审者眼里就是被无视。“这个我放到下个PR处理,工单已经建好了”或者“这是有意为之的行为,我加了注释说明”,一行就够。建立信任的是回复率,不是采纳率。
第四,把学到的东西说出来。“这个模式我以前不知道,学到了”这一行不是奉承,而是给评审者的反馈。评审本来就是一份回报稀薄的劳动,而这一行会改变下一次评审的用心程度。
当评审变成权力游戏
以上都是运转良好时的情形。也有不良好的时候。
信号大体相似:只有某位评审者手上的PR特别久;指摘的内容每次评审都在变,根本无从对齐;架构问题就那么放过去了,变量名和空格却挂上二十条意见。最后这一项,和诺斯古德·帕金森(C. Northcote Parkinson)在1957年指出的自行车棚效应结构完全一致。核反应堆的设计谁都提不出意见,自行车棚的颜色人人都有话说。琐碎意见扎堆的地方,通常意味着没人读过难的那部分。
也有实证研究提示,评审并非完全中立的技术行为。乔希·特雷尔(Josh Terrell)研究团队在2017年发表的GitHub拉取请求分析报告说,整体上女性的PR通过率更高,但对于能从个人资料识别出性别的外部贡献者,这个优势就消失了。这项研究在性别推定方法和观测数据的因果解读上受到过相当多的反驳,结论并未定案。不过没有定案,也同样不能拿来当作“评审是纯技术的”的依据。
应对上,制度比个人本事管用。让格式化工具和静态检查工具去裁定风格之争,而不是让人去裁。把强度前缀写进团队规则,把阻塞权限显性化。把批准耗时和往返次数当成团队指标看,而不是按人看。而如果反复只有某个人被适用不同的标准,那就不是在评审帖里能解决的问题,而是该拿到和上司的一对一里去谈的问题。
结语 — 评审穿过的不是代码,是人
好的评审文化,衡量指标不是找到多少缺陷。而是半年后能改动那段代码的人多了几个。缺陷靠测试和监控也能抓到,但设计的来龙去脉要传遍团队,评审几乎是唯一的通道。
今天马上能改的只有两件事:给下一条意见加上强度前缀,以及在下一个PR说明里加一行“请重点看这里”。文化不是靠一份共识文件建成的,而是靠这样一行一行攒出来的。
현재 단락 (1/37)
有两条评审意见。一条是“这段你为什么这么写”,另一条是“这个函数把校验和存储放在一起做了,以后只想改校验规则的时候,怕是连存储逻辑也得动。拆成两个怎么样”。它们指向的东西完全一样。但收到第一条的人开始...