前几天看了《Code Review 程序员的寄望与哀伤》，觉得我们团队已经进行了2年的CodeReview，结果还是比较满意的。有些经验应该与您分享和讨论。我们为什么要实施代码审查？我们面临着混乱的代码和频繁的错误。当时觉得应该改点东西，希望能提升产品的代码质量，改善开发团队面临的困境。而且我个人有很多开发经验，希望这些知识能够在团队内部传播开来。经过多方考量，我们最终相信实施CodeReview可以改善或解决我们面临的很多问题。这篇文章的目的不是告诉你如何在团队中实施CodeReview，首先因为我只在一个公司实施过，并没有多少经验。其次，每个公司、每个团队的情况都不一样。应根据公司或团队的实际情况选择合适的方案，并根据成员的反馈及时进行调整，以促进CodeReview的实施。因此，本文介绍我们公司是如何实现CodeReview的，以及我们是如何解决遇到的问题的。希望我们的经历能给你带来一些帮助。写的仓促，如有遗漏或错误，敬请指正。经过对流程和规则的简单对比和尝试，我们最终采用了GitFlow+PullRequest（PR）模式进行CodeReview。（PR模式详情请参考Git工作流程指南：PullRequest工作流程）PullRequest（PR）简单来说就是你没有权限向特定的仓库或分支提交代码，你请求有权限的人提交您从Mergeyourrepositoryorbranch提交的代码到指定的repository或branch。由于PR需要有权限的人确认，所以很适合在这个过程中做CodeReview。接受还是拒绝取决于CodeReview的结果。在支持PR模式的软件中，每个PR都有一个用于新增代码的比较（diff）接口。CodeReview人员可以在线浏览新添加的请求合并的代码，并对有问题的代码行添加注释，从而实现CodeReview。每个有权查看存储库的人都可以看到评论，每个人都可以回复任何人的评论，有点像论坛中的讨论。这种模式是事后审核，即代码已经提交到中央仓库，审核过程中频繁变更会造成历史签入记录混乱。当然，Git可以使用变更历史来解决这个问题。因为容易误用，所以我们一般只在基础类库等要求严格的项目上实现。我们所知道的支持PR模型的软件都是使用Git作为源码版本控制工具，所以我们的源码版本控制工具也迁移到了Git。因为Git是如此的灵活，所以诞生了很多Git流程来规范Git的使用。常见的有集中式工作流、功能分支工作流、Gitflow工作流、Forking工作流、Github工作流。我们对GitFlow做了一些调整，调整后的进程命名为BazaFlow，定义如下。根据BazaFlow，我们的大多数存储库只定义了2个主干分支，master和develop。（例外，我们有一个仓库，有3个开发团队同时开发，定义了4个主分支，目前比较顺利，估计主分支之间合并会比较麻烦。）master对应的是生产环境代码，所有生产环境的发布源都是master分支的代码。develop对应本地测试环境的代码。大多数情况下，QA（测试）只测试develop分支和master分支的代码。由于开发人员都在同一个团队，所以我们没有使用基于仓库的PR，而是基于分支的PR。我们限制了总支的操作权限，只有特定的人才能操作。develop分支是项目开发负责人和架构师，master分支是QA。有权限合并到主分支的成员将按照约定的规则进行合并，不会合并未经审核的PR。上面的一点其实挺重要的，所以对于有合并权限的，什么情况下可以合并代码，我们会有专门的约定。（见下面PR的说明）PR的发起者要积极推动PR的review，Leader也会密切关注PR的review进度，必要时及时介入。我们将CI服务器（什么是CI）配置为只编译特定的分支，通常是develop和master分支。所有代码合并到主分支后，会自动触发本地测试环境的编译和发布。QA不需要依赖开发人员编写的代码进行测试，也不需要手动操作这些，保证了开发人员和测试人员的独立性。我们本地测试环境的发布包括数据库和站点的发布。它是全自动的。发布完成后，就是一个可用的产品。如果你有时间，你也可以分享这部分。我们在Scrum中还使用了一个非常重要的概念：完成的定义。也就是我们规定，我们的其中一个任务的完成定义为：代码写好，自测，提交的PR审核通过，合并到主分支。也就是说，所有的代码都合并到主分支中才算完成任务，而合并到主分支中的必须经过CodeReview，这是强制性的。BazaFlow目前的版本是V0.9BazaFlow是从GitFlow演变而来的。GitFlow的开发模型如下图所示：由于我们托管软件对PullRequest的限制，我们对GitFlow进行了改动。变化如下：1、我们将为每个主要功能创建一个单独的特性分支，项目开发人员基于这个单独的特性分支创建自己的任务分支。例如，对于一个CS2项目，启动时的分支创建是：master->develop->feature/v2。开发者应该基于这个大特性分支feature/v2创建自己的任务分支，比如创建XXXX，使用单独的分支feature/v2-xxxx。完成此任务后，立即向上游分支（feature/v2）提交拉取请求。然后从feature/v2-xxxx创建自己的下一个任务分支，比如YYYYeditfeature/v2-yyyy。请注意，合并到上游分支的功能必须是相对独立且可用的。分支任务的工作量为0.5-1个工作日，不应超过2个工作日。如果超过2个工作日，则不会合并到上游，需要向团队说明。代码经过审查后，可能会进行必要的修改，修改在原分支中进行。修改后代码合并到上游分支，原分支会定期删除。项目组成员收到合并成功通知后，请从上游主要特性分支合并到自己当前的开发分支。提交pullrequest后创建新任务分支时，一定要通知相关合作同事（如前端同事），让他们在新分支上继续开发。2、对于小功能，估计0.5-1（不超过2）个工作日工作量的开发任务，可以直接基于开发分支创建特性分支。3、对于各个分支遇到的bug，请在该分支的基础上创建一个bug分支。如果缺陷跟踪管理系统中有对应项，名称请使用缺陷跟踪管理系统的ID，如BAZABUG-1354。例如这个bug的分支名称是bugfix/BAZABUG-1354。如果缺陷跟踪管理系统上没有相应的条目，请在名称中简要描述修改内容，如“JX9df2b01指的是bootstrapcss虚拟路径重写，避免找不到字体的问题”，分支名称可以是bugfix/miss-font。修改完成后，提交并推送到中央仓库，然后立即向上游分支提交pullrequest。4.发起pullrequest后，请将pullrequest的链接发送给IM上的codereviewer，以便及时通知对方进行review。2.实施我们在团队内部提倡质量第一。开发团队不能为了进步而牺牲质量，团队内部已经达成共识。所以，不管时间有多紧迫，CodeReview流程肯定会做。所有的问题肯定会提出来，但是会根据进度的紧急程度、问题的大小、修改的成本来决定是现在解决问题还是增加TODO，并记录在缺陷跟踪中管理系统，以防止它在未来被遗忘。在大多数情况下，我们会要求立即修复，即使它会延迟发布。我们很清楚，其实很多情况下，现在不解决，以后就不知道怎么解决了。我们在团队内部实施CodeReview的过程中并没有遇到太大的阻力。大概有两个原因。一是管理层了解之前遇到的各种问题，急于改进，所以一开始就支持。其次，绝大多数的开发者都觉得在这个过程中自己可以学到一些东西，没有抗拒感。遇到好的意见大家都很开心。***，慢慢形成一种氛围，全队都会自觉维护。附上我们审核的对话图。本童鞋试图将散落在系统各处的商务邮件发送代码整理出来，并用一套模型进行处理。调了3个版本定了基调，然后修改了很多细节才通过。合并前后大约需要一周左右的时间：从表面上看，CodeReview会拖延项目的进度，但在我们2年多的实施过程中，大部分时间并没有感觉到有任何拖延。原因是代码合并的周期虽然变长了，但是由于代码质量的提升，bug变少了，bug带来的返工问题也变少了，所以其实并没有耽误整体进度。我个人认为在一个成熟的团队上做CodeReview其实会加快整个项目的进度，但是没有统计数据来支持我的观点。（关于软件开发的衡量，欢迎有经验的同学告诉我）我们每个分支都有不止一个人有合并的权利，这样当有人请假的时候，代码还是可以合并的其他同事的评论。半年前，我们的团队加入了很多新成员。刚入职的新同事，对规格、项目、产品都不是很熟悉。因此，有一段时间，我们遇到了PR审核周期过长的问题。除了之前遇到的一些问题，我们总结了一个说明，目的是减轻开发者CodeReview的负担，加快PRreview的进程。说明如下：任务完成后才能提交pullrequest。PR应在一个工作日内合并或拒绝。如果存在严重问题（包括但不限于架构问题、安全问题、设计问题）、问题过多或任务无效，PR将被拒绝。严禁在一个PR中有多个任务，除非它们密切相关。PR提交后，只有Review发现的问题才允许再次提交代码。除非有充分理由，否则严禁在同一个PR中提交其他任务的代码。提交PR时，有一个描述框，内容会根据Commit消息自动合并。记住，如果一个commit的内容包含很多Commit，请不要使用自动生成的描述。请用简短但足够的语言来描述问题（最好在3句话以内）：你改变了什么，解决了什么问题，需要codereview的人应该关注那些影响比较大的改变。需要特别注意的是，如果更改了基本和公共组件，则必须添加新的一行特殊说明。邀请审稿人的原则：1.创建PR时，主要在Reviewers（审稿人）一栏填写“requiredreviewers”。只有所有这些人都通过审查才允许合并。2.除了“requiredreviewers”之外，还有一些其他的reviewers，我们可以在Description中作为“invitedreviewsguests”@进来。3.主要分支之间的合并，比如Develop=>Master，或者Master=>Develop等，需要将整个团队（development+QA）列为“requiredreviewer”。审核人名单由团队确定，可能包括以下候选人：TeamLeader前端架构师（如果有前端代码改动）（可以授权）后端架构师（如果有后端codechange）（可以授权）产品架构师这个PR解决的问题比较熟悉（以前负责过这部分业务的同事）这个PR解决的问题对他影响比较大（比如，(theclaimedtaskdependsonthecolleagueofthisPR)其他reviewer，包括但不限于：需要知道可以从这个PR学习的同事可以在这里授权修改代码的人，但不一定要通过review。就是说，根据约定，对于bug修复等变更，或者影响较小的变更，前端架构师和后端架构师可以授权团队中的资深开发人员代为进行审核。主干分支之间的合并，大特性的合并，需要前端架构师和后端架构师共同参与。上述审稿人的视角不同：组长看的是你是否完成任务，前后端架构师看的是是否符合公司统一的架构、风格、质量，而产品架构师是从整个产品层面来关注这个PR的。熟悉问题的同事可以更好的保证问题得到解决，不会引入新的问题。受影响的同事可以及时了解他的影响。如果teamleader或者productarchitect觉得PR邀请的reviewer不够或者太多，需要调整到合适的人员，其他同事可以在评论中提出建议。3.收获我们团队在实施CodeReview过程中收获颇丰，总结起来有以下几点：1.代码质量在短时间内得到了快速提升。有几个原因。每个人都知道他们的代码会被更认真地审查和编写。理论上，代码质量是由整个团队中最优秀的人决定的。还可以在评审过程中学习到其他同事的优秀编码。2.bug数量快速下降。但是我们没有统计数据来比较这个，这是一个遗憾。我和QA聊了聊，他给我的数据是，我们的一个新项目，每2周有一个大release，平均只会发现1~2个bug。这样提高了整个团队的幸福感，大家也不用经常被烧死。3、团队成员对项目的熟悉程度会更加均衡。新同事可以通过参与CodeReview快速熟悉团队规范。代码不会只有少数人知道和熟悉，任何人都可以修复错误，任何人都可以制作新功能。对于公司来说，避免了人员的风险，对于个人来说更容易（任何人都可以帮助你），你可以选择你喜欢做的任务。4.改善团队氛围。审核过程会需要很多沟通，多沟通可以拉近团队成员之间的距离。而且不管级别高低，每个人的代码都要review，这样可以在团队内部营造一种平等的氛围。每个成员都可以review别人的代码，很容易激发他们的积极性。亮化我们的数据：我们从2014年1月17日开始提交第一个PR，到2016年7月5日共发出6944个PR，其中通过6171个，拒绝739个。日均11.85个PR，最多的一天55个PR。这些PR总共产生了30,040条评论，平均每个PR有4.32条评论，最大的PR有239条评论。共有53位同事参与了上述PR评论，每位同事平均发送了539条评论。最多的用户发了5311条评论，最少的发了1条（刚实施CodeReview就离职的同事）。澄清一下，只会通过评论提出简单的问题。比较复杂的问题，比如架构、安全相关的问题，其实我们会面对面沟通，因为效率更高。4.小结虽然在合适的工具支持下实现CodeReview比较容易，但是并不特别依赖于具体的工具，所以上一篇没有具体说明我们使用了哪些工具，Git除外。原因是基于分支的PR过程依赖于创建大量分支，而Git创建分支非常简单，因此PR模式+Git是一个很好的搭配。在我们切换到Git之前，我们也做了CodeReview，采用提交代码后将commitID发送给相关同事进行review的流程。审核通过后，将在缺陷跟踪管理系统中进行评价。QA同事如果没有看到审核通过的评论就认为任务没有完成，拒绝测试。虽然不像现在这么直接方便，但还是做了。在PR审核过程中，新加入的团队成员普遍存在的问题是不符合代码规范。其实可以通过源码检查工具来解决。这部分我们一直在规划((╯□╰))，一直没有实现。
