Java lambda: combine doSomething() and removeIf() in one iteration-CodePudding

I want to filter some elements and call stop() function, and then remove them. Here is the sample code:

items.stream().filter(x -> x.children.isEmpty()).forEach(x::stop);
items.removeIf(x -> x.children.isEmpty());

Can we combine these two iterations into one in Java Stream style?

Just like do something then removeif() in one iteration.

CodePudding user response：

Your current code already entails side effects via forEach(), adding more side effects will not make it cleaner.

Instead of using peek() or stateful filter(), I'd rather rewrite your code this way.

Map<Boolean, List<Item>> itemByIsEmpty = items.stream()
            .collect(Collectors.partitioningBy(x -> x.getChildren().isEmpty()));
        
itemByIsEmpty.get(true).forEach(Item::stop);        
List<Item> nonEmpty = itemByIsEmpty.get(false);

CodePudding user response：

Something like this, which will NOT work?

for(Item i : items) {
  if(i.children.isEmpty()) {
    i.stop();
    items.remove(i); // This will fail!
  }
}

The solution is as always to build a new list (I assume that it's a list, but the idea is the same regardless of the collection):

List<Item> unstopped = new ArrayList<>();
for(Item i : items) {
  if(i.children.isEmpty()) {
    i.stop();
  } else {
    unstopped.add(i);
  }
}

And if you don't need to use a stream, might do just fine. It's certainly not hard to read what's going on.

Can we translate this to a lambda? Yes, but I don't like it, because well, the above code was easy to read, and this is not. You need to either create a filter with side effects, so that it returns true if it was not empty, and thus did not have to stop, or use forEach immediately.

// I do not recommend this, it's there for completion:
List<Items> unstopped = items.stream()
  .filter(i -> {
    if(i.children.isEmpty()) {
      i.stop() // side effect
      return false; // we're not keeping these
    } else {
      return true; // we're keeping the non-empty ones
    })
  .collect(toList());

But this looks bad, which is a hint that there's something wrong with the code. I would immensely prefer the older iteration based approach. Just try to give that Predicate we used a name. It's... hard.

The other way is to just use forEach immediately:

List<Item> unstopped = new ArrayList<>();
items.stream().forEach(i -> {
  if(i.children.isEmpty()) {
      i.stop()     
    } else {
      unstopped.add(i)
    })
  });

And that works. And it's kind of honest. But it's still just the same old loop-based way of doing it.

You might get away with using teeing, but it's still iffy. https://dzone.com/articles/java-12-the-teeing-collector

Or you could do what @Alexander Ivanchenko suggested, which works just fine. But I would ask myself this question first:

"Do I really need to use streams? Since I'm dealing with stateful side effects anyway, what's wrong with using the other syntax that handles that sort of thing really well instead?"

If the answer is that yes you need to use a stream, then that's fine. But it might be the wrong hammer for the nail.

CodePudding user response：

Use peek():

items = items.stream()
  .peek(i -> { if (i.children.isEmpty()) i.stop();})
  .filter(i -> !i.children.isEmpty());