programming with short-circuit evaluation in Java

Question

Does relying on short-circuit evaluation make the code fragile? I wrote a piece of code that essentially looks like the following. My professor wanted me to rewrite it.

(Note: I know for sure that only one of the four conditions will be true, because given any stream, there is only one 'next token', right?)

foo getFoo()
{
        Bar bar;
        if ((bar = peekAndGet('x')) != null 
                || (bar = peekAndGet('y')) != null 
                || (bar = peekAndGet('z')) != null 
                || (bar = peekAndGet('t')) != null) 
            return produce(bar);
        else 
            return null;
}

Is this really fragile? I find it working perfectly. But how should I rewrite it?

Answer 1

I would refactor it and write it like this:

char[] values = {'x', 'y', 'z', 't'};
for (char c : values) {
    Bar bar = peekAndGet(c);
    if (bar != null) return produce(bar);
}
return null;

Note: one good reason to do it, is that the first time I read your code I thought it looked buggy until I read your question. You want to keep those "Something looks wrong" moments for things that really are wrong.

Answer 2

you are initializing your Bar object inside the if statement, try initializing it outside the if statement.

Answer 3

It's not the short-circuiting alone that's the problem. It's short-circuiting plus side effects that's probably the target. It's widely frowned upon because it makes code harder to understand and easier to break. In your case, it's a little less onerous, but take this example:

if ((b && c++ > 1) || (c++ < 10)) { ... }

Who can possibly keep track of what's happening to c in a case like that?

Answer 4

The code is not in the least bit fragile. Its behavior is completely specified by the Java Language Specification, and does what I think you intend.

Having multiple side-effects, especially multiple assignments to the same variable, can make code less readable, and is discouraged in the JLS.