JAVA代码Review总结

1. 代码Review的目的

一般团队很难在Review中要求极致的代码质量，毕竟是项目而非艺术品。你很难通过Review要求程序进化为完美的程序，很难要求程序员都达到很高的水准，所以常见团队的Review更多的是达成一定的共识，底线是质量可以做到项目要求的高度。而更优秀代码的探讨可以作为锦上添花的讨论，根据情况推荐而不强求。

本文尝试总结下自己评审中遇到的通用问题，项目或业务名称通过简称来替代。

2. 重点Review的点

• 架构是否合理：虽然是在Review代码，其实很多时候我们都是在评判架构是否合理。 比如数据的同步机制是否合理等等
• 跨网络请求：跨网络请求一般是性能占比最高的地方，是否在循环中调用。
• 异常处理：结合业务查看异常处理流程是否正确合理，异常处理是出错的重灾区，需要重点关注。
• 监控日志：程序执行重点痕迹是否得到了妥善记录，辅助排查问题
• 算法效率：算法是否过于复杂

3. 评审中遇到的汇总

• 业务代码中耦合算法：比如「YHQ」系统的代码Review，命中黑名单和业务处理写在了一起，应该抽取黑名单模块；进而黑名单模块可以做成项目无关，整个公司通用。

• 代码实现不符合真实业务：比如「MF」系统的Worker代码review，把不能处理任务的状态直接设置为删除有风险，应该使用中间态暂存处理不了的任务；进一步说这些任务处理是通用的，业务无关的，应该使用通用的任务处理平台。

• 接口返回的出参过大：「HB」系统评审中接口返回使用了内部对象，应建立新对象，返回最小化的外部对象。

• 返回对象中重复字段网络开销：「YHQ」评审中列表对象打平，会带来重复字段，如果改动参数类型，影响上下游，需要平衡。

• 时间区间与一个时间单位一个数据的取舍：「MF」时间维度控制上限的数量需求评审中，结合实际诉求，时间区间灵活，但处理交叉比较复杂；真实需求单个时间设置数量更方便，直接使用每天的上限库存来实现。

• Redis的key拼错：很多评审多次出现，统一key的拼接方法或提供jar包。

• 不同机房分组要做同样的事情：「XLH」评审每个机房的服务器有状态，有固定的服务客户，必须配置对应的Redis。应该把服务做成无状态的，配置统一存在Redis中，不同分组可以互相替换，都可以服务所有客户。

4. 专项说明

4.1 卫语句使用

public Set<String> getWhiteSetByXXX(String businessXXX) { if (StockUtils.isNotEmptyMap(whiteListMap)) { if (whiteListMap.containsKey(businessXXX)) { return whiteListMap.get(businessXXX); } else { return new HashSet<String>(); } } else { return new HashSet<String>(); } }

评注：卫语句很适用空条件判断，顾名思义，卫语句很像门卫，是把不应该进门的坏人(异常场景)拦住。而IF- ELSE结构是对等结构，另一种误用就是，错误的使用卫语句，比如如下代码，明明是对等的IF-ELSE结构，硬写成卫语句，也是不合适的。

``` if(男){ //执行男的逻辑 ......省略N行代码..... return 结果； }

// //执行男的逻辑 ......省略N行代码..... return 结果； ```

4.2 方法与参数化

public String getHeight() { if (extParamMap == null) { return null; } return extParamMap.get("height"); }

评注：为每个Map-key添加方法，随着属性的增多，方法会快速膨胀，可以把key做成常量，提供一个通用规范，key作为参数传递进来。本例要看实际场景，有些特定场景包装方法是合适的。

4.3 数字可读性

return new RedisConfig("activityXXX", 604800);

评注：建个常量SevenDay 或 OneWeeK，常量的赋值使用TimeUnit或者相乘赋值都可以，常量计算只执行一次，不会影响效率。

4.4 map和循环

老程序，每次都需要执行遍历，浪费CPU计算资源

public static TableEnum getTableEnum(String field){ for(TableEnum tableEnum : TableEnum.values()){ if(tableEnum.name().equals(field)) return tableEnum; } return null; }

解决方法，直接使用Map存储，直接O1时间度get

private final static Map<String, TableEnum> tables = new HashMap<String, TableEnum>(); static { for(TableEnum tableEnum : TableEnum.values()){ tables.put(tableEnum.name(), tableEnum); } }

总结：Map比for遍历效率更高。

4.5 程序与数据结构

数据结构和程序有时候是可以互相转换的，比如如下程序

if (str.equals("a")) { return "1"; } else if (str.equals("b")) { return "2"; } else if (str.equals("c")) { return "3"; } else { throw new UnsupportedOperationException(); }

其实应该用数据结构表示

Map<String, String> map = new HashMap<>(); map.put("a", "1"); map.put("b", "2"); map.put("c", "3");

使用的时候

if (map.containsKey(str)) { return map.get(str); } else { throw new UnsupportedOperationException(); }

4.6 小心多维逻辑短路

//第一种情况 if(a&b&c){ return true; } //第二种情况 if(c&d){ return true; } //其他情况真的都考虑到了？ return false;

很容易遗漏逻辑，一定要经过逻辑树的推理，考虑每个分支。

如上代码，代码的书写者可能只是把这两种情况考虑清楚了，其他情况呢？这个四维条件(a,b,c,d)的逻辑树有16个分支，是否都考虑到了？

5. 架构设计评审

先有设计，再有代码，写代码之前要先进行设计和设计评审，避免评审设计造成代码重写。设计并不是某次需求重新进行的过程，而是设计要伴随着不同的需求进行迭代更新，要保持在需求的不断打磨下，还能保持架构设计的优雅。下面总结些设计评审中遇到的问题：

5.1 流程图

流程图中仅包含逻辑节点和外部介质(DB，缓存)构成的流程图是不够的，还需要体现影响流程的一些影响设计决策关键信息。比如在M F系统的评审中，流程图应该体现出来：

• MQ消息的不同的子单接收后，会被父单维度防重只有一个能进入流程。(影响设计决策：条件不满足的时候，是否可以靠剩下的子单来触发)
• 中间调度的其他系统得到父子关系，可能不是正确结果。(影响设计决策：不能仅靠子单触发，需要Worker自驱来重试其他系统的延迟)
• 最后一个Worker的任务流程会出结果，不会进行重试。(影响设计决策：如果需要重试的情况，不能进入最后一个Worker)

5.2 批量的概念误用

批量返回的结果是每个单个请求结果的简单叠加，一个接口如果设计为批量，就应该保证做批量的事情。比如多个参数比如(商品1，商品2)，返回的就应该是《商品1，商品1的结果；商品2，商品2的结果》，如果需要商品1和商品2综合考虑，比如把两个商品的价格相加然后做逻辑，这个就不是批量的概念，不应该和批量公用一个接口，不是一件事。