一个 Java Optional + Stream 重构的小例子

[TWorldIsNButThis]

public Optional<MyLog> lastLog(Collection<Long> shopIds) {
    List<Long> allAppIds = StreamUtils.map(appDao.findAll(), App::getId);
    List<MyLog> logs = new ArrayList<>();
    shopIds.forEach(sid -> {
        allAppIds.forEach(aid -> {
            MyLog lastLog = logDao.findLast(aid, sid, null, LocalDate.now().plusDays(1));
            if (Objects.nonNull(lastLog)) {
                logs.add(lastLog);
            }
        });
    });
    if (CollectionUtils.isEmpty(logs)) {
        return Optional.empty();
    }
    logs.sort(Comparator.comparing(MyLog::getTime));
    return Optional.of(Iterables.getLast(logs));
}

public Optional<MyLog> lastLog(Collection<Long> shopIds) {
    if (shopIds.isEmpty()) {
        return Optional.empty();
    }
    var allAppIds = appDao.findAllId();
    var end = LocalDate.now().plusDays(1);
    return shopIds.stream()
            .flatMap(shopId -> allAppIds.stream().flatMap(appId -> logDao.findLast(appId, shopId, end).stream()))
            .max(Comparator.comparing(MyLog::getTime));
}

我们用的 java 11 ，部分方法 java 8 可能没有

类名和方法名有做部分处理

原来的方法不知道为什么主动用了 optional 但还是写成这么一大坨

调整后的 early return 其实也能合并到 optional 里串起来，不过想想还是算了，反倒显得更麻烦

另外这个 max 是 idea 自动提示的调整，一开始是照着原方法的思路写成 sorted + findFirst

idea 很多 warning 都可以很方便的 alt + enter 直接重构掉

copilot 锐评： Overall, the refactored implementation is more concise and readable than the original implementation, and makes use of functional programming constructs to achieve the same result. It also avoids the use of nested loops, which can be harder to read and understand.

[allenzhangSB]

重构后还是在循环中查 db, 这个是最应该重构掉的

[yazinnnn]

如果是查库的话, 为啥不直接写 sql?

[oneisall8955]

日志表分库分表了吗？循环里面查 db 有影响

[anonydmer]

看起来就是对一个 appID 和一个 shopID 两个 id 集合做了笛卡尔集的遍历查最后一条日志，在不考虑楼上几位说的性能的前提下我更乐意写成：

shopIDs.stream()
.flatMap( s -> appIDs.stream().map( a -> new Pair(s, a)) )
.map(p -> logDAO.findLast(p.first(), p.second(), end)
.max(xxx)

第一步单独构建两个 id 集合的笛卡尔集（其实用个别的库可能比 stream 更方便）
第二步遍历取数据，这一步其实是可以利用 Stream 的并行计算的
第三步 max

[tulongtou]

循环里面操作数据库真是最应该优化的地方

[oldshensheep]

你写代码是不是一句 SQL 不用写，全用 Java 处理

App 表也不知道里面存的什么东西，没改前的 appDao.findAll()，我也是惊呆了。

[xuanbg]

换我写的话，一句 sql 直接搞定。。。

[nikenidage1]

脑壳疼
如果你用 C#，大概就是这样

shopIds.Where(log => log != null).OrderBy(log => log.Time).LastOrDefault();

[liprais]

没做过 profile 吧
循环里面查数据库

[StevenQAQ]

我理解你把 logDao 的查询提出来会好点，比如用日期范围查询一下，如果数据量很大就把 aid 和 sid 的范围数据也传进去。比循环查效率好很多。

[season8]

如果表设计得合理，直接按 shopIds 查询不就可以了，都不需要用 appId ，也不需要循环查询，一个 sql 就解决了

[imv2er]

我见了我深恶痛绝的返回 Optional<xxxx>

[TWorldIsNButThis]

回复一下，首先 findLastLog 并不是一个 mysql 简单查询，本身有其特有逻辑，它甚至不是 mysql

然后这个重构也并不是意在调整设计方案，而是调整查询的业务逻辑后顺手精简一下调用处代码

最后”不要循环调用查询“这类教条并不是放之四海而皆准的铁律，一切都要由结合业务做评估，对于业务代码，可读性永远是默认第一位的，性能优化要以实际表现为准。

[TWorldIsNButThis]

@anonydmer 如果是 kotlin ，我会这么写，而且构造完 pair 在 map 中使用的时候必须是解构声明而不是 first second 取值

但对于 java 还是直接一些为好

[hhhh115]

@TWorldIsNButThis #13 '不要循环调用查询' 这个并不怎么赞同吧，如果循环中写查询，每一个循环都要和数据库传输数据。有点儿像在，for 循环里进行 http 请求。
感觉如果是，正常 for 循环处理数据（不过网络，磁盘）的话，可读性确实更重要些。查询的话，先把数据处理好给数据库，然后一次查出所有出局，可能会好点吧。可读性差的话，多写点注释就好了吧

[Slurp]

@nikenidage1 扯淡，你这个逻辑都不对应。flatMap 哪去了？

[2013881276]

@anonydmer 补充完整了点：public Optional<MyLog> lastLog(final Collection<Long> shopIds) {
return Optional.ofNullable(CollectionUtil.isEmpty(shopIds) ? null : shopIds)
.map(shopIdList -> Pair.of(shopIdList, appDao.findAllId()))
.map(pair -> pair.getFirst()
.stream()
.flatMap(shopId -> {
final LocalDate end = LocalDate.now().plusDays(1);
return pair.getSecond().stream().flatMap(appId -> logDao.findLast(appId, shopId, end).stream());
})
.max(Comparator.comparing(MyLog::getTime)))
.orElse(Optional.empty());
}

[mmdsun]

排序 Comparator.comparing(MyLog::getTime)为空会报错吧？
可以换成一个 Comparator.Comparator.nullsFirst()或 Comparator.nullsLast()

[mmdsun]

@mmdsun 推荐用 Rxjava 或者 Reactor Project （或者用像 Spring Webflux 这种） ，Java 内置的操作符太少不够用。写起来也难看。

[zengguibo]

最应该优化的是在循环调用查询，正常是一次将数据取出来，在内存中比对

[forgottencoast]

@nikenidage1
真的，和 C#的 Linq 对比起来 Java 的 Stream 太难用了。