基于谷歌代码审查(Code Review)法则的思考与实践

背景

代码审查(Code Review),就是让别人来审查你的代码,其目的就是确保代码库的整体代码运行状况随着时间推移而不断改善。

中国有句古话:三人行必有我师。

代码审查同样如此:

  • 他人的审查或许会有不一样的思考和建议;
  • 人都会犯错,多一个人检查就减少犯错的机率。

因此代码审查是你编写的代码在合并到主分支前最重要的一项检查工作,也是一项最直接、最低成本的发现软件中的错误绝佳方式。

既然代码审查这么重要,而且有这样显而易见的收益,但总能听到代码审查在团队里执行起来不容易、效果不理想的问题。问题出在哪呢?

据我观察有两点原因:

第一,读别人代码需要花时间,往往还需要代码提交者带着业务为审查者讲一遍,同时占用双方时间;
其次,如果代码审查者工作繁重、压力大而没有时间,也很容易造成执行不到位,走过场;

如何才能比较好的开展代码审查?让我们先来看看大公司是怎么做的,Google 的这篇关于代码审查的文章里给出了具体法则。

Google 的代码审查法则

在进行代码审查时,应确保:

  • 代码经过精心设计
  • 该功能对代码用户很有帮助
  • 任何 UI 更改都是明智的,并且看起来不错
  • 任何并行编程都是安全完成的
  • 代码没有比需要的复杂
  • 开发人员没有实现他们将来可能需要的东西
  • 代码具有适当的单元测试
  • 测试经过精心设计
  • 开发人员对所有内容使用了清晰的名称
  • 注释清晰实用,并且主要说明Why而不是What
  • 代码已正确文档化
  • 该代码符合我们的样式指南

确保检查要求你检查的每一行代码,查看上下文,确保你在改善代码运行状况,并称赞开发人员所做的出色工作。

原文:https://google.github.io/eng-practices/review/reviewer/looking-for.html

代码审查法的落地

可见,想要更好的落地代码审查,需要先要确立法则,你可以根据实际情况对上述法则进行借鉴、删减或补充;

第二,作为技术领导应当积极布道,让开发者了解统一的代码审查法则;

第三,应当把法则里的具体规则尽可能地通过流程控制、自动化检查则纳入到 Pull Request 中。

另外提醒作为 Reviewer 要 Peaceful !!!在代码审查时注意不要带有“教育”性质的去给别人提出修改建议,那样很容易适得其反。

以下是一些不完全实践,供参考。

流程控制

规避任何不经 Review 的代码进入到主分支

以 Bitucket 为例。GitHub,GitLab 在设置上大同小异。

  • 打开分支权限设置里的选项 Prevent changes without a pull request 打开它。当然如果有需要可以在这个选项里添加 Exception,被添加的人可以不通过 Pull Reuqest 来提交代码。

  • 在 Merge Check 里开启 Minimum approvals 这个选项。比如设置 Number of approvals = 1,这样需要至少有一个 Reviewers 点击 Approve 按钮才允许 Merge。

自动化检查

通过CI流水线验证编译和测试

  • 建立自动化构建和测试 Pipeline,这样在创建 Pull Request 的时候可以自动构建、测试以及检查。Jenkins 的 Multi-branch pipeline 可以满足这个需求。

  • 开启 Bitucket 的 Merge Check 里 Minimum successful builds 选项,验证构建/测试结果,以防止任何没有通过构建和测试的代码可以 Merge 到主分支。

  • 另外,可以通过自行编写工具来实现,或可以集成其他 CI 工具来做检查,例如:

    • 针对 Pull Request 的修改历史来分析提交历史并推荐 Reiewer;
    • 通过 Lint 工具来检查编码规范;
    • 通过 REST API 检查是否需要压缩 Commits 来保证清晰的提交历史;
    • 通过 SonarQube 检查 Quality Gate 等。

实现自动化检查,可以帮助 Reviewers 将审查的工作精力放在代码的具体实现上,其他的交给工具。

最后

代码审查做的好不好,跟一个团队有没有良好的技术氛围,或者是否存在有技术领导力,有“品位”的技术大牛也是正相关的。

  • 如果团队里大多数都是有“品位”的工程师,他们会以写出优秀的代码(或挑刺)乐此不疲。
  • 相反如果团队不重视规范,只追求短期的绩效达成,只会让技术债越欠越多,产品越做越烂。

欢迎留言分享你的意见或建议。