聊聊 code review 的动机和方案选型

引子

有朋友问到code-review应该怎么做，有没有什么规范或者最佳实践呢？
很早之前就想聊一聊这个话题，一直没有时间，这次专门来讨论一下 code review 的动机和选型的话题。
讲过 code review 的优秀文章已经很多了，我希望从“找出事物背后驱动它的动因”的方式来分析一下为什么要 code review，该怎么 code review。

基础

• 对git工具，git branch，merge conflict有概念
• 对 code review 的概念有基本的认识

如果对Git不是很熟悉的同学，可以参考 Git Tutorial - Git 入门教程
关于 code review : 维基百科

进行 code review 的动机

所有范式和解决方案存在的理由都是为了“解决问题”。

code review 能干什么

review 的过程本质上是一个沟通的过程，通过一来一回的 comment 和 反馈，可以

• 增强代码的可读性
• 增强可维护性（可读性提高的副产品）
• 知识共享和经验传递
• 发现一些潜在的bug

如果将代码质量分为 1）可编译，2）可运行，3）可测试，4）可读，5）可维护，6）可重用
在 team-work 中 review 之前的代码，通过测试的情况下只能达到3的水平，review 之后的代码则可以达到4甚至更高的水平。

code review 不能干的事情

• 彻底消除bug
• 缩短工期
• 统一代码风格

code review 的组成部分

我们先从 code review 的组成部分开始，可以从它的组成部分，也可以逆推出 code review 适用的场景，它解决的问题，并且回答“我们是不是应该进行 code review” 这个问题。

code review 通常由以下部分组成

• 需要进行 code review 的项目
• 进行 code review 的人
• 用于进行 code review 的工具链/工作流

需要进行 code review 的项目

什么样的项目需要进行 code review ？

• 项目的重要性：项目是否需要严格的代码审计？对 安全漏洞/逻辑错误 的容忍度有多高？ 要求高的项目需要 code review 流程来提高质量。
• 项目的规模：项目复杂还是简单？相对独立还是比较混乱？复杂的项目相对来说更加需要 code review 来减少错误。
• 是否协同开发： 项目是否有复数个维护者？如果是一个人维护，则对 code review 的需求会小很多。
• 项目的生命周期：项目是一次交付不需要维护，还是需要一直持续更新维护？需要长期维护的项目对代码质量有更高的要求。
• 开发周期：项目有多少时间可以用来开发？code review需要额外的时间来进行，可能是开发用时的两倍甚至更多；如果时间不够，可能 人力/时间 资源没法让你进行 code review -- 每一次工程都是质量向deadline妥协的结果，不过也请记住，欠的技术债，还的时间越晚，利息就会越高。

如果上述问题都没有，很可能你的项目没有进行 code review 的必要，自己做得开心就好：）

进行 code review 的人

人是 code review 得以进行的必要元素之一，code review 过程的主体是代码编写者，执行者就是 reviewer，负责进行 review 的过程，为 author 提供反馈。
因此 code review 得以进行的基础是2个以上的人一起为同一个项目工作。

code review 的核心是人的共识，共识有两种

• review 前的共识
• 在 review 中希望达成的共识

review前的共识

review 之前的共识，应当是具体的东西（否则无法达成共识），需要事先约定，大家约法三章，之后像宪法一样遵守它。
具体的比如：

• 代码风格(Style Guide)： 这种Guide涉及开发的方方面面，可以用人家现成的风格，改造之后适合自己团队的现状就好。
• 项目约束：比如包的管理方式，相互之间的交互方式

大部分代码风格问题可以交给机器来解决，各种语言的lint工具（比如flake8），甚至lint工具能帮你发现一些逻辑上的错误。
所以，其实需要 review 的 “代码风格问题” 主要是在命名规范上面，比如能不能用拼音，用全称还是缩写，常量要怎么写？诸如此类。

review 中希望达成的共识

这个共识的内容应当是不那么具体的事情，因为不那么具体的事情一定会受到人的主观的影响

• 完成核心目标： 增强代码质量，具体来说，是可读性，可维护性，可重用性等等。
• 完成所选工作流的核心目标：后面对比工具链和工作流的时候回详谈。
• 传递经验：比如新成员接管老系统，老成员可以在 review 中具体问题具体分析，带新成员熟悉项目。

这些东西受到个人主观的影响非常大，所以在 review 中会产生很多矛盾，有一些小Tips可以帮助你解决部分问题。

工具链/工作流

工具链和工作流，是无法分离的整体，工具链直接决定了你能使用的工作流，所以对于工具链的选型，直接影响了你能使用的工作流。

我接触过的两种工作流，一种是基于gerrit + Jira + CI 的工作流，另一种使用 github/gitlab + CI 作为项目管理工具。

• Gerrit + Jira + CI：Gerrit 是 Google 推出的代码review工具，和Jira有良好的集成，Gerrit工作流中，review的单位是 commit， 特色是可以管理 不同的 commit 之间的依赖关系OpenStack的GerritWorkflow
• Github + CI / Gitlab ：Github 和 Gitlab 使用更加“现代”的工作流， 参考git-workflow

具体的工作流实施方式，可以参考工作流后面附带的链接，我主要想聊聊用Gerrit来管理多Production的工作方法(感谢老东家SmartX)。

用 Gerrit 工作流管理多Production版本的方法

• Gerrit 工作流中最核心的命令是 rebase， 你需要每次发起review之前，保证自己的代码已经从当前 master 上执行了 rebase，这保证你的代码在被review的时候总是没有冲突的。
• Gerrit 工作流非常强调每个单独的 commit 都需要是能单独合入master并且不会产生副作用的， Topic 是 Gerrit 用来 组织一个Feature的单位，但理想情况下，每个 Gerrit Commit都是是一个可以的独立运行的Feature。
• 管理多分支是 gerrit 工作流最擅长的事情，尤其对于2B类产品，维护多个生产版本是很常见的事情。
• 因为 Gerrit 每个 review 的单位是 commit 同时强调每个 commit 尽量小， 所以 review 起来工作量相对较小。
• 每个 commit 都要求 CI 能跑过。

如图所示，master 分支就是 Gerrit 工作流的开发分支，所有的commit都会被先合入主分支，待验证通过后，被 cherry-pick 到生产分支，对于过老的生产分支，可能只需要 cherry-pick bugfix 而不会将新的feature给 backport 到老分支。

正是因为强调每个commit的独立性和每个 commit 的内容的精简，cherry-pick 才能在 gerrit 工作流中工作良好。

对于 Github 类的 workflow，一个 feature-branch 可能有数十个 commit， 显然没法做到这么细粒度的管理。

用 Github/Gitlab 工作流管理单Production版本

本质上，用github工作流管理多分支多版本发布的区别是不大的，直接参考链接里面阮一峰老师的多版本的图，也是通过cherry-pick把新版本的feature应用到老版本。

区别主要是以下几个：

• cherry-pick 在应对 feature-branch这样有很多commit的时候，难以形成有效的提交记录，界限非常不明确，在稳定发行版中，可能会造成revert的时候的巨大困难。
• feature 难以被以很低的粒度应用到老版本，还是feature-branch的问题，feature-branch包含的commit太多会导致应用困难
• feature-branch review 起来困难比较大，因为代码量可能会很大。
• 优点是每一次从别的 branch 合并到主分支，能容纳非常大的feature提交，对于粒度控制的要求相对较低，降低了设计和编码的成本，以解决问题为目的。
• feature-branch 提交的时候，CI 只需要最后通过即可，所以可能会造成 cherry-pick 某个 commit 后，break掉整个分支。

团队应该根据 code review 的动机进行选型

综上所述，如果你的项目倾向于2C的产品，生产版本始终只有一个，或者最多多个预发行测试版，使用github/gitlab 工作流可能会更加轻松，用起来更加符合人类习惯。

如果你需要同时维护很多稳定发行版本，使用github/gitlab工作流，需要很多额外的规范和工作量，此时使用 gerrit 工作流，能给你带来很多好处，省很多心。

如果你们只是为了相互传递知识，其实任意一种方式都可以，但是出于简单起见，我推荐使用 github/gitlab 的工作流，发PullRequest 然后大家 Approve 就可以了。

code review 容易遇到的常见问题

冲突

代码冲突

代码冲突是每个团队合作都会遇到的问题。
我过去的工作中，一般遵循如下规则

• 每个feature提交者在提交的时候必须保证自己的代码和master没有冲突，并且必须使用rebase来保证fast-ward，避免将主分支merge到feature-branch，这样会产生非常混乱的提交历史，几乎不可读。
• 充分测试，起码要测试到代码冲突的部分，否则难以保证解决冲突的代码是没有问题的。

观念冲突

• 对于风格分歧较大： 保证相互交互的API部分的共识即可，其他部分爱怎么写怎么写，只要能过lint就行
• 实现方式分歧较大：这个必须要达成共识，有必要找第三者 review 并确定最后方案
• 基于以上原因，每个 commit 很小，可以减少实现完了一个巨大的功能然后 review 的时候才发现不能被接受的情况。

小Tips

• 永远记住，code review 是为了和人沟通，就事论事，没有谁优谁劣。保持这种态度，你的 review 进程的推进会容易很多。
• CI 是 code review 中基础的基础，CI 系统的建设是 code review 得以进行的基础，一旦系统有了CI和lint，没有人再有借口开脱自己写出了烂代码。
• 烂代码是阻止不了的，唯一的阻止方法是：不要和这个人维护同一个项目。
• 代码的可读性永远比简洁更加重要，甚至比性能更加重要，因为这个项目是需要继续维护下去的。
• 在修改成本较低的情况下，可以选择更优的实现方案，这种事情可以放在 code review 做，但是不要因为强迫症而强迫要一个人使用一个成本高昂的但是收益很低的方案。
• Typo是可以提出的，因为机器没法检查到所有的Typo。
• KISS，保持代码的简单易懂，对于大部分需求，只要实现最简单的一个函数也许就能解决，不要尝试实现一个复杂的范式或者一个通用度极高的抽象，因为将来需求会变化的，而且不一定和你的抽象一致，所以保持简单吧，将来改起来也容易，不会牵连过多。
• 英文命名方面，多查查词典吧：）
• 没法统一用英文的时候，就用中文写 commit message吧，后来的人会因为能读懂你写的 commit message 而感谢你的（比半吊子英语好太多了）。

题外话：team-work的品味

暂时只讲几个关键词吧

• 看完整个需求文档，包括不是自己做的部分，明确自己的功能设计能不能满足其他部分的需求（说起来容易做起来难）。
• 相互交互的API保持一致，事先就商量好。
• 为别人提供服务，让别人不关心你的实现，调用你的API就好，最好是做好封装而不是只写文档。