Should I use precondition checks to check intermediate results?

Question

Guava offers helper functions to check the preconditions but I could not find helper functions to check intermediate results.

private void foo(String param)
{
    checkNotNull(param, "Required parameter is not set");

    int x = get(param);
    if (x == -1) {
        throw new RuntimeException("This should never have happened and indicates a bug.");
    }
}

• Should I wrap the if (...) {....} part in my own helper?
• Or should I use checkState from Guava?
• Or should I view the failure of get() as a consequence of param and use checkArgument?
• Should I use asserts in these cases?
• Or am I missing something?

Answer 1

It's somewhere between a matter of preference and a matter of convention.

Generally, people will use asserts to indicate programming errors; that is, "if I did my job right, then a non-null param should never result in a -1 from get, regardless of user input or other outside forces." I treat them almost as comments that can optionally be verified at runtime.

On the other hand, if get might return -1 in some cases, but that input is invalid, then I would generally throw an IllegalArgumentException, and checkArgument is a perfectly reasonable way to do this. One drawback this has is that when you later catch that, it could have come from pretty much anywhere. Consider:

try {
    baz();
    bar();
    foo(myInput);
} catch (IllegalArgumentException e) {
    // Where did this come from!?
    // It could have come from foo(myInput), or baz(), or bar(),
    // or some method that any of them invoked, or really anywhere
    // in that stack.
    // It could be something totally unrelated to user input, just
    // a bug somewhere in my code.
    // Handle it somehow...
}

In cases where that matters -- for instance, you want to pop up a helpful note to the user that they're not allowed to enter -1 in their input form -- you may want to throw a custom exception so that you can more easily catch it later:

try {
    baz();
    bar();
    foo(myInput);
} catch (BadUserInputException e) {
    reportError("Bad input: " + e.getMessage());
    log.info("recorded bad user input", e);
}

As for checkState, it doesn't really sound right to me. That exception usually implies that the problem was the state that this was in (or some other, more global state in the application). From the docs:

Signals that a method has been invoked at an illegal or inappropriate time.

In your case, a -1 is never appropriate, so checkState is misleading. Now, if it had been:

if (x == -1 && (!allowNegativeOne()) { ... }

...then that would be more appropriate, though it still has the drawback that IllegalArgumentException had above.

So, lastly, there's the question of whether you should just keep the if as it is, or use a helper method. That really comes down to taste, how complex the check is, and how often it's used (e.g. in other methods). If the check is as simple as x == -1 and that check isn't ever performed by other methods (so code reuse is not an issue), I would just keep the if.