[译] 如何用人类的方式进行 Code Review

标签: dev | 发表时间:2017-12-02 00:00 | 作者:
出处:http://itindex.net/relian

前言

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

读了之后深有感触,目前国内大多数公司对于 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 中找到可以被自动化的地方,下面是一些常见的点:

自动化可以让作为评审者的你做更多有意义的事情。当你可以不需要在意某大类问题(例如 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=[]forpathinpaths:url='https://'url+=domainurl+=pathurls.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 放到另一个文件里。”

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

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

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

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

比较下面这两种情况:


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


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

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

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

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

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


第二部分

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

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

相关 [人类 code review] 推荐:

聊聊Code Review

- - 梦想风暴
hopesfish评论《 那一点的调用》时,问了一个关于Code Review的问题:. 想请教一下,TW的筒子是如何做code reivew或者鼓励客户做code review的. 我在翻阅博主的帖子的时候,似乎对这块没有特别强调,而是更多偏重于TDD,我觉得TDD的问题是一碰到没有责任心的程序猿,就很容易流于形式了.

Java Code Review清单

- - ImportNew
使用可以表达实际意图(Intention-Revealing)的名称. DRY(Don’t Repeat Yourself)原则,(拒绝重复). 用代码来解释自己的做法(译者注:即代码注释). *参考自: http://techbus.safaribooksonline.com/book/software-engineering-and-development/agile-development/9780136083238.

[译] 如何用人类的方式进行 Code Review

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

我的code review规则

- vento - 我的宝贝孙秀楠 ﹣C++, Lua, 大连,程序员
1) 是否有语法错误,编译错误,编译警告. 做法:下载最新代码,将编译警告级别提升到最高,检查output信息. 2)是否符合需求,完成requirement文档要求的内容,不能多,也不能少. 注意:即使发现有问题代码,如果与需求关联不大,不要涉及. 应该让每次enhancement和bug fix最简洁,牵涉范围最小,影响到组件最少.

Code Review那些事儿

- - 非技术 - ITeye博客
       曾经有一段 垃圾代码放在我的面前,我没有拒绝,等我真正开始接手的时候我才后悔莫及,程序员最痛苦的事莫过于此. ---------改编于周星星的经典台词.       虽然有点夸张,但编码界确实大大存在这种情况,每当接手别人的代码,都有一种想重新写一遍的感觉,等到别人再来接手你的代码时,同样的感觉.

代码审查(Code Review)清单

- - 博客 - 伯乐在线
代码审查可以帮助提高代码质量,避免由于代码习惯而造成的 bug. 下面列出的这些要点因该可以作为大部分代码审查的指导,如果是 Java 应用的话,这些建议应该被视作最佳实践. Javadoc 应该在每一个类和方法中添加. 如果是修复某个 bug,应该添加 bug ID. 走捷径的方法或者复杂的逻辑要有解释.

从Code Review 谈如何做技术

- - 酷 壳 - CoolShell.cn
(这篇文章缘由我的微博,我想多说一些,有些杂乱,想到哪写到哪). 这两天,在微博上表达了一下Code Review的重要性. 因为翻看了阿里内部的Review Board上的记录,从上面发现Code Review做得好的是一些比较偏技术的团队,而偏业务的技术团队基本上没有看到Code Review的记录.

我们该如何做好Code Review?

- - SegmentFault 最新的文章
我们该如何做好Code Review?. 午后的阳光,静静地照在你的脸上. 这时候配上一杯82年的java,脑子一片灵光闪过,呃......上午刚写完需求,下午好像没什么事了,不如看看自己写的代码. 至此,一场Code Review也就拉开序幕了. Code Review,即代码审查,是指对计算机源代码系统化地审查,常用软件同行评审的方式进行,其目的是在找出及修正在软件开发初期未发现的错误,提升软件质量及开发者的技术.

测试技术中CODE REVIEW的重要性

- - CSDN博客推荐文章
        [近期关注App自动化测试,欢迎交流,本博客文章版权归作者所有,转载请联系]     .         最近有网上的朋友向我咨询作为测试员是否应该跳槽,   首先我觉得应该向大家介绍一下什么是测试工程师,  什么是测试员,   在国内的一些中型企业并没有特别的指明.   这里测试工程师主要指测试开发工程师, 主要包括两类,  其一是测试软件开发的工程师,  其二是自动化测试脚本开发和维护的工程师,   而测试员主要指单纯编写/管理测试用例,  或是手工测试人员, 一些国内的大中型网络视频公司仍然在用纯手工测试,我感觉到很汗颜.