返回文章列表
开发 | 运维 | 安全 | 测试 | 医疗 2023-04-24

你关切的Code Review三大问题,我以业务实践作答

杨周
极狐(GitLab) 高级解决方案架构师

近日,在极狐 TechTalk 直播上,极狐(GitLab) 高级解决方案架构师杨周分享了高效 Code Review 秘籍以及 Code Review 在实际业务场景中的最佳实践,并手把手带大家基于极狐GitLab 进行了 Code Review。

 

以下内容整理自本次分享,干货爆满,分为上下两期:

 

上期回顾👉代码质量问题、分支创建与评审、Git 规范、安全漏洞

这期继续分析👉代码规范、Code Review 实践中大家最关注的几个问题。

 

代码规范

 

常见的代码规范问题有哪些?

 

不同公司有不同代码规范

 

很多公司制定了自己的代码规范,甚至是打印出来贴在墙上。Code Review 规范到底要不要去人工干预?

 

首先,我们看一下这段 Java 代码,这里有 3 个问题,Java 书写规范一般要求:

 

  • 类名和括号应该在同一行;
  • 函数名应该用驼峰;
  • 缩进应该用空格,而不是 tab。
     

 

不同语言的规范各不相同

 

我们再来看一个 Go 的代码。很多同学可能要抢答了,一眼看到这里的数据不对,底下俩空格上面是 tab?非常遗憾,这其实是对的。

 

 

大部分语言都是空格缩进,但 Go 官方要求用 tab……go fmt hello.go 会强制转换。这也是非常新颖的做法,就是语言官方自带规范,所以大家不要带着一个语言的习惯去评审另一个语言。

 

魔法数字难以维护

 

最后来看这个 PHP 代码有什么问题。这里有一个大括号换行问题,下面又有一个换行。

 

其实这些都是对的。PHP 很流行的规范是 PSR-2,它定义的是类后面大括号必须换行,和 Java 恰恰相反,但 if else 不换行。

 

大家要注意一下,不要搞得精神分裂了。

 

 

所以,这段代码超级换行没有任何问题,唯一的问题是魔法数字。if else 使用了 1 和 2 难以理解,这里应该用常量来表示。还有它的 if else 嵌套的太深了,复杂度太高,难以理解,难以维护。

 

代码规范有很多种问题,比如每个人的书写风格和习惯不同,人工去检查缩进换行浪费大量时间,更别说老项目,有很多的问题导致代码规范无法落地,这些都是业界常见的问题。

 

极狐GitLab 是怎么解决这些问题的?

 

首先,我们推荐大家采用业界知名的代码规范,而不要轻易自己造规范。因为除了写公司的代码之外,我们可能还会参加很多开源项目。开源项目一般都采用业界知名的规范,如果你自己造了一个跟大家都不一样的规范,做别的项目会比较麻烦。

 

 

上图是几家知名规范,比如 Java 有 Google 规范、PMD 规范,Go 语言有官方规范,PHP 有 PSR 规范。

 

这些规范主要分成两大类:风格规范和复杂度规范。

 

  • 风格规范规定缩进、换行、空格这些规范;
  • PMD 限制的是复杂度,复杂度不能大于10,这是默认值,如果团队目前复杂度比较高,可以自己去修改。另外,行数不能超过 60 行,否则太复杂了;类不能超过 1000 行,否则违反了面向对象的单一职责。

 

一般推荐大家先落地风格规范,等所有成员都上手了,再落地复杂度规范,再逐渐收紧,比如先调成 15 复杂度,逐渐收到 10 以下,甚至收到 5 以下。

 

业界知名代码规范都有一个特点,全部是有配套的命令行工具,而不是一个文档。有的大厂有自己的规范,发出 PDF 给大家看,这样是不合适的,而应该是一个配置文件加命令行工具,能让大家一键执行,才是一个合格的规范。比如 Java Checkstyle、 Code Sniffer、前端 eslint,连 Markdown 文档也是有规范的,比如阮一峰规范,用 Lint-md 来扫描。

 

 

怎么在团队里落地规范?

 

我们以 Java 为例来看一下,比如 check style,引入 Maven 插件,从 check style 官方开源项目里下载 xmail 放到指定目录里,执行 ./mvnw checkstyle:check,就会扫描出来有几个问题,在哪一行。

 

如果检查失败了,会有一个非 0 的退出码,对 Linux 比较熟的同学应该知道,退出码是非 0,意味着这是一个错误,我们就可以捕获它。

 

如果你不是用 Maven 构建,而是用更先进的 Gradle 构建,也类似引入 check style 插件,下载规范文件。

 

 

很多开发者会质疑:IDE 自带了代码规范,为什么还要下载代码规范放在 Git 里,还要配置命令行工具?

 

因为团队人多了之后,很难统一开发工具,有的人觉得 JetBrains IDEA 一年 1000+ 比较贵,用免费的 VSCode,结果各种编辑器默认的规范不同。而 Git 里的规范可以确保大家都可以指定同一个规范。

 

 

命令行工具也有大用处。

 

刚才说到有一个检查规范的命令,退出码非 0 的话,可以把它放在持续集成里面自动跑起来,这样就可以实现自动化的检查代码规范了。这也是持续集成的定义——通过在代码持续合并的过程中做一些自动化的工作,来实现质量内建。

 

 

下图是极狐GitLab 的持续集成配置,非常简洁,就这几行代码就可以跑起来了。

 

 

首先我声明了一个Java 17 的镜像,很多同学可能在用 OpenJDK, 一定要注意 OpenJDK 安全漏洞非常多,已经被 Java 官方废弃了,现在大家比较常用的是 Eclipse 镜像,这里就不展开说了。

 

接下来,执行刚才的 checkstyle 命令,然后在极狐GitLab 设置里勾选流水线必须成功。

 

 

因为有退出码,假如有任何不规范,就会导致整个流水线失败并终止,这样操作可以确保项目非常干净。如果流水线失败了,有一个叉号,自然就不能合并。

 

但我们并不清楚,流水线为什么失败了,是编译失败了,还是代码规范哪一行有问题?针对这个问题,极狐GitLab 提供了一个更好的方式。

 

Java 代码规范和极狐GitLab MR 打通

 

 

同样以 Java 规范为例,我们先引入 checkstyle 插件,下载代码规范,需要声明检查规范允许失败,失败后要把报告采集上来,并生成一个代码规范的报告,但它不是极狐GitLab 的格式,还需要通过 Maven  validate 对格式进行转换。

 

 

采集上来之后,在合并请求里就能看到多出来一个叫做“代码质量模块”的区域,这样开发同学一提交代码就能看到代码有规范问题,就可以自己修复。

 

上图可以看到历史遗留问题有几千个,大家暂停手头工作一起来修也不现实,理想的方式是做成增量代码规范,极狐GitLab 已经实现了增量代码规范报告。

 

极狐GitLab 增量代码质量报告

 

当我们第一次配置好规范后,在主分支或者 develop 公共分支上跑一次,产生了 8000 个问题,存起来,下次其他开发人员拉取临时分支的时候,又进行了一次全量扫描,可能也有 8000个问题,极狐GitLab 会对这两次问题自动进行 diff,从而获得一个增量代码质量报告,这个配置方式是最简单、最友好的。

 

 

增量报告的效果非常好,可以看出只有新写的代码带来了 12 个新问题,而几千个老问题不会再显示,当你改到某一行老代码,顺手修掉老问题,这里才会显示。这个开源工具能转换多种扫描报告,比如 PMD、Checkstyle。

 

 

总结一下,Code Review 时,完全不用操心代码规范和复杂度。极狐GitLab CI 结合业界热心开发者做的格式转换工具,就可以非常丝滑的打通,全自动的拦截代码规范。

 

Code Review 是否影响老代码?

 

随着项目越来越大,没人敢改老代码,有的人就把函数复制一份。这样虽然没有影响老代码,但是影响整个项目的可维护性,这是很多老项目必然面临的一个无解的问题。

 

另外,回归测试也越来越久,假如要改一个紧急的 bug,想上线也要回归,这么大的项目回归测试需要很久。那多花钱招一些测试人员行不行?

 

实际上是解决不了问题的,业界已经踩过这个坑了。因为 bug 多的根源是开发人员写的代码质量不高,而不是测试人员的问题。只有在开发阶段就确保代码质量,才能更好的规避这些问题,也就是进行自测。

 

 

很多开发同学都会用 Postman、Curl 或在浏览器里去测试,这样行不通的。因为你第一次写的时候,有耐心测了 4 次,比如左边代码有 4 种情况,测了 4 次,但第二次改的时候,可能就不想测这么多次了,或者别人改的时候也不会帮你测所有情况。于是,低成本、低质量必然的结局就产生了,这在国内非常普遍。

 

但如果严格要求任何人改一个字母、一个空格都要去测试所有情况,成本将高到难以接受。上图里的 4 种情况算少的,往上组合可能是 4×4×4 指数级的数量。

 

作为一个负责任的开发,有时间去操作 postman,不如去写一个脚本。把 postman 的参数都存在一个脚本里,自动去跑,这就是业界的终极方案,即自动化测试。

 

 

第一次写脚本虽然多花一点时间,以后再修改,成本就非常低了。不管改任何老代码,只要测试跑挂了,立马就能发现,实现了中等成本、高质量的方案。有人可能会问,中等成本到底有多少成本?根据国内某家大厂的调研,大概要多花 20%~40% 的开发成本。

 

技术管理的同学听到这里,可能还是觉得行不通,我们公司业务非常忙,如果再增加 40% 的开发成本,直接就要破产倒闭了。实际上不一定,公司总成本可能会下降。

 

有本书叫做《有效的单元测试》,里面做了一个统计,假设 A 公司不写单元测试,就像咱们国内大部分团队的情况:

 

  • 写代码花费 7 天,然后丢给测试同学;
  • 测试发现很多 bug,打回修改,又花了 12 天;
  • 26 天后终于上线了,客户还发现了 71 个 bug……

 

而另一种情况,B 公司写单测试,虽然写代码时间翻倍,花了 14 天,但提交之后基本没 bug,最后上线总时间 23 天,上线之后客户发现的 bug 还很少。

 

 

如果这是同一个行业的两家公司,就意味着 B 公司把钱砸在了开发人员上面,提升了开发成本,降低测试成本,最后公司的迭代速度、交付速度变快了,提高了公司的市场竞争力,而且客户满意度更高了;而 A 公司 bug 多,交付慢,竞争力越来越低。

 

有的开发同学从来没有学过单元测试,抱有一个偏见,觉得自己测自己的代码,肯定不仔细,还是丢给别人测比较好 ,其实这是个误解。

 

 

比如左边这个代码,有三种情况,右边只写了一种情况来验证它,不要紧,只要执行一下 mwnw verify,它就会告诉你:你的覆盖率测试覆盖率只有 33%。

 

写单元测试的核心指标就是覆盖率。那么,做到多少才算优秀?可以看下 GitLab 项目本身,后端 Ruby 做到了98%的单元测试,前端覆盖率做到了 76%。

 

 

因为后端都是数据和接口,非常便于做自动化测试,做到 90% 以上才算比较好。前端有的界面难以去自动化测试,可能需要一部分测试同学人工进行验收测试。

 

Google 是在 20 年前就开始这么做了,微软大概是在 2010 年左右,开始推行开发人员写单元测试,国内某家大厂在前两年开始要求开发人员写单元测试覆盖率达到 80%,否则就不允许合并。

 

强制测试覆盖率

 

如果覆盖率不到 80%,怎么去拦截?用 Java 插件 jacoco 就可以了。

 

 

比如这个 Java 项目,在 pom 插件里引入 Java 插件,里面写上 limit 0.8,就是最低 0.8 的覆盖率,然后去执行最下面执行命令,就可以算出来测试覆盖率。假设覆盖率只有 0.33,就会产生报错拦截。这个拦截配置在 CI 流水线里设置就可以了,非常简单。

 

 

覆盖率下降拦截

 

但是这样对老项目非常不友好,大量的老项目测试覆盖率是 0。如何让它逐渐提高,减少 bug 呢?极狐GitLab 有一个更好的方案:覆盖率下降拦截。

 

比如老项目的覆盖率 50% 多,而这次提交新写的代码又导致覆盖率下降了 2.85%,这个时候就会被拦截了。同时还会对测试报告进行标准化采集,与上面提到的代码质量报告类似。

 

 

合并请求的页面是开发同学最常使用的,极狐GitLab 把很多报告都做在这个页面里,让开发同学自助修复,不需要跳到任何别的地方,大幅度提高开发效率。

 

单元测试覆盖率报表

 

评估一家公司的技术水平,如果只看一个指标,就看单元测试覆盖率,如果看两个指标,则再看代码规范的错误率,即代码不规范的比例(代码规范错误率报表功能即将上线,敬请期待)。

 

极狐GitLab 会自动生成一个单元测试覆盖率报表,技术管理者可以看到公司所有项目的覆盖率排行榜。这样就知道各个团队的代码质量如何,bug 多不多。

 

通过报表里的走势图可以一目了然看到,项目覆盖率是不是在平滑上升。因此 Code Review 不需要去评判是否把老接口改挂了,假设改了一个地方,可能被几十个地方调用,人脑不是编译器,判断不出来,只有自动化测试才能够解决这个问题。

 

 

哪些需要人工审核?

 

Code Review:英文术语

 

前面提到的这么多问题都通过持续集成自动解决了,我们还需要人工审核吗?答案是要的。

 

比如 Java 项目, spring 项目里面提交了一个 SQL 文件,这段代码应该提交到代码库里进行数据库评审。那么当你新做了一个业务逻辑,建了一个表或者加了一个字段,怎么去评判数据库设计合不合理、文档放在哪里?要不要开个评审会?

 

直接把它提交到 Git 里,走代码评审就好了,这是非常成熟的一个流程。

 

 

如上图,用户表里有个字段:积分,怎么翻译?很多同学词典一搜积分叫 integral,就贴进数据库建表了,等来了个过了英语四级的新同学,发现这是数学积分,数学术语,跟用户积分完全没关系。

 

怎么样才能正确翻译?可以通过整句翻译,结合场景等方式更加准确,比如积分可以换礼物,才知道积分原来是 point,很多生活中的专用术语,很难通过机器直翻获取。

 

 

还有个例子,微信的「拍一拍」在英文版里怎么翻译?第一版不好,后面又改了个词,程序员就疯了,数据库字段和代码都写好了。跑了几个月又改掉了,那么代码里到底是跟着改还是不改呢?

 

所以英文术语设计是 Code Review 很重要的部分,关系到数据库字段设计,因为数据库字段往往会变成变量,变成接口,它一错后面全错了。

 

怎么降低术语审核难度?理论上术语应该由需求方提供,假设你是做一个航空行业的外包,这个航空行业有什么术语应该甲方来提供是比较合理。

 

如果不是外包,是自己做的产品,产品经理应该提供英文术语表吗?这个也不太现实,因为大部分产品只做中文版,产品经理只需要中文界面,没有想过英文术语。于是,代码中用什么英文术语,大部分时候都是开发人员自由发挥,一不小心就导致质量的灾难。

 

设计模式等技巧

 

Code Review 还要 review 一些业务逻辑、设计模式,即面向对象的最佳实践,有一些编程有更优雅的写法,这个是高级工程师指导初级工程师成长的很重要的一部分。

 

比如下图左边这段 Java 代码,它的缩进换行都很漂亮,唯一的问题就是看起来这些函数都太简单了,get ID、get name、set ID、set name…

 

 

这么简单的代码,全部重复的写,就是体力活,所以业界用 lombok 解决这个问题:定一个 @Setter、@Getter,就可以把这些函数全部省略掉了。

 

总结

 

最后我们总结一下,一个完善的持续集成流水线应该是怎样的?它包括:

  • 代码规范的质量门禁;
  • 强制的单元测试;
  • 编译打包;
  • 安全扫描。

 

有了极狐GitLab 这么一个自动落地规范的流程,把研发流程理顺,Code Review 就省心了。

 

  • 极狐GitLab 在持续集成里做了 Git 各种规范的拦截;
  • 在极狐GitLab git server 端,做了 Git 规范拦截,包括 commit 规范、分支规范、分支命名规范;
  • 在持续集成里做了运行环境的拦截,包括代码扫描、漏洞扫描。

 

尽量让这些工作自动化,这样高级工程师评审的工作量就大大降低了,初级工程师能够迅速成长,企业可以通过校招实现人才梯队建设,实现降本增效。

 

🌟 本期极狐 TechTalk《高效 Code Review 秘籍以及Code Review 在实际业务场景中的最佳实践》分享到此完结,希望对你有所启发和帮助!😊

 

极狐GitLab 一体化DevOps平台 专为中国用户研发,免费试用60天专业版高级功能
售前咨询
联系电话
在线支持
预约演示