在业务中台，如何做代码评审
The following article is from 有赞coder Author 有赞技术
部门：业务中台/测试开发
1、业务背景
业务方应用接入BOS需要依赖于bos-sdk，应用集群在启动时通过bos-sdk将应用指定注解的组件进行收集，收集完成后保存在DB中，集群中的每一台机器在重启时，需要保证入库时只有一条请求的处理能够正确入库，以保证数据不会重复入库以及数据插入冲突的情况，为防止出现上述情况，项目中采用分布式锁，对此我们针对项目中分布式锁的逻辑，以及业务拿到锁的实现进行了CR，CR的最佳指导我们采用结构化方式进行，分别从背景了解、业务场景、逻辑分析、异常分析、编程规范、非功能分析、可测性分析这几个唯度进行CR。
2、我们先看一下收集流程
3、分布锁的实现
3.1 锁实现逻辑：
3.2 锁实现时序调用：
3.4 我们针对上述锁的实现开始CR：
a 背景了解：
要了解背景，仍需要从代码中的注释作为入口：
第2行中文字描述“竞争获取分布式锁” 可能让人有点困惑，什么样的场景需要竞争锁，没有描述清楚：
第4行interval的注释“锁住时间”是不是改为锁的失效时间， 更能让人理解：
b 锁业务场景分析：
场景分析一：线程永远拿不到锁：代码第10行，如果发生异常，则返false,这样导致线程获取不到锁；
场景分析二：线程1 获取锁后，业务逻辑未处理完，锁失效，线程2可获取锁，引起数据入库(dumplicate)冲
c 逻辑分析：
场景遗漏：
try ....catch 中的 try对应的逻辑发生异常，catch则会返回false，线程就会一直拿不到锁；
逻辑冗余：
第1416行的逻辑是根据lastReqTime是不为空或长度不为0，直接返回true，往上追踪lastReqTime的值是第10行赋值，假设缓存readis不存在岩机可服务不可用情况，也不考虑网络抖动情况，在对key赋值是永运是成功的，即永远有值，则第1416的条件逻辑是多余的逻辑，即使出现岩机，网络抖动情况则对key的赋值会失败，走catch后，直接会返回false；
d 非功能点分析
可测性分析：
无发现问题(可以单测，将StringRedisTemplate 写成档板)
可监控：
第28行，可以按关键字进行监控告警，但根据输出的ERROR日志对定位问题可能会有误导，
4、业务获锁后处理逻辑
4.1 业务代码逻辑处理:
4.2 业务获取锁业务处理流程:
4.3 问题分析：
a 背景了解(注释中相关业务场景信息缺失)：
业务执行逻辑根据是否获取锁，走不同的逻辑处理，在注释中是了解不到的，此时只能通过业务的上下文去理解拿锁后的逻辑，注释欠缺；
b 逻辑分析
逻辑处理不合理：
第31~36行
try语句块的逻辑在此场景核心是关注DB操作的，不应在try语句块中加入其它逻辑调用，换句话理解，如果DB操作成功，第34行调用失败或调用异常，则会走catch，与try中关心场景本意不符。
第34行服务调用是一个空的实现类，里面没有任何逻辑，此处放在这里，用意不明确，建议删除
c 代码写法规范：
第13行，return Boolean.TRUE 这种写法是考虑性能上的差异吗？return true,return Boolean.TRUE，如无区别，建议统一风格；
第37行，catch中的异常拋出，建议加上error日志的打印，具备监控预警能力；
注释不规范 第19行，锁sleep 3 秒，与第21代码实现冲突；
Thread.sleep(4000)为什么是4S，而不是5000s或其它的数值，根据什么依据设置的，没有进行说明；
日志级别打印级别不规范：第33行，log.info("MindSaveConsumer-process saveOrUpdateMindAggr {}", saveSuccess ? "success" : " fail"); 这行是根据DB操作后，返回不同结果的日志打印，不管DB操作失败还是成功，打印的日志级别都是info，理论上讲对于DB操作异常的日志都应打error级别；
第26、27行，log.info("MindSaveConsumer-process locked version is {}",key)<26行>；throw new AbilityException(ErrorTypeEnum.LOCK_FAIL)<27号>；
结合27行给拋出的异常，建议第26行打印日志级别设置error或waring更合理，或者说线程本身在获取锁时因锁被占正常的行为，则27行没必要拋出异常，直接打印出info或waring更合理一点；且27行拋出异常描述为“加锁失败”，这个描述让人误解，建议更改为锁被占，获取锁失败，更适合此场景的异常描述；
异常分析：
线程永远拿不到锁：锁实现的10行代码出现异常，则返回false，此时后续请求 进来会永远拿不到锁；
拿锁的线程执行逻辑时间较长超过15s失效释放锁，则线程2拿锁之后，在进行DB操作时会产生dumplicate的冲突
d 非功能分析
可监控：
拿锁之后的DB操作不具备告警，及监控的能力；
24行，37行的异常日志无
可测性：
针对业务实现可以开启后门(http接口）以方便锁的正常功能逻辑校验
PS:在此项目里，如果要测试业务拿锁的后的场景漏洞验证，及功能验证一是：摸拟并发请求，验证锁的有效性；
性能无
性能层面相关的问题暂无；
PS:此处用的是分布式锁，在常用的场景下，其性能相对于其他锁的实现相对较高，但同时增加的代码设计的复杂性；
总结：
经过结构化CR，我们可以从背景了解、业务场景、逻辑分析、异常分析、编程规范、非功能分析、可测性这几个唯度发现代码在实现过程中的问题，当然上述代码中不论是锁自身实现，还是业务拿到锁之后的实现结合具体的业务场景可能还有一些隐藏的问题待挖掘，但通过结构化的CR方式 ，我们可以提前将一些显见的问题类型提前识别出来，防止这些问题击穿不同阶段的测试，引发线上问题，关于业务中台BOS的介绍，可参考有赞coder公众号中相关文章"千亿级公司低代码平台的测试体系介绍"。
往期推荐：
技术琐话
以分布式设计、架构、体系思想为基础，兼论研发相关的点点滴滴，不限于代码、质量体系和研发管理。