写点什么

8 行代码的 21 问题: 如何有效 Code Review?

用户头像
zzj8704
关注
发布于: 2020 年 06 月 22 日

1. 如何有效的做 CR?

很多同学都有这个疑问,如何结构化体系化的做 CR?如何综合应用各种手段尽快及早的发现代码问题和缺陷?

下面围绕这个实例,抛砖引玉,大家可以一起探讨;

1.1 CR 实例:8 行代码 21 问题

实例如下 ,短短 8 行代码,通过 CR 可以发现多少问题呢?21 处;这段代码谁写的不重要,探讨的重点是如何全面发现其中的问题和隐患;


1.2 CR 实例:如何结构化的 CR?

对这段代码,从背景了解,逻辑分析,异常分析,编码规范,非功能点,可测性来分析代码问题:

1.2.1. 背景了解

思考问题:代码块是要干什么?从上下文,方法签名和代码结构上快速看下代码:

Q1: 注释中,Line94,嗲吗出现音近拼写错误

Q2: 注释中,Line93(非 dev),Line94(生产),Line98(非 dev,非 test)不是很统一,应该是生产环境要保障是 https

Q3: 注释中,Line94,为啥生产中拿不到正确的 https, 原因是什么,什么情况触发,多大概率

生产环境可能拿不到正确的 schema, 是个异常的 CASE 处理

其中注释方法命名,出入参数没清晰表达场景


1.2.2. 逻辑分析

思考问题:代码究竟干了什么?选自己熟悉或则感兴趣的点开始:

  • Q4: Line98,dbmode {String} dbmode: [dev test, stable, prod,...]为啥只考虑 dev 和 test?

  • Q5: Line98,代码中是逆向判断,不符合条件(线上)会有很多,为啥不直接过滤 PROD, if(PROD) 比较好理解,简单;同时 case 数量也减少很多

  • Q6: Line100,http 替换成 https,但是中间端口没有替换,是否存在此端口未开通 https 的情况代码做了什么清楚多了

1.2.3. 异常分析

思考问题:代码干不了什么?真实的环境复杂,数据多样,看看会那些情况:

  • Q7: Line98,dev,test 都是小写,是否大写就不能匹配, 要是有枚举的可以统一处理

  • Q8: Line98,dbMode 没做判空,如果 dbMode 为空,则会认为是生产环境

  • Q9: Line99,httpGetRequest.getUri().xxx()可能报空指针异常

  • Q10: Line99,uri.startsWith("http://") 需要考虑前面有空格的情况

  • Q11: Line99,大写的 HTTP://是否也需要可也要匹配处理?现实中会出现这么多异常么,代码执行环境,输入最终是什么样的?

1.2.4. 编程规范分析

思考问题:代码怎么干最标准?各种情况逻辑情况有了,再看下能否最佳姿势表达:

  • Q12: Line98,记得判断环境,有直接的函数的,不用自己写? 之前主站有对这块治理过,是否可重用

  • Q13: Line98, equals 方法比对对象写在左边

  • Q14: Line98,if (!(StringUtil.equals(dbMode, "dev") || StringUtil.equals(dbMode, "test"))) if 中包含比较复杂判断,应该用变量替换例如:isSitEnv

  • Q15: Line99, httpGetRequest.getUri()调用了三次,存储到变量中可以少调用两次

集团和蚂蚁都有编程规范,代码提交前可以自行扫描一下,部分代码门禁也有接入

代码中多处字面量(dev, test, http, https),需要常量化,dbmode 适合枚举化

1.2.5. 非功能点分析

思考问题:代码有什么影响?跳出来再思考,代码虽小,也需要看下运行中链路,检查性能,稳定性等点:

  • Q16: Line100, replaceFirst("^http", "https"); 使用正则,匹配比简单字符操作要耗时

  • Q17: Line100, replaceFirst 中,每次都会 new*complie Pattern,大量命中存在不必要的重复 compile pattern 时间

  • Q18: Line100, 从 comment 中看出这个一个异常情况,线上如果触发了,需要一种方式能感知,例如增加打印日志并需要配置监控,目前没有日志无法感知,也无法调查到 root cause 发现功能之外情况,同时需要考虑三板斧

1.2.6. 可测试行分析

思考问题:代码怎么覆盖又快又好?

  • Q19: Line100, 由于限定了环境,在线下环境没发功能测试进行覆盖,最好单测覆盖最简单

  • Q20: Line100, 线上触发时也无日志打印,不具备可观察性;

  • Q21: Line100, 如果不加日志,在验收或则开量时进行 double check 出入参的正确性(或验收用例)

2. 抽象 1 步,如何复制和复用?

这里共 21 个问题,其实每个问题都能对应到快速 check list 中



可以看出:异常情形,编程规范,功能逻辑和注释这块问题比较多,非功能性和可测性问题也存在

2.1 问题分类

2.1.1 需求和上下文

每个 Reviewer 都会关注的地方,好的注释,代码结构很容易人理解:

  • Q3: Line94,为啥生产中拿不到正确的 https, 原因是什么,什么情况,多大概率

  • 这个是需求,是提前,为啥要做这个事情。在 CR 时需要需要了解,特别是对口的开发测试同学,否则发现不了基本的问题

2.1.2 代码逻辑

不管是否熟悉业务,都能发现逻辑问题:

  • Q8: Line98,dbMode 没做判空,如果 dbMode 为空,则会认为是生产环境

  • 这个是异常数据,是否真的存在,存在了有什么影响

2.1.3 代码规范

这种问题可能太多了,经验和方法也有很多。Follow 最佳实践和惯例,可以提高可读性和避免采坑:

  • Q14: Line98,if (!(StringUtil.equals(dbMode, "dev") || StringUtil.equals(dbMode, "test"))) if 中包含比较复杂判断,应该用变量替换例如:isSitEnv

  • 这个是编程规范问题,字面值常量化(枚举化),复杂判断,代码可读性,

2.1.4 业务逻辑

业务背景知识对实现的完整性至关重要:

  • Q4: Line98,dbmode {String} dbmode: [dev, sit, stable, gray, prod,...]为啥只考虑 dev 和 test?

  • 这个是常见错误,忽视了客观数据;很多异常 case 都是由此产生的

2.1.5 设计合理

合理的设计,要结合上线文来看,会有对比和取舍:

  • Q17: Line100, replaceFirst 中,每次都会 new*complie Pattern,大量命中存在不必要的重复 compile pattern 时间

  • 这个是设计问题(性能考量),string 的 replaceFirst 调用正则,一次 compile 没问题,也逻辑很清楚

  • 如果大量请求,正则重复 compile,则有不必要重复耗时计算,这里主要识别 replaceFirst 的实现机制,根据上下文来选择

2.1.6 非功能

需要跳出功能点来看,从静态的代码,看到质量的更多面:

  • Q18: Line100, 从 comment 中看出这个一个异常情况,线上如果触发了,需要一种方式能感知,例如增加打印日志并需要配置监控,目前没有日志无法感知,也无法调查到 root cause

  • 大家能看到有很多防御性代码,避免了线上的报错,是大家普遍接受的。过多防御代码,让系统逐步减少对异常感知能力,从而隐藏了真正的问题。

  • 另外防御性代码对今后的维护性造成了隐患,不清楚是否能改造。

2.1.7 可测性

可测性是测试同学,也是开发需要考虑的点:

  • Q19: Line100, 由于限定了环境,在线下环境没发功能测试进行覆盖,最好单测覆盖最简单

  • 这个是可测性的问题,测试同学在 CR 时需要考虑这块,考虑通过什么手段来覆盖;很多测试不一定要从集成后进行功能测试;CR 和单测是很好的方法。

2.2 结构化 CR

上述发现的问题和我们 CR 的方式很有关系,每个人进入 CR 的时机不一样,关注点也不一样。下面是从 CR 的简单流程,每个人 CR 时都能从中找一个切入点,根据经验和已有规则,能提出自己问题。



2.3 CR 常见规则

下图是一个 Expert Code Reviewer 的想法,正常大家在 Code Review 可能不会考虑这么多和全面,哪些可以帮助快速有效 CR 发现问题。根据之前出现的问题,下面列出最常见 check 点,帮助大家有效 CR。



2.3.1 可读性

  • 评论是否清晰有用?变量、类、方法的命名是否明确,可辨识

  • 代码可读性,将来其他同学能轻松理解并使用此代码

  • 阿里巴巴集团 JAVA 代码规范-基础规约可读性部分

这些规范可由门禁代码扫描[静态扫描规则库]发现部分,可适当关注。

  • 注释规约

  • 常量定义

  • 格式规约

  • 命名规约

2.3.2 代码逻辑

  • 代码是否实现 DEV 的意图

  • 边界问题,如数组越界,获取 object key,业务逻辑的边界(没有的情况、全等、全 null、全 undefined 等)

  • 资源泄露(内存泄露,文件句柄未释放,网络端口占用等)

  • 算术计算溢出(赋值截断,精度提升,移位,类型转化等)

  • 线程 &并发处理(ThreadLocal 退出清理,一锁二判三更新四释放等)

  • 异常数据处理(数据约定,长度,类型,默认值等)

  • 工具使用姿势是否正确(了解工具背后逻辑)

  • 工具常见问题,如 map.put key=null 报错,switch(param=null)报错

2.3.2 代码规范

  • 阿里巴巴集团 JAVA 代码规范-基础规约

  • 控制语句

  • OOP 规约

  • 集合处理

  • 并发处理

  • 其他平台语言开发参考:

  • 集团 iOS 开发规约

  • 集团 Android 开发规约

  • 集团前端开发规约

  • 集团 C++规约

是否 follow 业界 Best Practice Oracel Java Language Best Practices

2.3.3 业务逻辑

  • 功能实现是否完整性

  • 是否遗漏关联场景

  • 异常情形处理

  • 对依赖方实现,架构和规划有了解

2.3.4 设计合理性

  • 明确真实的业务需求和场景,真实环境中怎么使用的

  • 是否基于未确定的假设

  • 是否重复造轮子,有没有漏洞和留坑,有限制

  • 是否和架构,系分匹配,能否满足业务需求

  • 是否 follow 常见设计规约,如阿里巴巴设计规约, 蚂蚁金服 DB 设计规范

  • 异常日志

  • MySQL 规约

  • OceanBase

  • 蚂蚁中间件

  • 蚂蚁二方库

  • 蚂蚁安全

  • 服务器规约

  • 国际化设计 follow 国际际化规约,其实个人认为所有应用默认就应 follow

  • 错误码设计

  • 常见性能(是否有更优操作方法,是否有重复耗时任务, 在大量请求下是否触发性能问题)

  • 防错设计

2.3.5 非功能

常见的点,但不限于

1. 稳定性,上下游链路稳定性

  • 兼容性(代码兼容性, 数据兼容性,协议兼容性)

  • 系统依赖(高等级稳定依赖低等级系统)

  • 数据库和缓存(SQL 性能,是否能走到索引,缓存多入口刷新,失败补偿,缓存 &持久化不一致)

  • 流量和性能(服务重试+长耗时,下游服务负载过重,自身限流)

  • 资金安全(前端不进行金额计算,幂等字段,并发流水更新)

  • 服务隔离(是否一个失败是否会触发整体失败)

2. 安全性

  • 垂直权限和水平权限

  • 数据脱敏和防泄漏(不限于日志,前端返回)

  • 用户请求有效校验

  • 客户端安全请参考:

  • 蚂蚁金服 h5 安全开发规范

  • 蚂蚁金服 android 应用安全开发规范

  • 蚂蚁金服 IOS 应用安全开发规范

3. 可诊断性

  • 关键路径是否有日志

  • 日志信息是否可排查,能否各端串联起来,如:线程加上名称等,异常时打印信息是否可用于复原场景

4. 可监控,可灰度,可应急

  • 关键业务有日志埋点用于监控,并发现问题

  • 该功能可灰度方式和策略

  • 数据变更是否可灰度,怎么灰度的

  • 是否需要和有应急能力

5. 可测性

  • 是否可测, 是否满足可观察性,可操作性,可自动化等

  • 什么方式覆盖和难点,CR 覆盖,单测覆盖,功能,还是专项,是否需要额外可测性改造

  • 单元测试规约,很多点对功能自动化也使用

3 再抽象 1 步,如何系统性预防和发现暴露出的问题?

3.1 问题背后的问题

为什么会有这么问题呢?问题多属于注释,异常情形,编程规范,功能逻辑;下面具体的原因分析:



这里有人的经验,有方法,复杂环境和基础设施原因,都可以落到这个九宫格中。



究竟怎么防范和发现问题呢?人员经验可以积累,方法可以沉淀和传授和基础设施(技术,数据,AI)可以逐步建设,下面说下自己的想法:

3.2 人员

  • 提升质量意识,全面认识质量;

  • 如代码复杂度高,可读性,测试成本也高;发现治理根因比问题防范更重要等

  • 主动通过持续技术运营,技术分享等来弥补技术不足和积累经验

  • 在持续 Code Review 中学习,类似的问题就会越少

  • 防错设计意识

建议参考The Clean Coder: A Code of Conduct for Professional Programmers (Robert C. Martin Series)


3.3 方法

  • 养成良好 CR 方式,提交 CR 时 double check 一次加强自检,对 Author 和 Reviewer 来说同样重要:

  • 具体可参考Google的CR最佳实践英文版)可以组织 CR 学习

  • 设计方法

  • 不少假设和前提需要确定,同时需要数据和工具来支持,在正确前提下做设计,防范

  • 设计和实现方式,很多问题都有共性,或有存在的解,开发者和 Reviewer 都需要持续积累

  • 参考标准集团开发规约和业界最佳实践

  • 持续质量运营

  • 持续补充代码规范和 Best Practice 到扫描工具中并分享

  • 经典或常见问题了解,如国际 daily quiz 和 daily bug

3.4 基础设施(技术解法)



发布于: 2020 年 06 月 22 日阅读数: 333
用户头像

zzj8704

关注

每行代码都有它的使命 2018.05.24 加入

还未添加个人简介

评论

发布
暂无评论
8行代码的21问题: 如何有效Code Review?