6 个实用的 Code Review 实践技巧

2020-08-26 00:00:00 代码 工作 团队 评审 反馈

点击上方蓝字关注我们

Code reviews 是打造高效团队的重要方面,这已经成为共识。关于这个主题,有许多文章曾经讨论过,比如这篇论文——《 An Empirical Study of the Impact of Modern Code Review Practices on Software Quality 》。现实中,许多企业的无数团队都进行过某种形式的 code reviews。
而实际情况是,code reviews 刚开始时,人们的激情高涨,之后,code reviews 则流于形式,或者要么反馈不清晰、要么让人难以执行。长久以往,这让团队错失了加快学习、分享知识的机会,终难以提高代码的质量。
在 Shopify,我们不仅立足长远,而且希望追求发展更快。以我们的经验来看,的 code reviews 实践对工程师的成长和我们所打造的产品质量有着巨大影响。
噩梦般的编码经历
这样一个场景相信很多人都很熟悉:
你刚刚加入一个新团队,领导很快给你分配了一个编码任务。作为新人,你特别想表现自己,因为你想秀一下自己的编码水平。于是,你接下来做了这些事:
  1. 你为了完成任务疯狂地敲了三周代码;

  2. 你将一个包含大约 1000 行新代码的 Pull Request 提交评审;

  3. 你收到两条关于 code style 的评论,以及一个关于评审人表示他看不懂这些代码用途的问题;

  4. 你修复 code style 并回答评审人的问题,然后评审人通过你写的代码;

  5. 你把代码分支合并到 Master,双眼紧闭,紧握着拳头,紧咬牙关等待着结果。几分钟后,CI 完成。幸好,Master 没有崩溃。然而…

  6. 此后 6 个月,你一直战战兢兢,不知道代码何时会崩溃,以及以什么方式崩溃。

你可能经历过上述噩梦般的经历,那我们谈谈怎样改进这个流程吧!
实用的 Code Review 实践
在 Shopify,我们看重交付速度、学习以及长期发展。这些价值观虽然有时会产生冲突,但却引导我们不断尝试许多新技术,并推动团队变革。
我在本文总结了一系列 Shopify 内部使用的实用技巧。借助这些技巧,我们能交付经得起时间考验的有价值的代码。
术语说明:我们将 Pull Requests(PR)定义为合并到基础分支前进行 code reviews 的一个工作单元。Github 和 Bitbucket 的用户对这个术语很熟悉。
 将 Pull Request 拆分为较小的代码段
这个方法很简单,可以成为提高 code reviews 工作流程有用的技术。它之所以有效,主要有两个原因:
  • 评审人心理上更容易接受开始和完成一小块代码的评审工作。更大的 PR 自然会让评审人推迟和拖延评审,并且在评审过程中被打断的可能性更大。

  • 作为一名评审人,如果 PR 太长,就很难深入进去。要检查的代码越多,我们越需要耗费更多脑力来理解整个代码块。

将 PR 拆分为更小的代码段,让你有更多机会在更短时间内得到更深入的评审。
目前,我们无法设置一个适用于所有编程语言和所有类型工作的通用标准。对于内部的数据工程项目,我们原则上是要将 PR 控制在 200-300 行代码。如果超过这个阈值,我们一般会将它拆分成更小的块。
当然,我们也要注意不要将 PR 拆分得过小,因为这意味着评审人可能需要检查好几个 PR 才能理解整体逻辑。
使用 Draft PRs
你听过造一辆汽车与画一辆汽车的比喻吗?这个比喻是这么说的:
  1. 用户要你造一辆车;

  2. 6 个月后,你造了一辆漂亮的保时捷;

  3. 你向用户展示这辆车后,他们问你这辆车能不能放得下他们的 5 个孩子和冲浪板。

显然,这里的问题在于目标不清晰,团队没有收集到足够的反馈就直接构建解决方案。如果在步后,我们先画一幅汽车的草图,并将其展示给用户,他们会问相同的问题,这样就可以进一步了解客户需求。如此,就为我们节省了 6 个月的工作量。软件也不例外,我们可能会犯同样的错误,在用户不需要的特性或模块上投入大量工作。
在 Shopify,一般用 Work In Progress (WIP) PRs 来获得早期反馈,其目标是验证方向(算法、设计、API 等等选择)。尽早变更可以避免在细节、修饰、文档等方面浪费精力。
作为一名写代码的人,这意味着你要对变更工作方向持开放态度。
在 Shopify,我们信奉的原则是允许大家有自己的理解,但不固执己见。我们希望大家能在有充足理由的情况下自信地做出决定,但同时也能乐于学习其他更好的新方案。在实际工作中,我们使用 Github 的 Draft PRs,它们明确表明这项工作仍在流程中流转,Github 不允许你合并一个 Draft PR。其他工具可能有类似的功能,至少你创建正常 PR 时可以加上一个 WIP 标签,以明确表示该工作还处于前期阶段。这将帮助你的评审人专注于适当的领域,提出适当的反馈。
 One PR Per Concern
除了行数外,需要考虑的另一个维度是你的工作单元试图解决的问题数量。一个关注点可以是一个特性、一个错误修复、一个依赖项升级、一个 API 变更等等。你是否在重构的同时引入一个新特性?一次修复了两个错误?同时引入了类库升级和新的服务?
把 PR 分解为一个个单独的关注点,它会产生下列影响:
  • 更独立的评审单元,这意味着更好的审查质量;

  • 受影响的人更少,因此可以聚集在更少的几个专业领域中;

  • 原子性回滚,可以回滚小的 commit 或 PR。这是很有价值的,因为如果出了问题,就更容易确定错误是在哪里引入的,以及回滚哪些部分。

  • 将易事和难事分开。假设有一个新特性,需要重构一个频繁使用的 API。你可以更改这个 API,升级十几个调用的站点,然后实现这个特性。你的变更中有 80% 不是功能上的变更,明显可以忽略掉,而 20% 是需要仔细注意测试覆盖率、预期行为、错误处理等等的新代码,并且可能要经过多次修订。对于每一个修订,评审人都需要浏览所有的修改以找到相关的部分。通过将其分成两个 PR,很容易就可以快速完成大部分工作,并优化评审工作,将主要精力投入到难点上。

如果你终拿到手的 PR 包含多个关注点,那么你可以将其分解为多个单独的块。这样能针对每一块进行单独的评审,每次评审的迭代周期可以更快,从而加速这个 PR 的总体评审周期。通常情况下,有一部分工作能先快速完成,避免代码烂到不能用以及引起合并冲突。
将 PR 分解成单独的关注点
上例的 PR 包含三个不同的关注点,我们将其进行拆分。可以看到,每个评审人需要检查的上下文少了许多。重要的是,只要其中任何一个部分的评审完成,代码作者就能一边等待其他评审反馈,一边着手处理已经反馈的问题。在极端的情况下,代码作者会陆续收到各个部分的评审反馈,几乎可以不间断地处理完这一系列 PR,而不是完成初稿后,等上几天(已经去忙其他的事),然后后再返回头来处理反馈意见。
 专注代码,而不是人
专注于代码,而不是人,这条实践谈的是人与人之间的沟通方式和关系。从根本上讲,这是提倡我们尝试把注意力集中在如何改进产品上,避免作者将评审意见当作对他个人的批评。
以下是一些你可以遵循的技巧:
  • 评审人可以这样想:“这是我们自己的代码,我们该如何改进它呢?”

  • 提出肯定意见!如果你看到有些代码部分写得不错,就加条评论表扬一下。这能让代码作者继续保持好的一面,并有助于他在心理上更容易接受改进建议。

  • 代码作者不妨这么想,评审人的出发点肯定是好的,不要将评论当作是对个人的批评。

  • 下表列出了一些存在不足的评审反馈,以及如何按以上建议进行重写的建议。

归根结底,code review 给我们提供了互教互学的机会,我们应该对此持开放欢迎的态度。
 挑选合适的评审人
决定由谁来评审你的工作通常很有挑战性。以下问题可作为参考:
  • 谁具备你正在构建的特性或组件的上下文?

  • 谁精通你正在使用的语言、框架或工具?

  • 谁对这一主题知之甚深,有自己的理解?

  • 谁关心你所写代码的结果?

  • 谁应该学习这些东西?或者,如果你是一名正在评审“老鸟的菜鸟程序员”,不妨抓住这个机会多多提问学习。别怕你的问题太幼稚,一个强大的团队会找时间来分享知识。

无论你的团队遵循哪些原则,请记住,作为一名代码的作者,你有责任寻求并接受适当的人对你的代码进行高质量的 code review。
给评审人提供关键的上下文
后但同样非常重要的一点是,你的 PR 描述至关重要。这取决于你选择的评审人,不同的人会有不同的上下文。代码的作者有责任提供关键信息或更多上下文的链接,帮助评审人能够反馈有价值的意见。
你可以把以下问题放到你的 PR 模板中:
  • 为什么这个 PR 是必要的?

  • 谁会从中受益?

  • 可能会出什么问题?

  • 你还考虑过其他方法吗?你为什么决定采用这种方法?

  • 这对其他系统有什么影响?

好的代码不仅没有错误,还非常有用。作为一名代码的作者,请确保你的 PR 描述将代码与团队目标联系起来,好能与待办事项中的特性或缺陷描述联系起来。作为评审人,会先评审 PR 描述,如果它不够完整,你是无法针对未定义的目标来判断代码是否适当的,不如在评审代码前就把它打回去。请记住,有时代码审查的佳结果是认识到根本不需要这些代码!
我们会有哪些收获?
通过采用上面的一些技术,你可以在很大程度上影响软件构建过程的速度和质量。但除此之外,还有潜在的文化影响:
  • 团队将达成共识。团队会更了解你的工作,除你之外,其他团队成员可以完善代码库的这一部分。

  • 团队将共同承担责任。如果出现问题,不只是某个人的代码需要修复,而是整个团队的代码都需要修复。

任何团队成员都应该能够休上几天假,他几天不工作不会让公司面临风险,也不会因为担心世界末日而不停地去看电子邮件。
个人可以如何改进团队的代码审查流程?
如果你是团队主管,不妨开始尝试这些技巧,找出适合你所带团队的方法。
如果你是独立贡献者,可以与主管讨论一下为什么你认为代码审查技术很重要,以及它如何提高效率和帮助团队。
在下次一对一交流或团队会议上,探讨一下这个问题。
参考阅读:
https://engineering.shopify.com/blogs/engineering/great-code-reviews
< END >


我就知道你在看

相关文章