代码审查的必要性和最佳实践

落日映苍穹つ 2023-01-22 15:00 12阅读 0赞

**目录**

代码审查的流程

代码审查的争议

加班要累死了，完成项目都来不及，还做什么代码审查？

代码审查太费时间，改来改去无非是一些格式、注释、命名之类不痛不痒的问题。

团队的习惯和流程就是不做代码审查，大家都是这么过来的。

代码审查不利于团建，因为经常有程序员因为观点不同在代码审查的时候吵起来。

代码审查的好处

代码审查是个人和团队提升的最佳途径之一。

识别出设计的缺陷，找到安全、性能、依赖和兼容性等测试不易发现的问题。

设立团队质量标杆的最佳实践方式

代码审查工具

一些 Code Review 工具：

代码审查建议

建议一：小批量——每次 Review 的代码量要少

建议二：多批次——Review 要频繁发生

建议三：找对人——合适的 Reviewer

建议四：快速响应

建议五：使用现代工具

建议六：考虑结对编程

建议七：综合在线 Review 和线下 Review

建议八：及时表达肯定，委婉表达意见。

建议九：激励机制：激发主观能动性

--------------------

> 不做code review基本山就是背着定时炸弹前进：什么时候炸了都不知道。
> 
> 现在不做， 将来就是救火队员。
> 
> 好的code review会减少大量的不确定的坑：这个就像我们日常出门上班会确定门是否关好一样

# 代码审查的流程 #

先来简单介绍一下常见的代码审查的流程。为了开发某个新特性，或者修复某个特定问题，负责的程序员会从代码库的主分支（master branch）上面建立并 check out 一个新的分支，将工作分为一次到若干次的“代码变更”来提交。这每一次的代码变更，都可以组成一次代码审查的单元，有的公司把它叫做 CR（Code Change），有的叫做 PR（Pull Request），还有的叫做 CL（Change List），但无论叫做什么，它一般至少包含这么几项内容：

*  **帮助理解代码的描述**，如果有问题单（任务）来跟踪，需要包括相关的问题单号或者问题单链接；
 *  **实际的代码变更主体**，包括实际的代码和配置；
 *  **测试和结果**，根据项目的情况，它可以具备不同形式，比如单元测试代码，以及手工测试等其它测试执行的结果说明。

多数情况下，以上这三项都不可或缺，缺少任何一项都会让代码变更失去一定可审查的价值。

**进行审查的，一般是一起工作的，对代码涉及变更熟悉的其他程序员。这个“熟悉”，既包括业务，也包括技术**，二者当中，有一项不具备，就很难做好审查工作，给出有建设性的审查意见。

接下去的交互就在这个代码变更上面了，审查者会提出其问题和建议，变更的作者会选择性采纳并改进变更的描述、代码主体以及测试。**双方思考、争辩，以及妥协，目的都是寻求一个切合实际且可以改进代码质量的平衡。**

如果审查的程序员觉得代码没有太多问题，就会盖上一个“Approved”或者“Shipped”戳，表示认可和通过。这根据项目而定，一般代码变更最少要得到 1~3 个这样的认可，才可以将代码变更合并（merge）到主分支。而主分支的代码，会随着 CI/CD 的流程进入自动化的测试程序，并部署上线。

## 代码审查的争议 ##

在介绍代码审查的好处之前，我想先来谈谈争议。因为我观察到大多数的争议，都不是在否认代码审查的好处，而是聚焦**在不进行代码审查的那些“原因” 或者 “借口”上**，而有些讽刺的是，我认为这里面大部分的“原因”，所代表着的因果关系并不成立。

## 加班要累死了，完成项目都来不及，还做什么代码审查？ ##

> 类似的问题还有，“代码审查拖慢了进度”，“代码审查不利于快速上线”。这是最常见的不做代码审查，或者草草进行代码审查的理由了，但是稍稍一细想，就会发现这里的因果逻辑完全不对。
> 
> 这就像以前国内大兴“敏捷”的时候，有好多程序员，甚至项目经理，觉得因为项目时间紧才要实施敏捷，因为可以少写文档，少做测试，随意变更需求，可这里的因为所以根本是牛头不对马嘴。我记得知乎上有句流行的话叫做，“先问是不是，再问为什么”，这里也可以用，因为项目压力大就让“不做代码审查”来承担后果，这实在是过于牵强了。
> 
> 项目压力大，时间紧，可以草草做分析，不做设计，直接编码，不做重构、不做测试、不做审查，直接上线，快及一时，可是造成的损失，最后总是要有谁来背锅的。这个锅很可能，就是**上线后无尽的问题，就是恶性循环加班加点地改问题，就是代码一个版本比一个版本烂。当这些问题都焦头烂额，就更不要说团队和程序员的成长了。**

> ## 代码审查太费时间，改来改去无非是一些格式、注释、命名之类不痛不痒的问题。 ##
> 
> 这也是个逻辑不通的论述，虽然这个还比前面那个稍微好一点。只能提出这些“次要问题”，很可能是代码审查的能力不够，而并非代码审查没有价值；或者是代码审查的力度不够，只能提出一些浅表的问题，这个现象其实更为普遍。
> 
> 前面已经介绍过了，**一是技术，二是业务，二者缺一都无法做出比较好的审查**。在某些特殊情况下，有时候确实不具备完备的代码审查条件，我们现在来分业务、技术欠缺的情况进行讨论。
> 
> 如果团队中有业务达人，但是技术能力不足。比如说，新版本使用的是 Scala 来实现的，但是团队中没有精通 Scala 的程序员，这个时候可以寻找其它团队有 Scala 经验的程序员来重点进行技术层面的代码审查，而自己团队则主要关注于业务逻辑层面。当然，既然是自己团队的代码，所用到的技术要慢慢补起来。
> 
> 如果团队的成员具备技术能力，但是业务不了解。这种情况也可以进行将业务和技术分开审查这样的类似处理，但是如果业务相对复杂，可以先开一个预审查会，就着代码或者设计文档，简单地将业务逻辑介绍和讨论清楚，再进行审查。

## 团队的习惯和流程就是不做代码审查，大家都是这么过来的。 ##

我觉得这也不是一个论述不应该做代码审查的正当理由，类似的还有“绩效考评又不提代码审查”，以及“**我上班、码代码、下班、拿钱，审查代码干什么**”。大家都不做，并不代表不做就是正确的，如果你赞同代码审查的好处和必要性，那么你的思考会告诉你，应该做这件事情，大家不做并不是一个理由。

> 从来如此,便对吗

如果你发现这件事很难推动，你可以尝试去和你的项目经理聊一聊，或者结合自己的项目以及下面会讲到的代码审查的好处论一论，看看是不是能说服那些没有意识到代码审查好处的程序员和项目经理。当然，这是另外和人沟通以及表达自己观点的事情，如果大家都是朴素的干活拿钱的观点，没有对于代码质量和个人发展更高的追求，或者价值观和你相距十万八千里，改变很困难，你就应该好好思考是不是应该选择更好的团队了。

## 代码审查不利于团建，因为经常有程序员因为观点不同在代码审查的时候吵起来。 ##

这依然不是一个正当理由，这就好像说“**因为开车容易出交通事故，所以平时不允许开车**”这样荒谬的逻辑一样。

首先，如果有**偏执的不愿意合作的程序员**，那么不只是代码审查，任何需要沟通和协作的活动都可以把争吵的干柴点燃。对于这样的程序员的管理，或者如何和这样的程序员合作，是另外的一个话题，但这并不能否认代码审查的必要性。当然，在下文讲到实践的部分我会介绍一些小的技巧，帮助你在代码审查中心平气和地说服对方。

其次，**有控制的一定强度内的争执，未必是坏事。有句话叫做“理越辩越明”**，除了能做出尽可能合理的决定以外，在争论的过程中，你还会得到分析、思考、权衡、归纳、表达，乃至心理这些综合能力的锻炼，本来它们就不是很容易得到的机会，我们为什么还要放过呢？

# 代码审查的好处 #

下面我们来谈谈代码审查的好处。你可能会想，这有什么可谈的，这好处难道不是发现软件 bug，提高代码质量吗？

别急，代码审查的好处可远远不止这一个，我觉得它还至少包括下面这些好处。

## 代码审查是个人和团队提升的最佳途径之一。 ##

这里的学习，既包括技术学习，也包括业务学习。和英语学习一样，如果只听 BBC 或者 VOA 的纯正口音，没有任何语法错误，英文反而不容易学好，学英文就要接触生活英语，各种口音，各种不合标准的习惯用法。阅读代码也一样，要学习不同的代码风格和实现。

在做代码审查的时候，如果不理解代码，是无法给出最佳审查的。因此自己会被迫去仔细阅读代码，弄懂每一行每一个变量，而不是给一个 LGTM（“Looks Good To Me”）了事。

代码审查是团队关系建设和扩大双方影响力的有效方式。

争论是这个过程中必不可少的一环，争论除了能加深对于问题和解决方法的理解，在不断的反驳和妥协中，也能树立影响力，建立良好的关系。另外值得一提的是，**代码审查可不是说非得给别人挑刺儿，对于做得特别漂亮的地方，要赞扬，这也是建立良好关系的一种途径。**从团队合作和交流的角度来说，程序员往往缺乏沟通，每个人不能只专注于自己的那一份代码默默耕耘，而是需要建立自己的影响力的，代码审查过程中的交互，就是一个不可多得的方式。

## 识别出设计的缺陷，找到安全、性能、依赖和兼容性等测试不易发现的问题。 ##

代码审查在整个软件工程流程中还算早、中期，尽早发现问题就能够尽可能地减少修复问题的成本。而且，代码审查能够发现的问题，往往是其它途径不易发现的。因此，从这个角度来讲，代码审查要有方向性，比如主流程和某些重要用例，在审查的时候可以要求代码变更的程序员提供单元测试，或者是手工覆盖的测试结果，这样就可以认定这些分支覆盖到的逻辑是正确的，不需要在审查时额外关注。

## 设立团队质量标杆的最佳实践方式 ##

在我经历的团队中，基本上代码审查做得好的，代码质量都高。这不见得是程序员的能力特别出色，而是通过代码审查把这个质量的 bar 顶起来了。你可以想象，**一个对别人的代码颇为“挑剔”的人，他会对自己的代码截然相反地糊弄了事，睁一只眼闭一只眼吗？**特别对于刚踏入职场的程序员来说，这点尤为重要，要知道一个人刚工作的两三年，对性格、习惯这些关乎职业生涯因素的影响是巨大的，**一个好的标杆比任何口号都有效。**

# 代码审查工具 #

我在这里着重讲讲配置流程中非常重要的一步，就是配置机器审查和人工审查配合的工作流。代码审查是代码入库前质量保证的重要一环，所以通常是和 CI 工具配合使用。最好能够让机器自动化地完成关于代码风格、静态检查、单元测试等工作，这样可以让审查者把最宝贵的时间投入到逻辑、设计等难以自动化的问题上。这里，我和你分享一种最常见的工作流。

*  第一，将代码提交到本地 Git 仓库或者用于审查的远端 Git 服务器的分支上；
 *  第二，把 commit 提交给代码审查工具；
 *  第三，代码审查工具开始进行机器审查和人工审查；
 *  第四，如果审查通不过就打回重做，开发者修改后重新提交审查，直到审查通过后代码入库。

![img][]

至于具体的工具集，我这里给出两个例子：

*  第一个例子是 Facebook 采用的方式。他们使用 Phabricator 作为代码审查工具。同时直接使用原生的 Git Server 作为代码仓服务器。用户将改动提交到 Phabricator，然后进行机器和人工审查。检查通过后，Phabircator 负责将代码推送到 Git Server 上，或者用户手动将本地改动 Push 到 Git Server 上。这个工作流使用的是原生 Git Server 管理主仓，如果你要使用 GitLab 或者 GitHub 也可以。关于机器审查的配置，Phabricator 提供大量的钩子和插件机制，非常方便。具体细节你可以参考Phabricator 官方文档。
 *  第二个例子是使用 GitLab、Jenkins 和 SonarQube 进行配置。具体使用 GitLab 管理代码，代码入库后通过钩子触发 Jenkins，Jenkins 从 GitLab 获取代码，运行构建，并通过 Sonar 分析结果。这里有一篇不错的文章供你参考。

## 一些 Code Review 工具： ##

>  *  **Crucible：**Atlassian 内部代码审查工具；
>  *  **Gerrit：**Google 开源的 git 代码审查工具；
>  *  **GitHub：**程序员应该很熟悉了，上面的 "Pull Request" 在代码审查这里很好用；
>  *  **LGTM：**可用于 GitHub 和 Bitbucket 的 PR 代码安全漏洞和代码质量审查辅助工具；
>  *  **Phabricator：**Facebook 开源的 git/mercurial/svn 代码审查工具；
>  *  **PullRequest：**GitHub pull requests 代码审查辅助工具；
>  *  **Pull Reminders：**GitHub 上有 PR 需要你审核，该插件自动通过 Slack 提醒你；
>  *  **Reviewable：**基于 GitHub pull requests 的代码审查辅助工具；
>  *  **Sider：**GitHub 自动代码审查辅助工具；
>  *  **Upsource：**JetBrain 内部部署的 git/mercurial/perforce/svn 代码审查工具。
>  *

# 代码审查建议 #

## **建议一：小批量——每次 Review 的代码量要少** ##

研究发现， 成功的 CR 活动一定是小规模的。例如，《Modern Code Review：A Case at Google》论文介绍说， Google 的 CR 活动中，有 35% 的 CR 仅仅修改了一个文件，90% 的 CR 修改的文件数在 10 个文件以内，甚至有 10% 的 CR 仅仅修改了1行代码。

代码量少的好处显而易见：修改了哪里非常清晰，问题也会一目了然。一次推给别人 1000+ 行代码，还想得到有价值的 Review，可能性微乎其微。

当然，一次 Review 它代表的功能应该是有意义的，是完整的，如果不是修复缺陷，这一定程度上也对开发者迭代地开发功能的能力提出了要求。

## **建议二：多批次——Review 要频繁发生** ##

小批量必然导致了多批次。在微软 2013 年的一篇论文《iExpectations, Outcomes, and Challenges Of Modern Code Review》和前述的 Google 的论文中都提到了频繁 Review 的做法。其中，Google 的每周每 Developer 的代码变更中位数是 3 个，每周每 Reviewer 的 Review 中位数是 4 个。

## **建议三：找对人——合适的 Reviewer** ##

谁适合 Review 你的代码？选一个和被 Review 的代码毫不相干的人肯定是不明智的。下面列出了一些潜在的候选人：

如果你的组织有 Owner 机制，Owner 应该是合适人选

和你工作在相同上下文的同事

近期修改过相同代码的同事

比你更资深的程序员，希望得到他们的专业反馈

现在已经有一些工具，能够根据上下文推荐 Reviewer，这也为选择合适的 Reviewer 提供了便利。

## **建议四：快速响应** ##

当每次 Review 的粒度不大，Review 又比较频繁时，快速响应才能成为可能，也是必然的要求。在这个数据上，Google 的中位数是 4 小时。这个指标可以成为一个较好的参照。

## **建议五：使用现代工具** ##

快速响应、高质量的 Review 离不开现代工具。现代的 Review 工具能自动集成进工作流，高亮变化，甚至能自动汇总变更。这方面已经有许多现代的工具可以使用，还没有选好工具的读者，可以自行搜索。

## **建议六：考虑结对编程** ##

当我们提到 “小批量、多批次、快速反馈” 的时候，如果有过结对编程经验的同学，马上就会反映过来，结对编程和 CR 何其相似。

结对编程，共同编程的两位同学拥有完全相同的上下文，不存在上下文切换的烦恼，没有缺乏时间的烦恼，不需要借助额外的工具，反馈随时随地。事实上，在我的眼中，结对编程才是最好的 Code Review。

## **建议七：综合在线 Review 和线下 Review** ##

在线 Review 应该是常态化的行为。考虑到 CR 的 “知识传播” 价值，线下 Review 是有益的补充。有经验的团队，会周期或者不定期的组织线下 Review，这样能获得比在线 Review 更为广泛的知识传播面，也能引起更为热烈的讨论和辩论，有助于形成更高质量的共识。

## **建议八：**及时表达肯定，委婉表达意见。 ##

只针对代码，不针对人。这听起来很简单，都知道对事不对人的重要性，但是要非常小心不能违背。审查并不是只提反面意见的，**在遇到好的实现，不错的想法的时候，可以表示肯定**，当然这个数量不宜多，要不然适得其反。至于表达意见方面，我来举几个例子：

*  对于一些**次要问题**，我都会标注这个问题是一个 **picky 或者 nit 的问题**（“挑剔的问题”）。这样的好处在于，明确告知对方，我虽然提出了这个问题，但是它没有什么大不了的，如果你坚持不改，我也不打算说服你。或者说，**我对这个问题持有不同的看法，但是我也并不坚信我的提议更好**。
 *  使用**也许、或许、可能**、似乎这样表示不确定的语气词（英文中有时可以使用虚拟语气）。这样的好处是，缓和自己表达观点的语气。比如说：“**这个地方重构一下，去掉这个循环，也许会更好**。”
 *  **间接地表达否定**。比如说，你看到对方配置了周期为 60 秒，但是你觉得不对，但又不很确定，你可以这样说：“**我有一个疑问，为什么这里要使用 60 秒而不是其他值呢？**” 对方可能会反应过来这个值选取得不够恰当。你看，这个方式就是使用疑问，而非直接的否定，这就委婉得多。
 *  **放上例子、讨论的链接**，以及其它一些辅助材料证明自己的观点，但是不要直接表述观点，让对方来确认这个观点。比如说：“下面的讨论是关于这个逻辑的另一种实现方式，不知你觉得如何？”
 *  **先肯定，再否定。**这个我想很多人一直都在用，先摆事实诚恳地说一些同意和正面的话，然后用“不过”、“但是”和“然而”之类的将话锋一转，说出相反的情况，这样也就在言论中比较了优劣，意味着这是经过权衡得出的结论。

## **建议九：激励机制：激发主观能动性** ##

由于CodeReview本身跟人的经验或者意识都有很大关系，很多时候我们会为调动不起开发同学的积极性而烦恼，所以为了让大家更好的参与这个活动，我们一般都需要制定相应的激励机制。**也有人说了，如果是leader强制跟考核挂钩，那就不需要这个东东了**，嗯，但换个角度看，跟考核挂钩难道不是另外一种“激励”方式吗？

激励机制的设立有很多种，一般来说，都是在定期回顾的基础上根据CodeReview的实际情况对表现积极的同学进行一定的礼品奖励（选择什么礼品，要看组织的经济状况，哈哈）。

# 参考链接: #

[https://time.geekbang.org/column/article/137353][https_time.geekbang.org_column_article_137353]

[https://google.github.io/styleguide/javaguide.html][https_google.github.io_styleguide_javaguide.html]

[https://google.github.io/styleguide/][https_google.github.io_styleguide]

[https://github.com/google/eng-practices/blob/master/review/index.md][https_github.com_google_eng-practices_blob_master_review_index.md]

[img]: /images/20221020/d2632583b636480db95953e8ade8d693.png
[https_time.geekbang.org_column_article_137353]: https://time.geekbang.org/column/article/137353
[https_google.github.io_styleguide_javaguide.html]: https://google.github.io/styleguide/javaguide.html
[https_google.github.io_styleguide]: https://google.github.io/styleguide/
[https_github.com_google_eng-practices_blob_master_review_index.md]: https://github.com/google/eng-practices/blob/master/review/index.md