写点什么

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

作者:极狐GitLab
  • 2023-03-16
    江西
  • 本文字数:5945 字

    阅读完需:约 20 分钟

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

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


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



关注「极狐 GitLab」不迷路,一起 get Code Review 小技巧吧!


一、代码规范

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

1.1 不同公司有不同代码规范


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


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


  • 类名和括号应该在同一行;

  • 函数名应该用驼峰;

  • 缩进应该用空格,而不是 tab。


1.2 不同语言的规范各不相同


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



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

1.3 魔法数字难以维护


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


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


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



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


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

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


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



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


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


  • 风格规范规定缩进、换行、空格这些规范;

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


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


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


3. 怎么在团队里落地规范?


我们以 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 提供了一个更好的方式。

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



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



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


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

5. 极狐 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%,否则就不允许合并。

1. 强制测试覆盖率


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



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


2. 覆盖率下降拦截


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


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



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

3. 单元测试覆盖率报表


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


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


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



三、哪些需要人工审核?

1. Code Review:英文术语


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


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


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



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


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



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


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


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


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

2. 设计模式等技巧


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,获取更多干货知识,开启你的效能提升之旅!

发布于: 1 小时前阅读数: 6
用户头像

极狐GitLab

关注

开源开放,人人贡献 2021-05-19 加入

开放式一体化DevOps平台,助力行业高速协同增长!

评论

发布
暂无评论
你关切的Code Review三大问题,我以业务实践作答_DevOps_极狐GitLab_InfoQ写作社区