阅读 2488

【译】如何用人类的方式进行Code Review(How to Do Code Reviews Like a Human)

译者前言

原文是 Google 工程师 Michael Lynch 的个人博客文章:

  • https://mtlynch.io/human-code-reviews-1/
  • https://mtlynch.io/human-code-reviews-2/

读了之后深有感触,目前国内大多数公司对于 Code review 的重视程度还远远不够,大多数人都把它视为一件麻烦事。即使在有 Code review 流程的团队,也缺乏相关经验,而且目前中文技术圈关于 Code review 的文章真的太少了,所以在这里翻译这篇个人认为很不错的文章给大家看。


正文

最近,我读了很多关于 code review 最佳实践的文章。但我发现大多数文章都着重于教你如何找到代码中的 bug,而几乎完全忽略了 code review 的其他部分。比如你沟通的方式是否足够建设性及专业呢?无所谓!只要找到所有 bug,剩下的就自生自灭去吧。

所以我获得了一个启示:如果这些跟代码相关,为什么不能以一种更浪漫的形式呢?于是,我想把我的新书介绍给开发者们,以帮助他们以及他们热爱的生活。

我这本革命性的书将会教你一些实用的技巧,以帮助你最大限度地找到你同伴身上的所有缺点。但它不会包括以下内容:

  • 如何与你的同伴沟通,让你们相互理解、产生共鸣
  • 如何帮助你的同伴找到他们的不足之处

这本书适合你吗?我仿佛听到了你喊“不不不不!”

所以,为什么我们一定要用那种(没人性的)方式来讨论 code review 呢?

唯一的回答就是,我是在遥远的未来读这本书的,那个时代所有的开发者都是机器人。在那个世界里,你的团队小伙伴很喜欢冰冷的、生硬的、无情的评论,因为它们都是机器人,阅读这些评论能温暖它们的机械之心。

但我想做一个大胆的假设,你现在就想改善你们团队的 code review,而你们的团队成员都是活生生的人类。我还想做一个更大胆的假设,那就是“与同事建立积极的关系”本身就是一个目的,而不是简单地调整某个变量,以减少 bug。在这种情况下,你的 code review 会发生怎样的变化呢?

我的这篇文章会介绍一些技巧,让 code review 不再仅仅是一种技术上的流程,更是一种社交性的流程。


什么是 Code Review?

“Code Review” 这个术语实际上包含了一个很大范围内的活动,从最简单的读同伴的代码,到 20 个人正襟危坐在会议室里一行一行地剖析代码,都可以称为 “Code Review”。在后文里,我用这个术语来指代一个正式且书面化的过程,但不是像内部 Code Review 会议那样重量级。

参与 review 的人有两种,一种是作者(author),也就是代码的编写者和 review 的发起者;另一种是评审者(reviewer),也就是阅读代码,并且决定代码是否可以合并进入团队代码库的人。评审者可以有多个人,但后文里我会假设你是唯一的评审者。

在 review 开始之前,作者必须创建一个变更列表(changelist)。它含有一系列的修改,而作者想要把这些修改合并进入代码库。

当作者把变更列表交给评审者之后,review 就开始了。review 是一种轮转的方式进行的,每一轮都是一次作者和评审者之间的往返:作者提交变更列表,评审者对这些变更作出反馈。每个 review 都会有一轮或多轮。

当评审者**同意(approve)**这些更改之后,code review 就结束了。这个时候一般会评论一句 “LGTM”,也就是 “looks good to me” 的缩写。


这有哪些难点?

设想一下,如果一名程序员给你递交了一份他们自认为很赞的变更列表,而你回复了一大堆问题,告诉他这份变更列表并不好,这里就很容易让人感到冒犯。

这就是为啥我从来不怀念 IT 行业的原因,程序员都是些很不讨人喜欢的人… 要是放在航天行业,这些高估自己能力的人坟头草都已经一米多高了. --- Philip Greenspun,ArsDigita 联合创始人

对于代码的作者来说,评论或者批评他们的代码,很容易被视为一种暗示,即他们不是一名称职的程序员。虽然 code review 是一个很好的机会来分享知识、做一些工程上的抉择,但如果 code review 被视为人身攻击,那么这些好处都不会发生。

就算上面这种情况不会发生,你也会遇到沟通的问题,把你脑子里的想法用文字准确地表述下来是很具有挑战性的,因为别人很容易产生误解。代码的作者听不到你的语音语调,也看不到你的肢体语言,所以清楚的写下你的反馈是一件很重要的事情。对于戒备心理很强的代码作者而言,一句无意的 “你忘了关掉 file handle”,可以被理解为,“你竟然忘了关闭 file handle?你这个蠢猪。”


技巧

让电脑做重复的事情

在会议和邮件的交替轰炸中,你能专注于代码的时间实际上是很稀有的,你的精神耐受力甚至更短。读团队小伙伴的代码需要大量的精力和高度的精神集中,所以不要把精力花在计算机可以代替我们做的事情上,特别是那些计算机做得更好的事情。

空格问题就是一个明显的例子。比较一下,评审者靠肉眼找到一处缩进错误,然后协助代码作者修正它,和直接使用一个代码格式化工具,哪一个耗费的时间更少?

靠评审者的人力 靠代码格式化工具
评审者靠肉眼找到缩进错误
评审者写了一条评论来标注这个错误
作者读这条评论 什!么!事!都!不!需!要!干!
作者修正缩进错误
评审者再次检查,确认错误已修复

表格的右边之所以啥都不需要干,是因为作者已经使用了一个自动格式化工具,在每次保存代码时,都会自动执行。最坏的情况下,作者没检查代码就提交 review 了,持续集成系统也会报错,这样作者就可以在评审者发现之前就修复这个问题。

在你的 code review 中找到可以被自动化的地方,下面是一些常见的点:

任务 自动化解决方案
确认代码可以构建无误 持续集成系统,例如 Travis 或者 CircleCI
确认代码可以通过自动化测试 持续集成系统,例如 Travis 或者 CircleCI
确认代码的空格和缩进符合团队规范 代码格式化工具,例如 ClangFormat (C/C++ formatter) 或者 gofmt (Go formatter)
找出无用的 imports 或者无用的变量 代码格式检查工具,例如 pyflakes (Python linter) 或者 JSLint (JavaScript linter)

自动化可以让作为评审者的你做更多有意义的事情。当你可以不需要在意某大类问题(例如 imports 的顺序、源文件的命名约定),你就可以有更多的精力关注其他更有趣的问题,例如函数错误或者可读性差的问题。

自动化同样可以让代码的作者受益,它可以让作者快速地在几秒钟(而不是几小时)内找到一些粗心产生的低级错误。快速的错误反馈产生的修复成本也很低,因为代码作者的脑中依然有这段代码的上下文。另外,来自电脑的报错相比于来自评审者的纠错,从自尊心上讲,更容易让人接受。

你的团队应该立刻把这样一套自动化工具加入到 code review 的工作流中(比如 Git 的 pre-commit hook,还有 Github 的 webhook)。如果 review 需要评审者手工去做这些事情,那就丧失了很多益处。代码的作者总是会忘记遵守某些东西,以至于你必须重复地检查很多简单又低级的问题,而这些问题本应是自动化工具代替你做的。


用代码风格规范来解决代码风格的争议

关于代码风格的争吵在 code review 中是非常浪费时间的。一致的代码风格当然是非常重要的,但 code review 的时间并不该浪费在讨论圆括号该放在哪里。最好的做法是通过维护一份代码风格规范来避免这里的争吵。

一份优秀的代码风格规范,不仅仅定义了诸如命名规范、空格规范这些表面上的东西,同样也应该定义如何使用语言的某些特性。例如 JavaScript 和 Perl,它们具有函数式编程的特性 —— 也就是说它们提供了多种方式来完成同一件事情。代码规范里应该只定义一种正确的方式,这样的话才能让你的团队不会一半的人用某些语言特性,而另一半的人用完全不同的其它特性。

当你有了一份风格规范后,你就再也不需要把时间浪费在讨论谁的命名规范最好这种问题上了,只要按照规范来就可以。如果你的风格指南没有针对某个特殊问题作出规定,那么它在 review 过程中不应该被讨论。如果遇到规范中没有涵盖的问题,并且这个问题足够重要,那么可以与团队成员进行讨论,然后将讨论结果记录在代码风格规范中,这样你们以后就不用再讨论了。

选项一:使用一份已有的代码规范

在网上搜一搜,就能找到不少已有的代码规范,其中 Google Style Guide 是最广为人知的。当然,如果它不适合你的话,你也可以用别的规范。使用已有的规范,你可以直接获得收益,而不需要从零开始创建一份规范。

直接复用一份现成的规范,缺点在于,这些规范可能是为了原团队中某些特殊需求而优化的。比如 Google 的代码规范,对于新的语言特性十分保守,因为他们有一个巨大无比的代码库,这些代码可能会运行在很多地方,从家庭路由器,到最新款的 iPhone 上。如果你所在的是只有四个人和一款产品的小型初创公司,那么你可能会更喜欢使用最尖端的语言特性或者扩展。

选项二:渐进式地建立你自己的代码规范

如果不想直接使用现成的代码规范,当然可以自己创建一份。每当 code review 时产生了关于代码风格的争议,就把这个问题提给团队所有成员,来决定正式的标准。达成一致后,把这个标准写入你的代码规范中。

我一般喜欢把我团队的代码风格规范以 Markdown 的格式托管在版本控制软件之下(比如 Github pages)。这样,对规范的任何修改都会经过一个正式的 review 流程 —— 必须有某人明确地批准修改,团队中的任何人都有机会提出疑虑。用 Wiki 和 Google Docs 当然也是可以的。

选项三:混合式的方法

结合选项一和选项二,你可以用一份现成的代码规范,在它的基础上,建立你自己的代码格式规范。比如 Chromium C++ style guide 就是个很好的例子,它建立在 Google C++ style 的基础之上,但有它自己修改或添加的一些规则。


立刻开始 Review

code review 应该被视作高优先级的事情。你阅读代码并提供反馈意见时,花点时间无所谓,但 review 必须要立刻开始 —— 最好是在几分钟内。

Code Review 的接力赛

一旦团队成员提交给了你一份变更列表,这可能意味着在你的 review 完成之前,他们的工作会被阻塞住。虽然在理论上,版本控制系统可以让代码的作者切换到新的分支,然后把审核中的提交合并到新的分支,继续工作。但实际上,只有很少的人能高效率地做这些事情,因为这需要同时同步三个分支(译者注:master、review 中的分支、新分支)的变动。

当你立即开始 code review,你就创造了一个良性循环。你一轮 review 所需要花的时间,和变更列表的大小及复杂度成正相关。这就鼓励了代码的作者提交小范围的变更列表,这也让你的 review 变得更轻松愉悦,你的 review 也会更快,形成一个正向循环。

想象一下,你的小伙伴实现了一个新功能,需要改变 1000 行代码。 如果他们知道你可以在大约 2 小时内查看 200 行的更改列表,则可以将其功能分解为多个变更,每个约 200 行,于是你就可以在一两天内 review 完毕。 但是,如果你每个 review 都是拖了一天之后才开始 ,那么就需要花费几乎一周的时间才能做完 review。你的小伙伴当然不想就呆坐在那里一周,因此他们就会偏向于提交更大体积的 review,比如500-600行。 这些大的 review 要花的时间更多,而且会产生质量更差的反馈意见(因为你要在脑内记住 600 行的变化而不是 200 行)。

一轮 review 的最大周期应该限于一个工作日,如果你因为某些优先级更高的事情忙成狗了,那么请你告诉你的小伙伴,让他们把 review 的任务交给别人。如果每个月都有几次这样的情况发生,那就说明你的团队需要减速了,这样才能保证团队的开发不会失去控制(译者注:在中国就别想了)


自顶向下的方法

你在一轮 review 中提出的问题越多,代码作者感到的压力就会越大。具体数量的限制因人而异,但一般一轮 review 提出的问题应该限制在 20 - 50 个之内。

如果你担心评论太多,把代码原作者淹没在茫茫的问题之中,那么建议你在早期的 review 中先关注一些高层次的问题,例如重新设计类的接口,以及分解复杂函数等。直到这些问题得到解决,再去关注低层次的问题,比如变量命名,或者代码注释的清晰程度。

代码原作者关注高层次问题时,一些低层次的问题可能会被忽视。把这些低层次问题暂时延后到下一轮 review,就可以避免重复检查这些低级问题,也可以节省代码原作者的时间。这个小技巧可以让你在 review 过程中对应该关注的层面进行细分,帮助你和原作者以一种更加清晰、系统的方式处理更改列表。


多写代码示例

在理想的世界里,代码作者应该会很感谢他们收到的每一个 review,因为这是一个很好的学习机会,并且让他们纠正了错误。但在现实中,有诸多因素可能会导致作者负面地看待这些 review,并且反感你对他们代码的评论。也许他们正面临着压力,要在最后期限之前完成任务,所以除了即刻批准以外的任何事情都会被视为一种阻碍。 也许你们之间没有太多的合作经验,所以他们不相信你的反馈是好意的。

这有一个好方法,可以在 review 过程中舒缓作者的心情,那就是在 review 期间找机会送给他们礼物。所有开发者都喜欢接受的礼物是什么?那当然是,代码示例。

你可以通过写一些示例代码来减轻作者的负担,以展现出你作为评审者的慷慨大度。

比如说,你有一个同事并不是很熟悉 Python 的列表推导特性,他给你发了如下的代码:

urls = []
for path in paths:
  url = 'https://'
  url += domain
  url += path
  urls.append(url)
复制代码

作为回复,一句 “我们能不能用列表推导来简化这儿的代码?” 可能会让他们感到恼怒,因为他们或许需要花 20 分钟来搜索他们之前从没用过的东西。

但如果收到的评论是像下面这样,他们应该会很高兴:

可以考虑这样简化代码:

urls = ['https://' + domain + path for path in paths]
复制代码

这个小技巧不仅限于单行代码。我会经常创建自己的分支来向原作者展示大量的概念,比如拆解一个大的函数,或者添加单元测试来覆盖额外的边界情况。

这个小技巧会让你得到明确的、无争议的改进。在上面的示例中,很少有人会反对将代码行数减少83%。相比之下,如果你写了个冗长的例子来展示你个人品味上觉得 “更好” 的示例(例如,代码风格),这会使你看起来更一意孤行,而不是开明大方。

当然示例代码也不能写得太多了,如果你为原作者写了几乎覆盖整个变更列表的示例代码,那就表示你不认为他们有能力编写自己的代码。


不要说“你”

这听起来有些奇怪,但请相信我说的:在 code review 中,不要使用 “你” 这个词。

你在 review 中所做的评论应该是基于 “什么使得代码更好”,而不是 “谁提出了这个想法”。你的小伙伴在他们的变更列表上花费了大量的精力,他们当然会为他们所做的工作感到自豪,所以当收到批评时,自然会产生戒备心理。

你应该用一种最不会产生戒备心理的方式来评论代码。要记住你批评的是代码,而不是代码的作者。当代码的作者在评论里看到 “你” 这个词的时候,会把注意力从代码上转移到他们自己身上。这会加剧他们抗拒你的评论的可能性。

比如说下面这个无恶意的评论:

你把 'successfully' 拼错了

作者可能会把它读成两种意思:

  • 含义一:嘿,好兄弟!你把 'successfully' 拼错了,但我认为你这么聪明,这应该只是一处小粗心吧!
  • 含义二:你把 'successfully' 拼错了!白痴!

相比之下,如果你的评论里没有提及 “你” 这个词:

sucessfully -> successfully

这条评论就只是简单地纠正了拼写错误,没有包含任何对于作者的评价。

幸运的是,在评论中去掉 “你” 这个词非常容易。

选项一:用 “我们” 代替 “你”

你能不能把这个变量名写得更清晰一点?比如 seconds_remaining

修改之后:

我们能不能把这个变量名写得更清晰一点?比如 seconds_remaining

“我们” 这个词强调了团队对于代码的责任。代码的作者可能未来会去别的公司,你也可能会,但这里的代码会被它所属的团队一直维护着。当然,用 “我们” 这个词听起来有些愚蠢,因为这显然是你作为一名评审者,要求作者做的事情,但愚蠢总比指责更好。

选项二:移除句子的主语

避免使用 “你” 的另一个方法是移除句子的主语:

建议使用更清晰的变量名,比如 seconds_remaining

当然被动语态也是可以的。虽然我在写技术文章时尽量避免使用被动语态,但它在 “你” 这个词的问题上确实有用处:

这个变量应该使用更清晰的名字,比如 seconds_remaining

还有一种方法是把它化为一个问题:

要把这个变量名换成更清晰的吗?比如 seconds_remaining


使用请求的语气,而不是命令

Code review 比日常的交流需要更多的精力,因为很容易把讨论变成个人观点的碰撞。你总是期望评审者能在评论中保持礼貌,但奇怪的是,他们总是和期望相反。日常工作中,大多数人都不会对同事说:“把订书机拿给我,然后给我倒一杯苏打水过来。” 但 review 过程中却经常看到类似这样的评论:“把这个 class 放到另一个文件里。”

这样的语句经常会让你的评论令人恼怒。你的评论应该使用请求或者建议的语气,而不是命令。

比较下面这两种语气的区别:

命令 请求
把 Foo class 移到一个单独的文件里 我们能不能把 Foo class 移到一个单独的文件里?

大多数人都喜欢完全掌控他们的工作,使用请求的语气可以让他们有自主的感觉。

另外,请求的语气也让作者更容易礼貌地推辞你的评论,或许他们是出于某些原因,考虑过后才写下的代码。如果你的语气是命令式的,那么作者的任何推辞和解释都会变成直接的不服从行为。如果你用的是请求或者提问的语气,那么作者可以简单地回复你。

比较下面这两种情况:

命令(对抗的语气) 请求(相互合作的语气)
评审者:把 Foo class 放到单独的文件里 评审者:我们能不能把 Foo class 放到单独的文件里?
作者:我拒绝,因为那样的话它就会远离 Bar class。客户端里几乎都是一起用这两个 class 的。 作者:可以啊,但如果这样的话,它就会远离 Bar class,客户端几乎总是一起使用这两个 class 的,你觉得该怎么做?

看,当你的语气变成请求式之后,是不是交流少了很多火气呢?


评论应该基于原则,而不是观点

当你写下一条评论,要记得同时写下要做什么以及为什么要做。说 “我们应该把这个 class 切分成两个” 是不太好的,更好的说法是 “现在这个 class 负责下载并且解析文件。根据单一职责原则,我们应该把它切分成两个 class,一个负责下载,一个负责解析。”

你的评论应该是基于原则的,这样才可以让 review 是建设性的。当你有一个明确的理由为什么要这样做时,比如 “我们应该把这个方法私有化,以减少公共方法的数量”,作者一般都不会简单地回复 “不,我更喜欢现在这样”。即使他们这样简单地回复了,这样的回复也会看起来很傻,因为你都已经说明理由了,而他们只是因为个人偏好而拒绝你的理由。

软件开发既是一门艺术,也是一门科学。有些时候,即使有了既定的原则,你也不能清楚地证明一段代码是错误,因为有时代码只是丑陋或不直观而已。在这些情况下,请详细解释一下为什么,并保持客观。比如如果你说 “觉得这很难理解”,这就是一个客观的陈述。但如果你说 “这里写得乱七八糟”,这就是一种价值判断,是仁者见仁智者见智的。

另外,在提供支持性材料的时候,尽可能以链接的形式附在后面。团队的代码风格规范是最好的,当然你也可以发一个语言或库文档的链接,高赞数的 StackOverflow 答案也可以。但是要知道的是,离权威性文档越远,材料就越无力。


第二部分

如果你喜欢这篇文章,还可以去看它的第二部分,着重于介绍如何让 Code Review 顺利完成,而不是各种碰壁。第二篇文章会介绍以下技巧:

  • 如何处理超大的 Code review
  • 恰当地称赞对方
  • 限制 Review 的范围
  • 如何缓解僵局
评论