Is throwing an exception an anti-pattern here?

Question

I just had a discussion over a design choice after a code review. I wonder what your opinions are.

There's this Preferences class, which is a bucket for key-value pairs. Null values are legal (that's important). We expect that certain values may not be saved yet, and we want to handle these cases automatically by initializing them with predefined default value when requested.

The discussed solution used following pattern (NOTE: this is not the actual code, obviously - it's simplified for illustrative purposes):

public class Preferences {
    // null values are legal
    private Map<String, String> valuesFromDatabase;

    private static Map<String, String> defaultValues;

    class KeyNotFoundException extends Exception {
    }

    public String getByKey(String key) {
        try {
            return getValueByKey(key);
        } catch (KeyNotFoundException e) {
            String defaultValue = defaultValues.get(key);
            valuesFromDatabase.put(key, defaultvalue);
            return defaultValue;
        }
    }

    private String getValueByKey(String key) throws KeyNotFoundException {
        if (valuesFromDatabase.containsKey(key)) {
            return valuesFromDatabase.get(key);
        } else {
            throw new KeyNotFoundException();
        }
    }
}

It was criticized as an anti-pattern - abusing exceptions to control the flow. KeyNotFoundException - brought to life for that one use case only - is never going to be seen out of the scope of this class.

It's essentially two methods playing fetch with it merely to communicate something to each other.

The key not being present in the database isn't something alarming, or exceptional - we expect this to occur whenever a new preference setting is added, hence the mechanism that gracefully initializes it with a default value if needed.

The counterargument was that getValueByKey - the private method - as defined right now has no natural way of informing the public method about both the value, and whether the key was there. (If it wasn't, it has to be added so that the value can be updated).

Returning null would be ambiguous, since null is a perfectly legal value, so there's no telling whether it meant that the key wasn't there, or if there was a null.

getValueByKey would have to return some sort of a Tuple<Boolean, String>, the bool set to true if the key is already there, so that we can distinguish between (true, null) and (false, null). (An out parameter could be used in C#, but that's Java).

Is this a nicer alternative? Yes, you'd have to define some single-use class to the effect of Tuple<Boolean, String>, but then we're getting rid of KeyNotFoundException, so that kind of balances out. We're also avoiding the overhead of handling an exception, although it's not significant in practical terms - there are no performance considerations to speak of, it's a client app and it's not like user preferences will be retrieved millions of times per second.

A variation of this approach could use Guava's Optional<String> (Guava is already used throughout the project) instead of some custom Tuple<Boolean, String>, and then we can differentiate between Optional.<String>absent() and "proper" null. It still feels hackish, though, for reasons that are easy to see - introducing two levels of "nullness" seem to abuse the concept that stood behind creating Optionals in the first place.

Another option would be to explicitly check whether the key exists (add a boolean containsKey(String key) method and call getValueByKey only if we have already asserted that it exists).

Finally, one could also inline the private method, but the actual getByKey is somewhat more complex than my code sample, thus inlining would make it look quite nasty.

I may be splitting hairs here, but I'm curious what you would bet on to be closest to best practice in this case. I didn't find an answer in Oracle's or Google's style guides.

Is using exceptions like in the code sample an anti-pattern, or is it acceptable given that alternatives aren't very clean, either? If it is, under what circumstances would it be fine? And vice versa?

Answer 1

Yes, your colleague is right: that is bad code. If an error can be handled locally, then it should be handled immediately. An exception should not be thrown and then handled immediately.

This is much cleaner then your version (the getValueByKey() method is removed) :

public String getByKey(String key) {
    if (valuesFromDatabase.containsKey(key)) {
        return valuesFromDatabase.get(key);
    } else {
        String defaultValue = defaultValues.get(key);
        valuesFromDatabase.put(key, defaultvalue);
        return defaultValue;
    }
}

An exception should be thrown only if you do not know how to resolve the error locally.

Answer 2

I wouldn't call this use of Exceptions an anti-pattern, just not the best solution to the problem of communicating a complex result.

The best solution (assuming you're still on Java 7) would be to use Guava's Optional; I disagree that it's use in this case would be hackish. It seems to me, based on Guava's extended explanation of Optional, that this is a perfect example of when to use it. You're differentiating between "no value found" and "value of null found".

Answer 3

Since there's no performance considerations and it's an implementation detail, it ultimately doesn't matter which solution you choose. But I have to agree it's bad style; the key being absent is something that you know will happen, and you don't even handle it more than one call up the stack, which is where exceptions are most useful.

The tuple approach is a bit hacky because the second field is meaningless when the boolean is false. Checking if the key exists beforehand is silly because the map is looking up the key twice. (Well, you're already doing that, so in this case thrice.) The Optional solution is a perfect fit for the problem. It might seem a bit ironic to store a null in an Optional, but if that's what the user wants to do, you can't say no.

As noted by Mike in the comments, there's an issue with this; neither Guava nor Java 8's Optional allow storing nulls. Thus you'd need to roll your own which -- while straightforward -- involves a fair amount of boilerplate, so might be overkill for something that will only be used once internally. You could also change the map's type to Map<String, Optional<String>>, but handling Optional<Optional<String>>s gets awkward.

A reasonable compromise might be to keep the exception, but acknowledge its role as an "alternative return". Create a new checked exception with the stack trace disabled, and throw a pre-created instance of it which you can keep in a static variable. This is cheap -- possibly as cheap as a local branch -- and you can't forget to handle it, so the lack of stack trace isn't an issue.

Answer 4

There's this Preferences class, which is a bucket for key-value pairs. Null values are legal (that's important). We expect that certain values may not be saved yet, and we want to handle these cases automatically by initializing them with predefined default value when requested.

The problem is exactly this. But you already posted the solution yourself:

A variation of this approach could use Guava's Optional (Guava is already used throughout the project) instead of some custom Tuple, and then we can differentiate between Optional.absent() and "proper" null. It still feels hackish, though, for reasons that are easy to see - introducing two levels of "nullness" seem to abuse the concept that stood behind creating Optionals in the first place.

However, don't use null or Optional. You can and should use Optional only. For your "hackish" feeling, just use it nested, so you end up with Optional<Optional<String>> which makes it explicit that there might a key in the database (first option layer) and that it might contain a predefined value (second option layer).

This approach is better than using exceptions and it is quite easy to understand as long as Optional is not new to you.

Also please note that Optional has some comfort functions, so that you don't have to do all the work yourself. These include:

• static static <T> Optional<T> fromNullable(T nullableReference) to convert your database input to the Optional types
• abstract T or(T defaultValue) to get the key value of the inner option layer or (if not present) get your default key value

Answer 5

I know I am late to the party, but anyways your use case resembles how Java's Properties lets one define a set of default properties too, which will be checked if there is no corresponding key loaded by the instance.

Looking at how the implementation is done for Properties.getProperty(String) (from Java 7):

Object oval = super.get(key);
String sval = (oval instanceof String) ? (String)oval : null;
return ((sval == null) && (defaults != null)) ? defaults.getProperty(key) : sval;

There really isn't a need to "abuse exceptions to control the flow", as you have quoted.

A similar, but slightly more terse, approach to @BЈовић's answer can also be:

public String getByKey(String key) {
    if (!valuesFromDatabase.containsKey(key)) {
        valuesFromDatabase.put(key, defaultValues.get(key));
    }
    return valuesFromDatabase.get(key);
}

Answer 6

Though I think @BЈовић's answer is fine in case getValueByKey is needed nowhere else, I don't think your solution is bad in case your program contains both use cases:

• retrieval by key with automatic creation in case the key does not exist beforehand, and

• retrieval without that automatism, without changing anything in the database, repository, or key map (think of getValueByKey beeing public, not private)

If this is your situation, and as long as the performance hit is acceptable, I think your proposed solution is fully ok. It has the advantage of avoiding duplication of the retrieval code, it does not rely on third party frameworks, and it is pretty simple (at least in my eyes).

In fact, in such a situation, in depends on the context if a missing key is an "exceptional" situation or not. For a context where it is, something like getValueByKey is needed. For a context where automatic key creation is expected, providing a method which reuses the already available function, swallows the exception and provides a different failure behaviour, makes perfectly sense. This can be interpreted as an extension or "decorator" of getValueByKey, not so much as a function where the "exception is abused for flow of control".

Of course, there is a third alternative: create a third, private method returning the Tuple<Boolean, String>, as you suggested, and reuse this method both in getValueByKey and getByKey. For a more complicated case that might be indeed the better alternative, but for such a simple case as shown here, this has IMHO a smell of overengineering, and I doubt the code gets really more maintainable that way. I am here with the topmost answer here by Karl Bielefeldt:

"you shouldn't feel bad about using exceptions when it simplifies your code".

Answer 7

Optional is the correct solution. However, if you prefer, there is an alternative which has less of a "two nulls" feel, consider a sentinel.

Define a private static string keyNotFoundSentinel, with a value new String("").*

Now the private method can return keyNotFoundSentinel rather than throw new KeyNotFoundException(). The public method can check for that value

String rval = getValueByKey(key);
if (rval == keyNotFoundSentinel) { // reference equality, not string.equals
    ... // generate default value
} else {
    return rval;
}

In reality, null is a special case of a sentinel. It is a sentinel value defined by the language, with a few special behaviors (i.e. well defined behavior if you call a method on null). It just happens to be so useful to have a sentinel like that that nearly ever language uses it.

* We intentionally use new String("") rather than merely "" to prevent Java from "interning" the string, which would give it the same reference as any other empty string. Because did this extra step, we are guaranteed that the String referred to by keyNotFoundSentinel is a unique instance, which we need to ensure it can never appear in the map itself.

Answer 8

Learn from the framework that learned from all Java's pain points:

.NET provides two far more elegant solutions to this problem, exemplified by:

• Dictionary<TKey, TValue>.TryGetValue(TKey, out TValue)
• Nullable<T>.GetValueOrDefault(T default)

The latter is very easy to write in Java, the former just requires a "strong reference" helper class.