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 日 阅读数: 59
用户头像

zzj8704

关注

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

还未添加个人简介

评论

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