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

8行代码的21问题

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 代码逻辑

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

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

2.1.3 代码规范

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

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

2.1.4 业务逻辑

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

• Q4: Line98，dbmode {String} dbmode： [dev, sit, stable, gray, prod,...]为啥只考虑dev和test?
• 这个是常见错误，忽视了客观数据；很多异常case都是由此产生的

2.1.5 设计合理

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

• 这个是设计问题（性能考量），string的replaceFirst调用正则，一次compile没问题，也逻辑很清楚
• 如果大量请求，正则重复compile，则有不必要重复耗时计算，这里主要识别replaceFirst的实现机制，根据上下文来选择

2.1.6 非功能

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

• 大家能看到有很多防御性代码，避免了线上的报错，是大家普遍接受的。过多防御代码，让系统逐步减少对异常感知能力，从而隐藏了真正的问题。
• 另外防御性代码对今后的维护性造成了隐患，不清楚是否能改造。

2.1.7 可测性

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

• 这个是可测性的问题，测试同学在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和数据）

究竟怎么防范和发现问题呢？人员经验可以积累，方法可以沉淀和传授和基础设施（技术，数据，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 基础设施（技术解法）