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 代码逻辑
不管是否熟悉业务，都能发现逻辑问题：
- 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 代码规范
2.3.3 业务逻辑
- 功能实现是否完整性
- 是否遗漏关联场景
- 异常情形处理
- 对依赖方实现，架构和规划有了解
2.3.4 设计合理性
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 方法
3.4 基础设施（技术解法）
