Single exit point meets Java streams

Question

We currently have a sensitive discussion going in the company that touches on a couple of old primary opinion based discussions. Nevertheless, I would like to discuss the case in this forum to establish some sensible code guidelines. Let's assume we have the following two classes of non-modifiable legacy classes:

 public class SomeValue {

    private String value;

    public SomeValue(final String value) {
        this.value = value;
    }

    public String getValue() {
        return value;
    }
}

public class SomeCondition {

    private SomeValue a;
    private SomeValue b;

    public SomeCondition(final SomeValue a, final SomeValue b) {
        this.a = a;
        this.b = b;
    }

    public boolean isA() {
        return a != null;
    }

    public boolean isB() {
        return b != null;
    }

    public String getAOrBValue() {
        if (isA()) {
            return a.getValue();
        }
        if (isB()) {
            return b.getValue();
        }
        return null;
    }
}

Now, in discussion is the best style for a method that checks whether any of a or b is set and returns its value (prioritizing a). The two versions on offer are those:

Version 1:

    protected String getValue(final List<SomeCondition> conditions) {
    if (isNotEmpty(conditions)) {
        for (SomeConditions conditions : condition) {

            if (condition.isA())) {
                return a.getValue();
            }

            if (condition.isB()) {
                return b.getValue();
            }
        }
    }

    return "default";
}

Critics of this version say, the returns inside several if statements being contained in a for loop not only hide else statements but also a break statement. If one tried to have one return at the end of the method, one would see all this hidden complexity definitely forcing one to refactor. If one had to bugfix this method, one would be in serious trouble.

Version 2:

protected String getValue(final List<SomeCondition> conditions) {
    return conditions
    .stream()
    .map(this::getAOrBValue)
    .filter(match -> match != null)
    .findFirst()
    .orElse("default");
}

Critics of this version say, this code is hard to read since it misuses java streaming api to handle something that usually would be covered by simple for and if statements. Also, the filtering algorithm is harder to understand since it is distributed over two classes.

I am aware that part of this discussion is primary opinion based. Nevertheless, I feel that there are some firmly established programming conventions that could help us, decide which of the two styles we want to make official in our code guidelines. Can anybody shed some light on the matter?

Answer 1

it misuses java streaming api to handle something that usually would be covered by simple for and if statements

This is a vacuous argument. Everything the streaming API does except for parallelization can be handled by for and if statements. Whether they're simple is a matter of taste; IMO your first example is already not simple. (You also managed to put a compile error and a redundant if into it; in my experience it's a sign of unnecessary complexity if the recreation for a question on SO already contains bugs.)

That said, the comparison is not fair, since the first version can be simplified.

protected String getValue(final List<SomeCondition> conditions) {
    for (SomeCondition condition : conditions) {
        String value = condition.getAOrBValue();
        if (value != null)
            return value;
    }

    return "default";
}

There is no point in doing a check for an empty collection before the loop; if it's empty, the loop will simply not run. And not using the getAOrBValue method, which conveniently already packages the double conditional with prioritization of A, is just making your job harder.

On the other hand, the stream version can also be improved a lot by adding a very generic helper function somewhere and using that. (Note: I'm a bit rusty with Java generics, there might be syntax errors here.)

// In some StreamUtility class
public static <T, U> Stream<T> mapNonNull(Stream<U> stream, Function<? super U, T> function) {
  return stream.map(function).filter(x => x != null);
}

// In your class
import static StreamUtility.mapNonNull;

protected String getValue(final List<SomeCondition> conditions) {
  return mapNonNull(conditions.stream(), Condition::getAOrBValue)
    .findFirst().orElse("default");
}

Aside from losing the pretty chain syntax, this version is a lot more readable because it packages the "map and filter" operation, which is the source of most of the complexity.

Of course, one of the arguments against the non-stream version is also nonsensical:

If one tried to have one return at the end of the method, one would see all this hidden complexity definitely forcing one to refactor.

The question is, why would you ever want to do this? Single-exit hasn't been a goal for most programmers in a long time.

In the end, given that you can reduce the loop version to a single if, and Java doesn't support pretty extension methods, I'd go for the loop version in this particular instance. However, this is a judgement call for just this case, and not an opinion about streams in general.

Answer 2

I would frame your company policy around readability, and consequently judge everything based around that, rather than a specific policy of having one exit point (or not). Code has many more readers that writers, and you want the code to be easily comprehensible.

Answer 3

The last option is definitively more difficult to upgrade or edit and since it hides what's behind those linq-like function it could be difficult to debug. To solve the " If one tried to have one return at the end of the method, one would see all this hidden complexity definitely forcing one to refactor" problem i would do something like this:

protected String getValue(final List<SomeCondition> conditions) 
{
    if (isNotEmpty(conditions)) 
    {
        List<String> values = new List<String>();

        for (int i = 0; i < conditions.size(); i++) 
        {

            if (condition.get(i).isA())) 
            {
                values.add(a.getValue());
            }
            else if (condition.isB()) 
            {
                values.add(b.getValue());
            }
            else    
                values.add("default");
         }
    }

    return values.get(0);
}

In this way the function is very easy to edit. You can easly add a new condition with another else if and put any logic before the return. You can even return another value that maches the criteria instead of the first or use those values for your calculations. My concern is another. First it is unclear where you get a or b to call the getValue method. And second in the SomeCondition legacy class you already have a public String getAOrBValue() method that makes the same thing you and your colleagues are trying to do with a new function. So why don't just make something like this?

protected String getValue(final List<SomeCondition> conditions) 
{
    if (isNotEmpty(conditions)) 
    {
        List<String> values = new List<String>();
        for (SomeConditions condition : conditions) 
        {
            values.add(condition.getAOrBValue());         
        }
    }

    //you can add some filter logic on values here
    return values.get(0);
}