一个较重的代码坏味：“炫技式”的单行代码
风格和习惯很重要。
很多代码坏味都是由于不良的风格和习惯导致的，并不涉及高深复杂的技术。
有一些众所周知的代码坏味。当然，也有一些个人觉得很不好的习惯和做法。我个人就不喜欢把多行代码都“挤”到一行的写法。这种代码更像是一种“炫技式”的代码，虽然体现了一点技艺水平，但放在生产环境里并不合适：这样的代码难以理解、排查和修改起来都麻烦且耗时。我认为这种风格的代码很丑陋，这种代码所体现出来的风格和习惯很糟糕。
这样的代码俯拾即是。请看：
代码坏味举例
链式写法的误用
链式写法是把关联的多个方法调用的代码全部“挤”到一行。
究其原因，是这么写很爽。不过爽是有代价的： NPE 潜伏其中，而且报 NPE 时，定位是哪一步调用产生的 NPE 是耗时且费脑的。
如下代码所示：如果 response 为 null，或者 response.getData() 为 null，或者 response.getData().getShoppingCartDTOList() 为 null, 或者 CartResponseBuilderV2::buildCartList 的结果为 null，都会导致 NPE。链式写法仅适合于 Builder 模式，可以平行调用给一个对象的多个字段赋值，但不适合这种递进式的调用。
我之所以不喜欢链式写法，是因为排查过一个链式写法导致的 NPE。需要把链式写法拆成多行，然后单步调试是哪一行报了 NPE。很麻烦，很耗时。我不喜欢自找麻烦，痛恨时间在这种看似很爽却遗患无穷的做法中徒然地消逝：芝麻点的问题排查起来也至少是两个小时。
有两种解决技巧：1. 拆解成多个单行调用并做判空（如果变量有复用更好）； 2. 用 Optional 避免 NPE。
反例：
cartList = response.getData().getShoppingCartDTOList().stream().map(CartResponseBuilderV2::buildCartList).collect(Collectors.toList());
正例：
T data = response.getData();
if (data != null && CollectionUtils.isNotEmpty(data.getShoppingCartDTOList())) {
cartList = StreamUtil.map(data.getShoppingCartDTOList(), CartResponseBuilderV2::buildCartList);
}
多个变量赋值与运算“挤”在一行
这种代码的每一个子句都会潜藏 BUG。此外，这种代码无论如何都是缺乏美感的。
实际上应该分拆成多行，每行一个赋值语句。
反例：
long discounts =
Optional.ofNullable(orderItemInfo).map(OrderItemInfo::getPrice).orElse(0L)
- Optional.ofNullable(orderItemInfo).map(OrderItemInfo::getPcOrderItemPrice)
.map(PcOrderItemPrice::getUnitPrice).orElse(0L);
正例：
Long originPrice = Optional.ofNullable(orderItemInfo).map(OrderItemInfo::getPrice).orElse(0L);
Long unitPrice = Optional.ofNullable(orderItemInfo).map(OrderItemInfo::getPcOrderItemPrice)
.map(PcOrderItemPrice::getUnitPrice).orElse(0L);
long discounts = originPrice - unitPrice
参数的构造与方法的调用糅合在一行
参数构造稍微复杂一点，要弄清楚括号的匹配都是件杀死很多脑细胞的事情。有时甚至会不小心把参数写到不正确的地方，而这时候恰巧不报错，就只能在运行时报错和排查了。我真遇到过。
如下代码所示 如果 findImageIds 方法返回 null 时， addAll 方法会报 NPE。
反例：
configTrustImages.addAll(imageClient.findImageIds(ImageQueryParam.builder().imageIds(imageIds).build()));
正例：
ImageQuery query = ImageQueryParam.builder().imageIds(imageIds).build();
Set<String> imageIds = imageClient.findImageIds(query); // 如果这个方法返回空列表而不是 null ，那就没问题。
if (CollectionUtils.isEmpty(imageIds)) {
configTrustImages.add(imageIds);
}
另一个示例：Long.parseLong(orderInfoList.get(0).getShopId()) 就有潜在的数组越界问题。
反例：
BatchQuerySnapshotDTO requestParam =
buildRequestParam(Long.parseLong(orderInfoList.get(0).getShopId()),
sameUserVersionOrderMap.keySet(), exportContext);
正例：
if (CollectionUtils.isNotEmpty(orderInfoList)) {
Long shopId = Long.parseLong(orderInfoList.get(0).getShopId());
Set<String> sameUserVersionOrderKeys = sameUserVersionOrderMap.keySet();
BatchQuerySnapshotDTO requestParam = buildRequestParam(shopId, sameUserVersionOrderKeys, exportContext);
}
滥用三目运算符
坏味是什么？
- 看这段逻辑，我就看了好一会，耗时耗神；
- 如果要添加关于 refundStatus 的新的逻辑，写在哪里 ？ 继续在里面加 ？
- 里面复杂的 refundStatus 逻辑一定是缺乏覆盖性单测的，代码不可靠。
- 滥用了三目运算符。这些快捷运算符本来是为了写成简洁的单行代码，可是一旦滥用，就会与其原意背道而驰。
解决方法： 把构建 refundStatus 的部分抽离出来，如下所示。这样，这个方法就清晰很多，也很容易进行覆盖性单测，更可靠。
人是很容易效仿的。这样的代码看多了，你也会忍不住来上几行。
拆解单行复杂的代码
自从 Java8 有了 stream 和 lambda 表达式，程序员的自由放纵就如银河之水天上泻，奔流在屏无羁绊。lambda 表达式原本只是用于表达简短的表达式，类似于 C 语言的内联函数，但实际上被当做方法的替代者滥用，且喜欢嵌套在整个逻辑流，让整行代码理解起来很费劲、修改起来很麻烦、几乎难以单独测试。这种代码很容易滋生 BUG，很丑陋。
lambda表达式含有API调用
先来看一个比较简单的反例。如下代码所示：
response.setData(result.stream().map(
container -> ImageContainerDto.toDto(container, hostService.findById(container.getAgentId())))
.collect(Collectors.toList()));
这行代码将多个动作糅合在一起，显得不够清晰：
- hostService.findById(container.getAgentId()) 是一个依赖 IO 的调用；
- result.stream().map(...).collect(...) 是流的转换；
- response.setData 是一个赋值的过程。
可以写成如下，更加清晰：
List<ImageContainerDto> imageContainers = StreamUtil.map(result, this::convert);
response.setData(imageContainers);
public ImageContainerDto convert(Container c) {
Host host = hostService.findById(container.getAgentId());
return ImageContainerDto.toDto(container, host);
}
此外，这行代码可能潜藏性能风险。当 result 条数很多时，每次都去查一遍 DB 获取 host ，有潜在性能（如果接口调用有缓存还好），且 IO 与 流操作（CPU 操作）混合在一起，是不好的做法。应当将 IO 操作和 CPU 操作分离，IO 部分做成批量并发的。进一步地，写成：
List<String> agentIds = StreamUtils.map(result, Container::getAgentId);
List<Host> hosts = hostService.findBatchHosts(agentIds);
Map<String,Host> hostMap = buildMap(hosts);
List<ImageContainerDto> imageContainers = StreamUtils.map(result, container -> ImageContainerDto.toDto(container, hostMap.get(agentId)));
response.setData(imageContainers);
lambda表达式滥用和嵌套
看一看，以下代码你要花多长时间才能理解其意图？ 第一眼，你是理解成了 orderInfoList.forEach(orderInfo -> orderInfo.map.map.map.ifPresent) 还是理解成了 orderInfoList.forEach(orderInfo -> orderinfo.map.map).map.ifPresent , 还是理解成了 orderInfoList.forEach(orderInfo -> orderinfo.map.map.map).ifPresent ？
反例：
orderInfoList.forEach(orderInfo -> Optional.ofNullable(orderInfo.getTcExtra())
.map(extra -> extra.get(EDU_STUDENT_INFO))
.map(studentInfoStr -> JsonUtil.readMap((String) studentInfoStr)).map(studentInfo -> {
Long studentId = (Long) studentInfo.get(STUDENT_ID);
Integer version = (Integer) studentInfo.get(VERSION);
if (studentInfo.containsKey(ID_CARD_NO) && studentId != null && version != null) {
return Pair.of(studentId, version);
} else {
return null;
}
}).ifPresent(
idVersionPair -> sameVersionOrderMap
.computeIfAbsent(idVersionPair, k -> Lists.newArrayList())
.add(orderInfo.getOrderNo())));
stream 数据流处理里的 map, ifPresent 等应用的函数如果比较复杂，应当抽离成新的函数，作为方法引用。分隔成多行之后，代码显得更自然更清晰了。
如何拆解以上的复杂语句呢？
-
先把容易识别的拆成子函数，减少复杂代码体的大小。比如
extra -> extra.get(EDU_STUDENT_INFO)).map(studentInfoStr -> JsonUtil.readMap((String) studentInfoStr)还是可以识别出来的： optional.map.map.get 结构。 用一个子函数来替代getStudentInfo(OrderInfo orderInfo)； -
接着，
studentInfo -> { // ... return Pair.of(studentId, version); else return null; }是一个比较明显的 lambda 表达式滥用，应该抽离出一个子函数buildStuPair；
以上两步抽离出来，这个复杂代码体就比较小了。接下来，可能会有点迷惑。 ifPresent 到底是最外层的 foreach 调用的接续调用还是里层的 lambda 表达式的接续调用，add 是 ifPresent 的接续调用还是哪里的接续调用？ 这时候，要数括号的匹配了。不了解 ifPresent 方法的人，肯定很难继续往下拆分和理解。
话说回来，能够写出这段代码的人，对 stream, optional 之类的运用还是可以的。
正例：
public class NestedLambda {
private static final String EDU_STUDENT_INFO = "student_info";
private static final String STUDENT_ID = "student_id";
private static final String VERSION = "version";
private static final String ID_CARD_NO = "id_card";
public void nestedLambda() {
List<OrderInfo> orderInfoList = Lists.newArrayList();
Map<Pair, List<String>> sameVersionOrderMap = new HashMap();
orderInfoList.forEach(orderInfo -> buildStudentPair(orderInfo).ifPresent(
idVersionPair -> setOrderMap(idVersionPair, orderInfo, sameVersionOrderMap)));
}
private void setOrderMap(Pair idVersionPair, OrderInfo orderInfo, Map<Pair, List<String>> sameVersionOrderMap) {
sameVersionOrderMap
.computeIfAbsent(idVersionPair, k -> Lists.newArrayList())
.add(orderInfo.getOrderNo());
}
private Optional<Pair> buildStudentPair(OrderInfo orderInfo) {
return getStudentInfo(orderInfo).map(this::buildStuPair);
}
private Optional<Map> getStudentInfo(OrderInfo orderInfo) {
return Optional.ofNullable(orderInfo.getTcExtra())
.map(extra -> extra.get(EDU_STUDENT_INFO))
.map(studentInfoStr -> JsonUtil.readMap((String) studentInfoStr));
}
private Pair buildStuPair(Map studentInfo) {
Long studentId = (Long) studentInfo.get(STUDENT_ID);
Integer version = (Integer) studentInfo.get(VERSION);
if (studentInfo.containsKey(ID_CARD_NO) && studentId != null && version != null) {
return Pair.of(studentId, version);
} else {
return null;
}
}
}
为什么会写出这样的代码
为什么会写出这种“炫技式”的代码呢？ 究其根因，我认为有两点：
-
在乎表达逻辑的“爽”感，而不在乎阅读者的“爽”感；
-
缺乏表达语义的意识和方法技巧，语文不过关。
很多程序员仍然觉得编程只是编写大段大段的逻辑，写完逻辑就觉得完成任务了，而没有意识到：编程本质是一种表达。这种表达不仅要保证逻辑的准确性以供机器执行，更要保证语义的可读性和可理解性，供他们的同行阅读和改进。因此，代码在某种意义上类似于“文章”，是考验语言组织能力的。能不能把语义表达清晰明白，也是编程技艺的一大看点。
优化类似代码的方法也很简单：识别其中的语义，赋值给变量，抽离出函数，这样，缠结不清的复杂代码体就会化解成清晰可读的代码，回到它的本色。
小结
很多编程语言赋予了程序员很大的灵活性。然而，如果程序员不克制冲动的欲望，自由放纵地写代码，就很容易写出看上去很爽但实际上很丑陋的代码。
代码都挤在一行，也会感觉不舒服的。想想你挤地铁的感受，考虑下代码的感受。
