咱们是怎么作Code Review的

前几天看了《Code Review 程序员的寄望与哀伤》，想到咱们团队开展Code Review也有2年了，结果还算比较满意，有些经验应该能够和你们一块儿分享、探讨。
咱们为何要推行Code Review呢？咱们当时面临着代码混乱、Bug频出的情况。
当时我以为要有所改变，但愿能提升产品的代码质量，改善开发团队面临的困境。而且我我的在开发上有不少经验，也但愿这些知识可以在团队内传播。
各类考虑后，咱们最后认为推行Code Review能改善或解决咱们面临的不少问题。

这篇文章的目的不是告诉你们怎么在一个团队内推行Code Review，首先由于我我的仅在一家公司内推行过，并无不少经验。
其次每家公司、每一个团队的状况都不太同样，应该根据公司或团队的实际状况选择恰当的方案，并根据成员的反馈来及时调整，推进Code Review的实施。
因此，本文是介绍咱们公司是如何实施Code Review的，咱们是如何解决咱们遇到的问题的，但愿咱们的经验能给你们带来些帮助。
行文仓促，若有遗漏或错误，欢迎指正。

1、流程和规则

通过简单的对比、试用，咱们最后采用了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，定义见后文。
根据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

当前版本 V0.9

Baza Flow 由 Git Flow 演化而来，Git Flow的开发模式以下图所示：

因为咱们的托管软件对于Pull Request的限制，咱们对Git Flow作了改动，改动的地方有：
一、每个大功能咱们会建立一个单独的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后建立新任务分支的时候务必知会一下相关配合同事（好比前端的同事），让他们在新的分支上继续开发。

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

三、在各个分支遇到的bug，请基于该分支建立一个Bug分支。
      若是在缺陷跟踪管理系统上有对应的项，命名请使用缺陷跟踪管理系统的ID，好比BAZABUG-1354 好比这个Bug的分支命名就是bugfix/BAZABUG-1354。
      若是在缺陷跟踪管理系统上没有对应的项，命名请简短的说明修改内容，好比“JX 9df2b01 引用bootstrap css虚拟路径重写，避免出现字体没法找到的问题”，分支命名能够是bugfix/miss-font。
      完成修改之后提交并推送到中心仓库而后当即向上游分支提交pull request。
四、发起pull request之后，请将pull request的连接在IM上发给代码审核者，以此通知对方及时进行审核。

2、执行

咱们在团队内部提倡质量优先，开发团队不能为了进度牺牲质量，并在团队内部达成了共识。
因此，不管进度有多么紧迫，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的合并，前端架构师和后端架构师须要参与。 

上述审核人关注的视角不太同样：
团队Leader关注你是否完成了任务，先后端架构师关注是否符合公司统一的架构、风格、质量，产品架构师从整个产品层面来关注这个PR。
熟悉此问题的同事能够更好的保证问题被解决，确保没有引入新问题。
被影响的同事能够及时了解他受到的影响。 

团队Leader或者产品架构师若是以为PR邀请的审核者不足或者过多，必须调整为合适的人员，其它同事能够在评论中建议。

3、收获

咱们团队实施Code Review收获很多，总结出来大概有如下几点：

一、短时间内迅速提升了代码质量。
     缘由有几个，你们知道本身的代码会被人审核以后写得会比较认真。
     理论上代码质量是由整个团队内最优秀的那我的决定的。
     你们也能在Review的过程当中学习到其它同事优秀的编码。

二、Bug数量迅速减小。
    可是这个咱们没有数据统计比较，比较遗憾。
    我和QA聊过，他给个人数据是在咱们的一个新项目每2周一次的大发布，平均只会发现1~2个Bug。
    这点提升了整个团队的幸福感，你们不用常常被迫不及待。

三、团队成员对项目的熟悉程度会比较均衡。
    新同事经过参与Code Review能很快熟悉团队的规范。
    代码不会只有个别人了解、熟悉，Bug谁都能改，新功能谁都能作。
    对公司来讲避免了人员的风险，对我的来讲比较轻松（谁都能来帮你），能够选本身喜欢的任务作。

四、改善团队的氛围
    Review的过程当中会须要很是多的沟通，多沟通能拉近团队成员的距离。
    而且不管级别高低，你们的代码都是要通过Review的，能够在团队内营造一个平等的氛围。
    每一个成员均可以审查别人的代码，这很容易激发他们的积极性。

亮一下咱们的数据:

咱们从2014年1月17日开始第一个PR的提交，到2016年7月5日一共发出了6944个PR，其中6171个经过，739个拒绝。日均11.85个PR，最多的一天提了55个PR。
这些PR一共产生了30040个评论，平均每一个PR有4.32个评论，最多的一个PR有239个评论。
参与上述PR评论的同事一共有53位，平均每位同事发出了539个评论，最多的用户发出了5311个评论，最少的发了1个（刚推行Code Review就离职的同事）。
须要说明一下，只有简单的问题会经过评论来提出。比较复杂的，好比涉及到架构、安全等方面的问题，其实都会面对面的沟通，由于这样效率更高。

4、总结

虽然有合适的工具支持会更容易实施Code Review，但它自己并不特别依赖具体的工具，因此前文并无具体指明咱们用了什么工具，除了Git。
缘由是基于分支的PR流程依赖于大量建立分支，而Git建立一个分支很是的简单，因此PR模式+Git是一个很好的搭配。
咱们在切换到Git以前，也作Code Review，采用的是提交代码之后把commit的Id发给相关同事来审查的流程。
审核经过之后会在缺陷跟踪管理系统里面评论，QA同事没见到审核经过的评论就认为任务没有完成，拒绝进行测试。
虽然没有如今这样直接方便，可是也仍是作起来了。

PR审核的过程当中，新加入的团队成员常见的问题是不符合代码规范之类的，实际上是能够经过源代码检查工具来解决的，这部分咱们一直在计划中（( ╯□╰ )），并无开始实施。

转自：https://www.cnblogs.com/wenhx/p/How-We-Code-Review.html