代码审查的艺术:Dropbox 的故事

Dropbox 的 iOS 应用中的每一行代码,都是开始于被添加到 Maniphest 中的一个 bug 或者功能任务,Maniphest 是我们的任务管理系统。当一位工程师在上面接受一个任务,那么在开始写代码前相应的责任就已经赋予他。Phabricator 这个平台包含了我们的代码审查工具,这个代码审查工具有很多很好的功能,但它在评估对象之间的相互协作上不是做的很好。为了弥补这点,我们的工程师在开始他们的工作之前需要知道审查他们的任务的人是谁[1]。对于被审查代码的工程师来说,这样能确保在他们的团队中有一个橡皮鸭,这个橡皮鸭知道项目中一些改动代码的背景和原因,并且对代码的设计决策上起到协助的作用。对于审查者来说,这有助于他们将一些变化考虑进他们的开发周期评估中,这样有助于开发周期评估的准确。如果不出意外的话,我们的经验会告诉我们提前做好计划可以有效地避免审查代码过程中的重复劳动。针对项目中的变化做计划可以像在白板前做交流一样简单,也可以像写一篇建设性文档一样深入。这都取决于我们自己的选择。

[1] 我的团队中每个人都要审查代码。新来的同事在可以独立审查较大的任务之前,会先被分配一些比较少的代码量。

随着任务的展开,工程师需要一直谨记我们的代码规范。这个规范是一个最佳实践和一致规范的大融合,它的存在使我们不用去猜测我们应该怎样编码,也使审查变得更容易[2]。因为这是一个大项目,开发团队中没有一个人能对整个项目有完美的映射或理解。所以我们的工程师需要依赖团队中其他工程师的帮助,将这些代码的功能表现拼成一个整体,这有助与我们在阅读代码时能理解其中的逻辑。

[2] 即使这样,每当一个新成员加入时,总还是不免要展开一次关于使用 property 还是 ivar 的辩论。

当这个任务的工作进行到某个阶段时,我们的工程师很可能会做出一些明显不合理的或者不受欢迎的决定。捕获这个心理的最佳时间就是发生这一刻 -- 为将来向审查者做好解释的准备。去解释这些变化,说起来容易做起来难,我们的工程师被鼓励使用 //TODO//HAX,和 //FIXME 来在代码中写注释。//TODO//FIXME 从字面上就可以理解它的意义,尽管后者会产生编译警告,所以必须在下一次发布之前要被解决。//HAX 这个注释比较有趣的地方。我们用它标注那些用来绕过 Apple 的 API 里的 bug 但又不容易一眼看明白的方法[3]。我们的注释会写上日期和写这个注释人的名字[4],在之后很多时候我们总会感激这些额外的上下文的[5]。

[3] 标注里通常是第三方来源或者 radar 的链接,还有特殊的重现步骤。

[4] 比如像 //HAX:(ashleynh) 2015-03-09

[5] Hello 👋 iOS 7

随着开发的继续,我们的工程师可能会陷入那些看似对现有功能的快速优化中去,但这是一个陷阱,这个额外的优化不可避免地可能会存在很多潜在后果,就像是一个兔子洞。这是一个很典型的 DoingTooMuch™ 。我们唯一的解决方式就是,针对这个优化创建一个新的任务,然后回过头来专注到当前这个被分配的任务上。

如果我们的工程师已经做到这一步,那真是太棒了!这个任务的要求就已经达到了。然而,写代码只是任务的一部分而已。接下来要开始针对变化进行工作。

我们的工程师会用命令行工具 Arcanist 来将变化的代码上传到 Differential 这个代码审查工具中。在这个过程中,我们会运行脚本和单元测试。其中脚本用来将我们的代码格式化,这样有助于让我们专注于阅读功能性的改变,而且不用对代码格式再吹毛求疵。我们尤其还会用 clang-format 这个插件来对间距和行距进行格式化,然后用 homegrown script 这个脚本将我们的 import 按字母排序。这些脚本将这些琐碎的事情像魔法般简单地实现,但是我们的工程师在提交这些代码变化之前,也还可以再非常认真地检查几遍。

代码被自动格式化之后,现有那些有改动的单元测试将会被执行。发现任何有失败的用例时,我们的工程师都会在上传代码之前先去解决它们。

虽然这些改动的代码被上传了,但是在这些代码被审查之前,我们的工程师还是有些表格需要去填写。首先,写这些代码的目的是什么?这些目的是怎样被达到的?接下来,我们的工程师需要附上一个测试计划。我们的工程师确实创建了一个测试计划了么?这个计划能考虑到所有可能引起代码出错的边界情况了么?这个测试计划设计的足够模块化么,是否有可能进行单元测试?如果这些问题的答案有任何一个是 “NO” 的话,那么我们的工程师就不得不关掉 Differential,然后重新打开 Xcode 去修改了。

现在,有了这份经过我们工程师深思熟虑之后写好的测试计划,那些改动的代码可以准备被提交去测试。

这个时候,我们的目光就要移到负责审查的工程师的身上,他们将努力用友善的方式给出一份建设性的反馈。使用类似 meme 的文本配合图片会非常有帮助。请记住负责这个代码的工程师投入了很多精力在这些代码上,他们在很努力地解决这些问题,所以在回馈中,要注意语气,要和善。

其实在一个任务开始的时候,负责审查的工程师就已经参与进来了,所以他们希望,当自己问负责这个任务的工程师一些大的框架性问题的时,比如说 “这些代码已经尽可能的模块化了么?” 或者 “代码中有避免那些没必要的重复么?”,他们希望他们听到的回答是 “当然!”,如果是这样的话,负责审查的工程师就会开始往更深的层面去挖掘,来完成一个全面的评估,这个评估中可能包括一些补充和修改。一般情况下,除非代码是很明显的,否则如果你提交的代码没有注释或没有对应修改请求的话,这些代码是不会被接受的。所以这个时候我们的目光又回到了提交代码的工程师身上。

当被审查的工程师在收到的反馈中读到对于修改进行的注释的时候,我们的工程师需要谨记的是,这些反馈并不是针对他个人的。代码在任何规模的项目中都不可能绝对正确,在较大的项目中更是如此。审查代码可以促进工程师之间的交流,这样提供了一个很好的成长机会。一个完整的代码审查流程需要有效的工程系统的努力,而这正是有效沟通的文化的象征。

经过几次代码审查的迭代后,根据代码改动的量,工程师们的代码就可以准备合并[6]。Dropbox 的 iOS 应用中的每一行代码都是开始于 Maniphest 的一个任务里,而完结于我们的工程师的自豪感中。现在,让我们再选择另外一个任务吧。

[6] 这对于审查代码和被审查代码的工程师来说,这些都是非常好的机会,在他们提交或者审查以后的代码时,他们可以将这些问题添加到一个名为“需要考虑的常见问题”的列表中


原文 The Art of Code Review: A Dropbox Story