【如何有效做Code Review】8行代码提出的21个问题-阿里云开发者社区

开发者社区> 开发与运维> 正文

【如何有效做Code Review】8行代码提出的21个问题

简介: - 很多同学都有这个疑问,如何结构化体系化的做CR?如何综合应用各种手段尽快及早的发现代码问题和缺陷? - 下面围绕这个实例,抛砖引玉,大家可以一起探讨;  - 实例如下 ,短短8行代码,通过CR可以发现多少问题呢?21处;这段代码谁写的不重要,探讨的重点是如何全面发现其中的问题和隐患;  

8行代码的21问题

1. 如何有效的做CR?

很多同学都有这个疑问,如何结构化体系化的做CR?如何综合应用各种手段尽快及早的发现代码问题和缺陷?
下面围绕这个实例,抛砖引玉,大家可以一起探讨;
 

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

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

code.png


 

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中
 
dist.png

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

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时都能从中找一个切入点,根据经验和已有规则,能提出自己问题。
 
stru.png

 

2.3 CR常见规则

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

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 问题背后的问题

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

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

正确的结果 不正确结果
我知道我知道
已知正确前提
这个是期望

经验总结
- 复制经验规则方法
- 经验沉淀到技术
原因:人的意识,人的失误

怎么解决?
- 自省和练习
- 他查
- 技术发现
我知道我不知道
已知错误前提
侥幸 原因:缺乏专业精神

怎么解决?
- 自身尝试和经验积累
- 他人经验规则方法
- 技术发现问题
- 数据度量
我不知道我不知道
未知前提
运气不错
怎么解决?
- 发现未知(AI和数据)

 
 
究竟怎么防范和发现问题呢?人员经验可以积累,方法可以沉淀和传授和基础设施(技术,数据,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来说同样重要:

  • 设计方法

    • 不少假设和前提需要确定,同时需要数据和工具来支持,在正确前提下做设计,防范
    • 设计和实现方式,很多问题都有共性,或有存在的解,开发者和Reviewer都需要持续积累
    • 参考标准集团开发规约和业界最佳实践
  • 持续质量运营

    • 持续补充代码规范和Best Practice到扫描工具中并分享
    • 经典或常见问题了解,如国际daily quiz 和daily bug

 

3.4 基础设施(技术解法)

tech.png

版权声明:本文内容由阿里云实名注册用户自发贡献,版权归原作者所有,阿里云开发者社区不拥有其著作权,亦不承担相应法律责任。具体规则请查看《阿里云开发者社区用户服务协议》和《阿里云开发者社区知识产权保护指引》。如果您发现本社区中有涉嫌抄袭的内容,填写侵权投诉表单进行举报,一经查实,本社区将立刻删除涉嫌侵权内容。

分享:
开发与运维
使用钉钉扫一扫加入圈子
+ 订阅

集结各类场景实战经验,助你开发运维畅行无忧

其他文章