代码审查过程

　　对我而言，把代码产品化而没有合适的审查流程，像是一场抽抽乐游戏。代码当然也有可能会挺好，不过总还是有一定概率某人的哪块积木没抽好，然后一切轰然崩塌。无论是采用持续集成服务、结对审查、QA审查，还是所有这些方案的组合，都可以大大降低引入风险的概率。
　　编程团队规模已经超过了你的掌控能力
　　我在Think Through Math（下文简称 TTM）的工作有18个开发者组成的工程团队，其中有两位是QA专家。这算是有挺多人（相对而言）在同时编码了。我们的架构中有好几种不同的服务，所以不是每个人都能对代码的每个角落了如指掌。我们中有些人偏重于前端的工作，有些人侧重于数据仓库和报表，还有些人则在后端折腾Ruby代码。我们都会经常重新搭配分组以相互传播知识，不过始终还是有相当多的人在为不同的项目工作，而没有一个人能把整个系统吃透。
　　我知道在这种事上我们并不孤单——有其他开发者也处于差不多的情况。一些人只是按部班开发着应用，一些人则尝试把庞大的Rails应用拆分成小粒度的服务，还有一些人在处理遗留代码拯救项目。不管是什么类型的项目，都可以从某种恰当的代码审查流程获得很大收益。对按部班的开发项目而言，可以确保做修改，添加新特性时一切正常；对大型系统拆分而言，是可以在代码质量改进并正确分解的时候确保每个新服务都仍保持功能；而对于拯救项目，则是可以确保代码适配新标准时，所需的功能都正常。更棒的是，流程合理的话这些都可以自动完成。
　　TTM当前审查流程
　　在我们TTM的整个审查流程的迭代中，我不记得有哪次是不好使的，不过总有一些比其他更有效。新的一代似乎是目前为止好的（如人所愿），不过仍然存在改进空间。下一篇帖子里我会很乐意解释我们针对审查流程达成共识和改进的新方法，不过这不在本帖讨论范围。下面是我们当前审查流程执行的要点：
　　代码审查流程
　　首先，开发者（通常是一组结对），完成了bug修改，新特性或重构，把代码提交给Github。如果他们只是想要CI服务器在他们提交的代码分支运行测试套件，他们会打上我们的WIP（半成品）标签，这样其他开发者和机器人知道这次什么都不用做。这时（还有每次代码提交PR时），我们的Hound服务器会运行来检查我们的代码风格，以确保提交的代码的风格满足我们所选用的代码风格。
　　代码一旦绪，开发者为PR打上“需要代码审查”标签（Needs Code Review label）。他（她）还会附上完备的清单形式的关于QA该如何检测PR的指南。在预设的时间以后，我们的能干的聊天机器人，mathbot会来检查带着“需要代码审查”标签”的PR（Pull Request），给它们分配一两名其他的开发人员来审查代码。若是某个代码审查者觉得审查自己不熟悉领域的代码不太舒服，他们只需在聊天室里请求熟悉相关代码的人来替换。
　　我们通常会要求开发者仔细检视代码，而不是简单地做个人肉 lint 工具或者编译器。从我们的流程定义页面照搬的对审查风格的要求是：
　　●保持好奇，审查PR的作者为何如此实现；
　　●查找逻辑上的瑕疵，写反了的布尔判断，潜在的对象的错误选择，类型不匹配；
　　●寻求代码命名和动机的清晰；
　　●寻找可以优化的地方。太细的优化并不常需要，主要是能够避免的东西如：
　　●循环内部的偶发DB调用
　　●循环内部可以避免的繁重工作
　　●频繁使用和代价高昂的操作，却没有使用缓存或记录的机制
　　如果审查者或其中一人认为代码可以调整，他们会在Github的PR添加简短评论，开发者能获得一个很清楚的标注，说哪些地方有问题。之后审查者移除“需要代码审查”的标签并替换成“需要修改”的标签，提醒开发者关注评论，并修改实现，或者申辩这样编写代码的原因。
　　有一件重要的事情需要注意，这里所有的评论都是以尊重和积极的态度完成的。我们不是为了对别人评头论足。我们是为了帮助彼此尽可能写出好的代码，让整个系统更完善。自我提升是我们整个团队共有的目标，作为一个学习型的团队，我们总是在技术和实践上互相帮助。
　　要求的修改完成后，或者代码被证实合理后，开发者移除“需要修改”标签，重新标记为“需要代码审查”，这样审查者可以继续干活儿了。一旦他们都签发（sign off）了本次改动，“需要代码审查”标签翻成了“完成代码审查”，也是不需要再做代码审查了。如果不需要用户体验评估，那么”需要QA”标签会打上。
　　用户体验评估
　　我们的产品有广大受众——学生、学校的教职员、管理员等。所以我们非常关注我们版本的变化和用户界面/体验的变化。据此，一旦我们的系统前端做出了任何可能使站点上某项外观变化的改动，我们都需要为PR加上“需要UX”标签，以使UX小组的某人可以审查我们的变化。因为UX要改变通常也影响到代码，所以这个步骤一般和代码审查流程并行完成。如果需要再修改，审查者移除标签并设置为”需要修改“，如同之前代码的流程。一旦都好了，他们会移除”需要UX“标签并带上”需要QA“标签。