Should I use precondition checks to check intermediate results?

guava, java

Solution

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`.

Problem

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?

Original source