How to split up complex conditions and keep short circuit evaluation?

Question

Sometimes conditions can become quite complex, so for readability I usually split them up and give each component a meaningful name. This defeats short-circuit evaluation however, which can pose a problem. I came up with a wrapper approach, but it is way too verbose in my opinion.

Can anybody come up with a neat solution for this?

See code below for examples of what I mean:

public class BooleanEvaluator {

    // problem: complex boolean expression, hard to read
    public static void main1(String[] args) {

        if (args != null && args.length == 2 && !args[0].equals(args[1])) {
            System.out.println("Args are ok");
        }
    }

    // solution: simplified by splitting up and using meaningful names
    // problem: no short circuit evaluation
    public static void main2(String[] args) {

        boolean argsNotNull = args != null;
        boolean argsLengthOk = args.length == 2;
        boolean argsAreNotEqual = !args[0].equals(args[1]);

        if (argsNotNull && argsLengthOk && argsAreNotEqual) {
            System.out.println("Args are ok");
        }
    }

    // solution: wrappers to delay the evaluation 
    // problem: verbose
    public static void main3(final String[] args) {

        abstract class BooleanExpression {
            abstract boolean eval();
        }

        BooleanExpression argsNotNull = new BooleanExpression() {
            boolean eval() {
                return args != null;
            }
        };

        BooleanExpression argsLengthIsOk = new BooleanExpression() {
            boolean eval() {
                return args.length == 2;
            }
        };

        BooleanExpression argsAreNotEqual = new BooleanExpression() {
            boolean eval() {
                return !args[0].equals(args[1]);
            }
        };

        if (argsNotNull.eval() && argsLengthIsOk.eval() && argsAreNotEqual.eval()) {
            System.out.println("Args are ok");
        }
    }
}

Response to answers:

Thanks for all your ideas! The following alternatives were submitted so far:

• Break lines and add comments
• Leave as is
• Extract methods
• Early returns
• Nested / Split up if's

Break lines and add comments:

Just adding linebreaks within a condition gets undone by the code formatter in Eclipse (ctrl+shift+f). Inline comments helps with this, but leaves little space on each line and can result in ugly wrapping. In simple cases this might be enough however.

Leave as is:

The example condition I gave is quite simplistic, so you might not need to address readability issues in this case. I was thinking of situations where the condition is much more complex, for example:

private boolean targetFound(String target, List<String> items,
        int position, int min, int max) {

    return ((position >= min && position < max && ((position % 2 == 0 && items
            .get(position).equals(target)) || (position % 2 == 1)
            && position > min && items.get(position - 1).equals(target)))
            || (position < min && items.get(0).equals(target)) || (position >= max && items
            .get(items.size() - 1).equals(target)));
}

I would not recommend leaving this as it is.

Extract methods:

I considered extracting methods, as was suggested in several answers. The disadvantage of that is that these methods typically have a very low granularity and may not be very meaningful by themselves, so it can clutter your class, for example:

private static boolean lengthOK(String[] args) {
    return args.length == 2;
}

This would not really deserve to be a separate method at class level. Also you have to pass all the relevant arguments to each method. If you create a separate class purely for evaluating a very complex condition then this might be an ok solution IMO.

What I tried to achieve with the BooleanExpression approach is that the logic remains local. Notice that even the declaration of BooleanExpression is local (I don't think I've ever come across a use-case for a local class declaration before!).

Early returns:

The early returns solution seems adequate, even though I don't favor the idiom. An alternative notation:

public static boolean areArgsOk(String[] args) {

    check_args: {
        if (args == null) {
            break check_args;
        }
        if (args.length != 2) {
            break check_args;
        }
        if (args[0].equals(args[1])) {
            break check_args;
        }
        return true;
    }
    return false;
}

I realize most people hate labels and breaks, and this style might be too uncommon to be considered readable.

Nested/split up if's:

It allows the introduction of meaningful names in combination with optimized evaluation. A drawback is the complex tree of conditional statements that can ensue

Showdown

So to see which approach I utlimately favor, I applied several of the suggested solutions to the complex targetFound example presented above. Here are my results:

nested / split if's, with meaningful names very verbose, meaningful names don't really help the readability here

private boolean targetFound1(String target, List<String> items,
        int position, int min, int max) {

    boolean result;
    boolean inWindow = position >= min && position < max;
    if (inWindow) {

        boolean foundInEvenPosition = position % 2 == 0
                && items.get(position).equals(target);
        if (foundInEvenPosition) {
            result = true;
        } else {
            boolean foundInOddPosition = (position % 2 == 1)
                    && position > min
                    && items.get(position - 1).equals(target);
            result = foundInOddPosition;
        }
    } else {
        boolean beforeWindow = position < min;
        if (beforeWindow) {

            boolean matchesFirstItem = items.get(0).equals(target);
            result = matchesFirstItem;
        } else {

            boolean afterWindow = position >= max;
            if (afterWindow) {

                boolean matchesLastItem = items.get(items.size() - 1)
                        .equals(target);
                result = matchesLastItem;
            } else {
                result = false;
            }
        }
    }
    return result;
}

nested / split if's, with comments less verbose, but still hard to read and easy to create bugs

private boolean targetFound2(String target, List<String> items,
        int position, int min, int max) {

    boolean result;
    if ((position >= min && position < max)) { // in window

        if ((position % 2 == 0 && items.get(position).equals(target))) {
            // even position
            result = true;
        } else { // odd position
            result = ((position % 2 == 1) && position > min && items.get(
                    position - 1).equals(target));
        }
    } else if ((position < min)) { // before window
        result = items.get(0).equals(target);
    } else if ((position >= max)) { // after window
        result = items.get(items.size() - 1).equals(target);
    } else {
        result = false;
    }
    return result;
}

early returns even more compact, but the conditional tree remains just as complex

private boolean targetFound3(String target, List<String> items,
        int position, int min, int max) {

    if ((position >= min && position < max)) { // in window

        if ((position % 2 == 0 && items.get(position).equals(target))) {
            return true; // even position
        } else {
            return (position % 2 == 1) && position > min && items.get(
                    position - 1).equals(target); // odd position
        }
    } else if ((position < min)) { // before window
        return items.get(0).equals(target);
    } else if ((position >= max)) { // after window
        return items.get(items.size() - 1).equals(target);
    } else {
        return false;
    }
}

extracted methods results in nonsensical methods in your class the parameter passing is annoying

private boolean targetFound4(String target, List<String> items,
        int position, int min, int max) {

    return (foundInWindow(target, items, position, min, max)
            || foundBefore(target, items, position, min) || foundAfter(
            target, items, position, max));
}

private boolean foundAfter(String target, List<String> items, int position,
        int max) {
    return (position >= max && items.get(items.size() - 1).equals(target));
}

private boolean foundBefore(String target, List<String> items,
        int position, int min) {
    return (position < min && items.get(0).equals(target));
}

private boolean foundInWindow(String target, List<String> items,
        int position, int min, int max) {
    return (position >= min && position < max && ((position % 2 == 0 && items
            .get(position).equals(target)) || (position % 2 == 1)
            && position > min && items.get(position - 1).equals(target)));
}

BooleanExpression wrappers revisited note that the method parameters must be declared final for this complex case the verbosity is defendable IMO Maybe closures will make this easier, if they ever agree on that (-

private boolean targetFound5(final String target, final List<String> items,
        final int position, final int min, final int max) {

    abstract class BooleanExpression {
        abstract boolean eval();
    }

    BooleanExpression foundInWindow = new BooleanExpression() {

        boolean eval() {
            return position >= min && position < max
                    && (foundAtEvenPosition() || foundAtOddPosition());
        }

        private boolean foundAtEvenPosition() {
            return position % 2 == 0 && items.get(position).equals(target);
        }

        private boolean foundAtOddPosition() {
            return position % 2 == 1 && position > min
                    && items.get(position - 1).equals(target);
        }
    };

    BooleanExpression foundBefore = new BooleanExpression() {
        boolean eval() {
            return position < min && items.get(0).equals(target);
        }
    };

    BooleanExpression foundAfter = new BooleanExpression() {
        boolean eval() {
            return position >= max
                    && items.get(items.size() - 1).equals(target);
        }
    };

    return foundInWindow.eval() || foundBefore.eval() || foundAfter.eval();
}

I guess it really depends on the situation (as always). For very complex conditions the wrapper approach might be defendable, although it is uncommon.

Thanks for all your input!

EDIT: afterthought. It might be even better to create a specific class for complex logic such as this:

import java.util.ArrayList;
import java.util.List;

public class IsTargetFoundExpression {

    private final String target;
    private final List<String> items;
    private final int position;
    private final int min;
    private final int max;

    public IsTargetFoundExpression(String target, List<String> items, int position, int min, int max) {
        this.target = target;
        this.items = new ArrayList(items);
        this.position = position;
        this.min = min;
        this.max = max;
    }

    public boolean evaluate() {
        return foundInWindow() || foundBefore() || foundAfter();
    }

    private boolean foundInWindow() {
        return position >= min && position < max && (foundAtEvenPosition() || foundAtOddPosition());
    }

    private boolean foundAtEvenPosition() {
        return position % 2 == 0 && items.get(position).equals(target);
    }

    private boolean foundAtOddPosition() {
        return position % 2 == 1 && position > min && items.get(position - 1).equals(target);
    }

    private boolean foundBefore() {
        return position < min && items.get(0).equals(target);
    }

    private boolean foundAfter() {
        return position >= max && items.get(items.size() - 1).equals(target);
    }
}

The logic is complex enough to warrant a separate class (and unit test, yay!). It will keep the code using this logic more readable and promotes reuse in case this logic is needed elsewhere. I think this is a nice class because it truely has a single responsibility and only final fields.

Answer 1

Your first solution is good for this kind of complexity. If the conditions were more complex, I would write private methods for each check you need to run. Something like:

public class DemoStackOverflow {

    public static void main(String[] args) {
    if ( areValid(args) ) {
        System.out.println("Arguments OK");
    }
    }

    /**
     * Validation of parameters.
     * 
     * @param args an array with the parameters to validate.
     * @return true if the arguments are not null, the quantity of arguments match 
     * the expected quantity and the first and second are not equal; 
     *         false, otherwise.
     */
    private static boolean areValid(String[] args) {
       return notNull(args) && lengthOK(args) && areDifferent(args);
    }

    private static boolean notNull(String[] args) {
       return args != null;
    }

    private static boolean lengthOK(String[] args) {
       return args.length == EXPECTED_ARGS;
    }

    private static boolean areDifferent(String[] args) {
       return !args[0].equals(args[1]);
    }

    /** Quantity of expected arguments */
    private static final int EXPECTED_ARGS = 2;

}