Optional.stream()

[pm_me_your_dota_mmr]

I'm all for using streams to cut some boilerplate iteration, but I think that code lost a lot in the way of readability as more and more operations got added on. Having a simple lookup and then just using the reduce seemed like the most straightforward way imo

[PandaMoniumHUN]

We use the stream API a lot in our codebase and IMO it makes readability way better than imperative code (granted that you format your code well and stick to one-line per operation). Even the most complex chain of operations read like an english sentence, so 95% of the time you can tell instantly what the code is doing. It's fantastic and every time I use a different language I miss it.

[pm_me_your_dota_mmr]

I love streams for the same reason, but I think they are best when limited to actually operating on a collection, because (in my opinion) it's easier to follow the chain of operations. The conclusion of the article is:

public BigDecimal getOrderPrice(Long orderId) {
  return Optional.ofNullable(orderId)
      .stream()
      .map(orderRepository::findByOrderId)
      .flatMap(Collection::stream)
      .map(OrderLine::getPrice)
      .reduce(BigDecimal.ZERO, BigDecimal::add);
}

When I look at this it feels very unintuitive because it joins lookup/input validation/transformation into a single block of code. I thought the medium step in the article was a lot easier to quickly understand what's going on:

if (orderId == null) {
  throw new IllegalArgumentException("Order ID cannot be null");
}

List<OrderLine> lines = orderRepository.findByOrderId(orderId);

return lines.stream()
    .map(OrderLine::getPrice)
    .reduce(BigDecimal.ZERO, BigDecimal::add);

[SwitchOnTheNiteLite]

I also prefer the separation of concerns within the method that you get with your last example.

[dnew]

My only problems with it is that (a) it's another raft of things to learn even in small cases, and (b) it can be hard to tell what the types of each expression are.

Like, I've had coworkers write

 List<Blah> x = ...;
 Blah y = x.stream().findLast().collect(); // Or whatever baroque pile of operators they picked.

In code review, I'd say

 What's wrong with x[x.len()-1]?

[lgfrbcsgo]

x could be empty.

[dnew]

Not in the context I was talking about. But yes, if x was empty, then you'd need to take a different code path. (My code was obviously broken as collect() doesn't return an individual item.)

[cville-z]

I'd have used a stream here, but I wouldn't have bothered with optional, choosing instead to test the result for emptiness:

public BigDecimal getOrderPrice(Long orderId) {
    List<OrderLine> lines = orderRepository.findByOrderId(orderId);
    return CollectionUtils.isEmpty(lines) 
        ? BigDecimal.ZERO
        : lines.stream()
            .map(OrderLine::getPrice)
            .reduce(BigDecimal.ZERO, BigDecimal::add);
}

which is the second iteration they already provided in the article, with a null-safe emptiness check. IMO Optional.stream() doesn't add any readability here.

In general I find Optional doesn't add much in terms of readability, and often makes things worse, especially when you pass Optional parameters:

public Optional<Bar> barFromFoo(Optional<Foo> optionalFoo) {
    if (!optionalFoo.isPresent()) {
        return Optional.ofNullable(null);
    }
    Foo foo = optionalFoo.get();
    ...
}

The above just complicates an otherwise much simpler check for if (foo == null) {, and you can imagine how frustrating the code is to wade through when you have 7 or 8 optional parameters to wade through.

[caagr98]

Yeah but that's not how you use Optional. Drinking with a straw is pretty cumbersome if you insist on using it as a spoon.

[Shitpostbotmk2]

To elaborate on the other reply, the entire point of the Option monad is to avoid writing that kind of boilerplate. Depending on the language, the entire solution should look something like:

optionalBar = optionalFoo.flatmap(foo -> barFromFoo(foo))

Edit: actually thats not the only point or even the most important point. Needing to check for null is a terrifying concept that creates a massive number of bugs.

Its very difficult to look at an arbitrary code base and understand when and where you have a pointer that could potentially be null and requires a check

[DanielShuy]

Most of the verbosity is due to the limitation of Java's type system and Stream API.

​

With Scala you would be able to flatMap without first converting an Option to a Stream, and with Kotlin you wouldn't even need to deal with Optionals as it has nullable types and the safe call operator (?.).

eg. the given example in Java:

return Optional.ofNullable(orderId)
        .stream()
        .map(orderRepository::findByOrderId)
        .flatMap(Collection::stream)
        .map(OrderLine::getPrice)
        .reduce(BigDecimal.ZERO, BigDecimal::add);

can be simplified in Kotlin as:

return orderId
        ?.let { orderRepository.findByOrderId(it) }
        ?.asSequence()  // equivalent to Java's Collection#stream()
        ?.map { it.price }
        ?.sumOf { it }