惰性使然，很多团队推行不了 Code Review

原文地址：

https://www.cnblogs.com/wenhx/p/5641766.html

我们团队开展 Code Review 也有 2 年了，结果还算比较满意，有些经验应该可以和大家一起分享、探讨。

我们为什么要推行 Code Review 呢？我们当时面临着代码混乱、Bug 频出的状况。

当时我觉得要有所改变，希望能提高产品的代码质量，改善开发团队面临的困境。并且我个人在开发上有很多经验，也希望这些知识能够在团队内传播。

各种考虑后，我们最后认为推行 Code Review 能改善或解决我们面临的很多问题。

这篇文章的目的不是告诉大家怎么在一个团队内推行 Code Review，首先因为我个人仅在一家公司内推行过，并没有很多经验。

其次每家公司、每个团队的情况都不太一样，应该根据公司或团队的实际情况选择恰当的方案，并根据成员的反馈来及时调整，推动 Code Review 的实施。

所以，本文是介绍我们公司是如何实施 Code Review 的，我们是如何解决我们遇到的问题的，希望我们的经验能给大家带来些帮助。

行文仓促，如有遗漏或错误，欢迎指正。

一、流程和规则

经过简单的对比、试用，我们最后采用了 Git Flow+Pull Request（PR）模式来做 Code Review。（PR 模式详情可参见 Git 工作流指南：Pull Request 工作流）

Pull Request(PR) 简单的说就是你没有权限往一个特定的仓库或分支提交代码，你请求有权限的人把你提交的代码从你的仓库或分支合并到指定的仓库或分支。

由于 PR 需要有权限的人确认，所以非常适合在这个过程中做 Code Review，是否接受或者拒绝就取决于 Code Review 的结果。

在支持 PR 模式的软件里，每一个 PR 都有一个新增代码的对比（diff）界面。

代码审核者可以在线浏览请求合并的新增代码，并针对有疑问的代码行添加评论，通过这种方式来实现 Code Review。

评论可以被所有有权限查看仓库的人看到，每个人都可以回复任何人的评论，有点像论坛里某个帖子的讨论。

这种模式是事后审核，也就是代码已经提交到了中心仓库，Review 过程中频繁的改动会造成历史签入记录的混乱。

当然 Git 可以采用更改历史记录来解决这个问题，由于容易误操作，我们一般只在基础类库这类要求比较严格的项目上实施。

我们所了解到的支持 PR 模式的软件都采用 Git 作为源代码版本控制工具，所以我们的源代码版本控制工具也迁移到了Git。

由于 Git 太灵活了，因此诞生了很多的 Git 流程，用来规范 Git 的使用。常见的有集中式工作流、功能分支工作流、Gitflow 工作流、Forking 工作流、Github 工作流。

我们对 Git Flow 做了些调整，调整后的流程被命名为 Baza Flow，我们大部分仓库只定义了 2 个主干分支，master 和 develop。(例外，我们有一个仓库有 3 个开发小组同时进行开发，定义了 4 个主干分支，目前还比较顺畅，再多估计主干分支之间的合并就比较繁琐了。)

master 对应生产环境代码，所有面向生产环境的发布来源都是master分支的代码。develop 则对应本地测试环境的代码。

绝大多数情况下，QA（测试）只测试 develop 分支和 master 分支的代码。

由于开发人员都在一个团队内，所以我们没有采用基于仓库的 PR，采用的是基于分支的 PR。

我们对主干分支的操作权限做了限制，只有特定的人才能操作，develop 分支是项目开发 Leader 和架构师，master 分支是 QA。

有权限往主干分支合并的成员会按照约定的规则来执行合并，不会合并没有完成审核的PR。

上面这点其实蛮重要的，所以我们会对有权限合并的人有特别的约定，在什么情况下才能合并代码。（见后文 PR 的说明）

PR 的发起人要主动的推动 PR 的审核，Leader 也会密切关注 PR 审核的进度，在需要的时候及时介入。

我们配置了 CI 服务器（什么是 CI）只编译特定的分支，通常是 develop 和 master 分支。

所有的代码合并到了主干分支之后，都会自动触发编译和本地测试环境的发布，QA 无需依赖开发人员编译的代码来测试，也无需自己手工操作这些，保证了开发人员和测试人员的相互独立。

我们本地测试环境的发布包含了数据库和站点的发布，全自动的，发布完成以后就是一个可用的产品。我们还使用了 Scrum 里面一个很重要的概念：完成定义——代码编写完成，经过自测，提交的 PR 经过审核并且合并到主干分支。也就是说，所有的代码被合并到了主干分支之后任务才算是完成，而被合并到主干分支必须要经过 Code Review，这是强制的。

Baza Flow 

Baza Flow（当前版本 V0.9） 由 Git Flow 演化而来，Git Flow的开发模式如下图所示：

由于我们的托管软件对于 Pull Request 的限制，我们对 Git Flow 做了改动，改动的地方有：

1. 每一个大功能我们会创建一个单独的 feature 分支，项目开发人员基于这个单独的 feature 分支创建自己的任务分支。

比如，对于 CS 2 项目来说，启动的时候分支的创建是：master -> develop -> feature/v2。

开发人员应该基于这个大特性分支 feature/v2 来创建自己的任务分支，比如创建 XXXX，可以用一个单独的分支 feature/v2-xxxx。

完成这个任务以后，立即向上游分支（feature/v2）提交 pull request。然后从 feature/v2-xxxx 创建自己的下一个任务分支，比如 YYYY 编辑 feature/v2-yyyy。

请注意，合并到上游分支的功能必须相对独立而且是可用的，分支任务工作量 0.5-1 个工作日，不宜超过 2 个工作日，超过 2 个工作日不向上游合并，需要向团队解释。

代码经过 Review 以后，可能会进行必要的修改，修改在原分支修改，修改完毕代码合并进上游分支，原分支会定期删除。

项目组成员在收到合并成功的通知后，请自行从上游大特性分支向下合并到自己当前的开发分支。

提交 pull request 后创建新任务分支的时候务必知会一下相关配合同事（比如前端的同事），让他们在新的分支上继续开发。

2. 对于小功能，预计在 0.5-1 个（不超过 2 个）工作日工作量的开发任务，直接基于 develop 分支创建特性分支即可。

3. 在各个分支遇到的 bug，请基于该分支创建一个 Bug 分支。

如果在缺陷跟踪管理系统上有对应的项，命名请使用缺陷跟踪管理系统的 ID，比如 BAZABUG-1354 比如这个 Bug 的分支命名就是 bugfix/BAZABUG-1354。

如果在缺陷跟踪管理系统上没有对应的项，命名请简短的说明修改内容，比如 “JX 9df2b01 引用 bootstrap css 虚拟路径重写，避免出现字体无法找到的问题”，分支命名可以是 bugfix/miss-font。

完成修改以后提交并推送到中心仓库然后立即向上游分支提交 pull request。

4. 发起 pull request 以后，请将pull request的链接在IM上发给代码审核者，以此通知对方及时进行审核。

二、执行

我们在团队内部提倡质量优先，开发团队不能为了进度牺牲质量，并在团队内部达成了共识。所以，无论进度有多么紧迫，Code Review 的过程都一定会做。

所有的问题一定会被提出，只是会根据进度的紧迫程度，以及问题的大小，改动成本，决定问题是现在解决，还是加一个 TODO，并记录在缺陷跟踪管理系统内，以防日后遗忘。

多数情况下，我们都会要求立即解决，哪怕因此造成了发布的推迟。我们深知，其实多数情况下，现在不解决，日后不知道猴年马月才能解决。

我们在团队内推行 Code Review 的过程中没有遇到太多阻力。原因大概有两点：

首先，管理层方面了解之前遇到的各种问题，也迫切希望能有所改善，所以从一开始就是支持的态度。

其次，绝大部分开发人员觉得在这个过程中能自己能学习到东西，并没有抵触，遇到很好的意见时大家都还是很高兴的。

最后，慢慢的形成了一种氛围，整个团队都会自觉的维护它。

附一张我们审核的对话图，这位童鞋尝试对系统内部散落各地发业务邮件的代码做一个整理，用一套模式来处理，调整了 3 版才定调，然后修改了很多细节才通过了合并，前后大概用一个多星期时间：

表面上看来 Code Review 会延缓项目的进度，但是在我们 2 年多的执行过程中，大多数时候没感觉到有延缓。

原因是，虽然代码合并的周期变长了，但是由于代码质量提高了，导致 Bug 变少了，由于 Bug 引起的返工问题也变少了，因此整体的进度其实并没有延缓。

我个人认为对一个成熟的团队其实做 Code Review 反而会加快整体的项目进度，但是手头上没有统计数据支撑我的观点。

我们每个分支有权限合并的人都不止一个，这样可以保证有人请假不在的时候，代码仍然可以被其他同事审核通过之后合并。

半年前，我们团队加入了很多新成员，刚加入的新同事对规范、项目、产品的熟悉程度都不高，导致了有一段时间，我们遇到了PR审核周期变长的问题。

加上之前遇到的一些问题，我们总结了一个说明，目的是减轻Code Review对开发人员工作的负担，加快 PR 审核通过的过程。

说明如下：

Pull Request 的说明

• 任务完成才能提交 PR。

• PR 应该在一个工作日内被合并或者被拒绝。

• PR 在有严重问题（包括但不限于架构问题、安全问题、设计问题），太多问题，或者任务无效的情况下会被拒绝。

• 严禁一个PR里面有多个任务，除非它们是紧密关联的。

• PR提交之后只允许针对Review发现问题再次提交代码，除非有充足的理由，严禁在同一个PR中再次提交其它任务的代码。

• 提交PR时候有一个描述框，内容会自动根据 Commit 的 message 合并而成。切记，如果一次提交的内容包含很多Commit，请不要使用自动生成的描述。

• 请用简短但是足够说明问题的语言（理想是控制在3句话之内）来描述：你改动了什么，解决了什么问题，需要代码审查的人留意那些影响比较大的改动。特别需要留意，如果对基础、公共的组件进行了改动，一定要另起一行特别说明。

 审核人员邀请原则：

1. 在创建 PR 时，Reviewers（审核人）一栏里主要填写“必需审核人”。只有这些人审核都通过，才允许合并。

2. 除了“必需审核人”外，还有一些其它审核人，我们可以在 Description 里做为“邀请审核嘉宾”@进来。

3. 主干分支间的合并，如 Develop => Master，或 Master => Develop 等，则需要把整个团队（开发+QA）都列为“必需审核人”。

必须审核人的列表由团队决定，可能包括以下人选：

• 团队Leader

• 前端架构师（如果有前端代码改动） （可以授权）

• 后端架构师（如果有后端代码改动） （可以授权）

• 产品架构师

• 对此PR解决的问题比较熟悉的（之前一直负责这部分业务的同事）

• 此PR解决的问题对他影响比较大（比如认领的任务依赖此PR的同事）

其它审核人，包括但不限于：需要知悉此处代码改动的人但又不必非要其审核通过的同事，以及可以从这个 PR 中学习的同事。

可以授权指的是，根据约定，Bug 修复之类的改动，或者影响较小的改动，前端架构师和后端架构师可以授权团队内的某个资深开发人员，由这个资深开发人员代表他们进行审核。

主干分支之间的合并，大型 Feature 的合并，前端架构师和后端架构师需要参与。

三、收获

我们团队实施 Code Review 收获不少，总结出来大概有以下几点：

1. 短期内迅速提高了代码质量

原因有几个，大家知道自己的代码会被人审核之后写得会比较认真。理论上代码质量是由整个团队内最优秀的那个人决定的。大家也能在 Review 的过程中学习到其它同事优秀的编码。

2. Bug 数量迅速减少

但是这个我们没有数据统计比较，比较遗憾。我和 QA 聊过，他给我的数据是在我们的一个新项目每 2 周一次的大发布，平均只会发现 1~2 个 Bug。这点提高了整个团队的幸福感，大家不用经常被火烧眉毛。

3. 团队成员对项目的熟悉程度会比较均衡

新同事通过参与Code Review能很快熟悉团队的规范。代码不会只有个别人了解、熟悉，Bug谁都能改，新功能谁都能做。对公司来说避免了人员的风险，对个人来说比较轻松（谁都能来帮你），可以选自己喜欢的任务做。

4. 改善团队的氛围

Review 的过程中会需要非常多的沟通，多沟通能拉近团队成员的距离。并且无论级别高低，大家的代码都是要经过 Review 的，可以在团队内营造一个平等的氛围。每个成员都可以审查别人的代码，这很容易激发他们的积极性。

四、总结

虽然有合适的工具支持会更容易实施 Code Review，但它本身并不特别依赖具体的工具，所以前文并没有具体指明我们用了什么工具，除了 Git。

原因是基于分支的 PR 流程依赖于大量创建分支，而Git创建一个分支非常的简单，所以 PR 模式+Git 是一个很好的搭配。

---

Code Review 是软件工程中的一个重要环节，它不但可以提高代码质量，提前发现 Bug，还可以统一团队的代码规范，有益于形成团队技术氛围，加深技术团队成员沟通，以老带新互助成长等等。

为提升效率，我们可以将 Code Review 的内容分为固定规则和代码逻辑两部分。对于固定规则部分，借助于工具，完全可以自动化进行。这样开发人员在 Code Review 时只需要关注代码逻辑。

本场 Chat 作者：王书光，火币集团资深 iOS 工程师，本次分享的主要内容如下：

• Code Review 简介

• Objective-C 代码的自动化 Code Review

• Swift 代码的自动化 Code Review

• 实战演示

如对本场 Chat 感兴趣，可以识别二维码或点击阅读原文进行预订。