其他分享
首页 > 其他分享> > 一个较重的代码坏味：“炫技式”的单行代码

一个较重的代码坏味：“炫技式”的单行代码

作者：互联网

风格和习惯是很重要的。


很多代码坏味都是由于不良的风格和习惯导致的，并不涉及高深复杂的技术。

有一些众所周知的代码坏味。当然，也有一些个人觉得很不好的习惯和做法。我个人就不喜欢把多行代码都“挤”到一行的写法。这种代码更像是一种“炫技式”的代码，虽然体现了一点技艺水平，但放在生产环境里并不合适：这样的代码难以理解、排查和修改起来都麻烦且耗时。我认为这种风格的代码很丑陋，这种代码所体现出来的风格和习惯很糟糕。

这样的代码俯拾即是。请看：

代码坏味举例

链式写法的误用

链式写法是把关联的多个方法调用的代码全部“挤”到一行。

究其原因，是这么写很爽。不过爽是有代价的： NPE 潜伏其中，而且报 NPE 时，定位是哪一步调用产生的 NPE 是耗时且费脑的。

如下代码所示：如果 response 为 null，或者 response.getData() 为 null，或者 response.getData().getShoppingCartDTOList() 为 null, 或者 CartResponseBuilderV2::buildCartList 的结果为 null，都会导致 NPE。链式写法仅适合于 Builder 模式，可以平行调用给一个对象的多个字段赋值，但不适合这种递进式的调用。

我之所以不喜欢链式写法，是因为排查过一个链式写法导致的 NPE。需要把链式写法拆成多行，然后单步调试是哪一行报了 NPE。很麻烦，很耗时。我不喜欢自找麻烦，痛恨时间在这种看似很爽却遗患无穷的做法中徒然地消逝：芝麻点的问题排查起来也至少是两个小时。

有两种解决技巧：1. 拆解成多个单行调用并做判空（如果变量有复用更好）； 2. 用 Optional 避免 NPE。

反例：


if (response.getData() != null && CollectionUtils.isNotEmpty(response.getData().getShoppingCartDTOList())) {
      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);
}

滥用三目运算符

坏味是什么？

  1. 看这段逻辑，我就看了好一会，耗时耗神；
  2. 如果要添加关于 refundStatus 的新的逻辑，写在哪里 ？ 继续在里面加 ？
  3. 里面复杂的 refundStatus 逻辑一定是缺乏覆盖性单测的，代码不可靠。
  4. 滥用了三目运算符。这些快捷运算符本来是为了写成简洁的单行代码，可是一旦滥用，就会与其原意背道而驰。

解决方法： 把构建 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()));

这行代码将多个动作揉和在一起，显得不够清晰：

可以写成如下，更加清晰：


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 等应用的函数如果比较复杂，应当抽离成新的函数，作为方法引用。分隔成多行之后，代码显得更自然更清晰了。

如何拆解以上的复杂语句呢？

以上两步抽离出来，这个复杂代码体就比较小了。接下来，可能会有点迷惑。 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;
        }
    }
}

小结

很多编程语言赋予了程序员很大的灵活性。然而，如果程序员不克制冲动的欲望，自由放纵地写代码，就很容易写出看上去很爽但实际上很丑陋的代码。

代码都挤在一行，也会感觉不舒服的。想想你挤地铁的感受，考虑下代码的感受。


标签：map,炫技,orderInfo,get,代码,null,坏味,response
来源： https://www.cnblogs.com/lovesqcc/p/16559923.html