16
votes

I'm currently tackling with the issue of transforming for loops which use bitwise operations using Lambdas in Java 8.

Given a set of complex entries, it's required for the loop to traverse ALL entries and call a given method on them (method returns boolean value). Afterwards, return the result.

In other words, I need to call the method on all entries and store the result. The reason behind this is that each entry independently performs a complex operation and must be performed. The end result is a combination of results.

Code snippet:

boolean areAllSuccessful = true;
for (SomeEntry entry : entries) {
     areAllSuccessful = areAllSuccessful & entry.doComplexAction(); // keep calling the method on other entries regardless of the result.
}

return areAllSuccessful;

The problem is that the Lambda Functions in Java 8 usually perform short-circuit operations (once the false entry has been detected, the "loop" breaks and the false result is returned).

My best solution so far was to use map/filter/count combination:

return entries
       .stream()
       .map(entry -> entry.doComplexAction())
       .filter(result -> result == false)
       .count() > 0

Is there a smarter/cleaner way of doing this?

Thanks!

3
I think your use case is better suited for traditional for loops than for streams. Really, trying to make everything be stream-based is not the way to go. If it works as it is, then leave it that way. - fps
Just a comment: isSuccessful() is a really bad name for a method with side-effects that makes lots of operations. You should 1) make clear in the question that the method has side-effects and thus needs to be called on every entry, and 2) rename the method to something like doComplexActions(). - walen
Don't use side effects if you need streams XOR don't use streams if you need side-effects. Everything else is just asking for troubles. - user11153
that the called method has side effects, should be written more prominently (bold, blinking, in the title and whatever ;-)). - Roland
Don't use bitwise operations to avoid short circuiting. You could easily have just reversed the order (entry.isSuccessful() && areAllSuccessful) to force the method to execute. Don't use screwdrivers to hammer nails. You'll just confuse everyone. Also, don't put a long comment at the end of the line where it'll go off the screen and devs won't read it. Put it on its own line before the line it annotates. - jpmc26

3 Answers

29
votes

Shouldn't that look something like this:

boolean areAllSuccessful = entries.stream()
   .map(entry -> entry.isSuccessful())
   .reduce(Boolean.TRUE, Boolean::logicalAnd);
21
votes

The neatest, and most efficient, way is to use a method reference with allMatch()

return entries.stream().allMatch(SomeEntry::isSuccessful);

If you have 1000's of elements, consider using parallelStream() instead.

This doesn't process every element (it returns at the first false), so if your isSuccessful() method has side effects, it's a bad name and you should rename it or refactor the code to perform the side effects in a process() (or similar) method and have isSuccessful() return the result, throwing an IllegalStateException if process() hasn't first been called.

If you don't refactor, some developer (including you) will call isSuccessful() without realising it "does stuff", which could be bad.

11
votes

You don't really need map() if you're going to use count():

return !(entries
        .stream()
        .filter(entry -> !entry.isSuccessful())
        .count() > 0);

If the method isSuccessful() didn't have side-effects, and all you needed to know was if all the entries were successful, you could have used allMatch():

return entries
        .stream()
        .allMatch(entry -> entry.isSuccessful());

This is indeed a short-circuit operation that will return false as soon as it finds an entry where isSuccessful() is false, not consuming the whole stream unless necessary. But you already commented that isSuccesful() really means "do some complex actions and then tell me if they were successful", so it doesn't apply.