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?

  • 1
    @GlenH7 the accepted (and highest voted) answer sums up that "you should use them [exceptions] in cases when they make error handling easier with less code clutter". I guess I'm asking here whether the alternatives that I listed should be considered "less code clutter". – Konrad Morawski Feb 12 '15 at 16:11
  • 1
    I retracted my VTC - you're asking for something related to, but different than the duplicate I suggested. – GlenH7 Feb 12 '15 at 16:17
  • 3
    I think the question becomes more interesting when getValueByKey is also public. – Doc Brown Feb 12 '15 at 17:40
  • 2
    For future reference, you did the right thing. This would have been off topic on Code Review because it's example code. – RubberDuck Feb 12 '15 at 21:04
  • 1
    Just to chime in --- this is perfectly idiomatic Python, and would (I think) be the preferred way to handle this problem in that language. Obviously, though, different languages have different conventions. – Patrick Collins Feb 12 '15 at 21:14

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.

  • 14
    Thanks for the answer. For the record, I'm this colleague (I didn't like the original code), I just phrased the question neutrally so that it wasn't loaded :) – Konrad Morawski Feb 12 '15 at 16:04
  • 59
    First sign of insanity: you start talking to yourself. – Robert Harvey Feb 12 '15 at 16:14
  • 1
    @BЈовић: well, in that case I think it is ok, but what if you need two variants - one method retrieving the value without autocreation of missing keys, and one with? What do you think is the cleanest (and DRY) alternative then? – Doc Brown Feb 12 '15 at 16:49
  • 3
    @RobertHarvey :) someone else authored this piece of code, an actual separate person, I was the reviewer – Konrad Morawski Feb 12 '15 at 17:13
  • 1
    @Alvaro, using exceptions frequently isn't a problem. Using exceptions badly is a problem, regardless of language. – kdbanman Feb 19 '15 at 16:29

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

  • 1
    I don't think Optional can hold null. Optional.of() only takes a not-null reference and Optional.fromNullable() treats null as "value not present". – Navin Feb 13 '15 at 7:43
  • 1
    @Navin it can't hold null, but you have Optional.absent() at your disposal. And so, Optional.fromNullable(string) will be equal to Optional.<String>absent() if string was null, or Optional.of(string) if it wasn't. – Konrad Morawski Feb 13 '15 at 10:25
  • @KonradMorawski I thought the problem in the OP was that you could not distinguish between a null string and a nonexistant string without throwing an exception. Optional.absent() covers one of these scenarios. How do you represent the other one? – Navin Feb 13 '15 at 10:38
  • @Navin with an actual null. – Konrad Morawski Feb 13 '15 at 11:33
  • 4
    @KonradMorawski: That sounds like a really bad idea. I can hardly think of a clearer way to say "this method never returns null!" than to declare it as returning Optional<...> . . . – ruakh Feb 15 '15 at 3:30

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.

  • 1
    Well, in reality it's only Map<String, String> on the database table level, because the values column has to be of one type. There's a wrapper DAO layer that strong-types each key-value pair, though. But I omitted this for clarity. (Of course you could have a one-row table with n columns instead, but then adding each new value requires updating the table schema, so removing the complexity associated with having to parse them comes at the cost of introducing another complexity elsewhere). – Konrad Morawski Feb 12 '15 at 17:07
  • Good point about the tuple. Indeed, what would (false, "foo") be supposed to mean? – Konrad Morawski Feb 12 '15 at 17:21
  • At least with Guava's Optional, trying to put a null in results in an exception. You'd have to use Optional.absent(). – Mike Partridge Feb 12 '15 at 18:03
  • @MikePartridge Good point. An ugly workaround would be to change the map to Map<String, Optional<String>> and then you'd end up with Optional<Optional<String>>. A less confusing solution would be to roll your own Optional that allows null, or a similar algebraic data type that disallows null but has 3 states - absent, null, or present. Both are simple to implement, though the amount of boilerplate involved is high for something that will only be used internally. – Doval Feb 12 '15 at 18:21
  • 2
    "Since there's no performance considerations and it's an implementation detail, it ultimately doesn't matter which solution you choose" - Except that if you were using C# using an exception will annoy anyone who is trying to debug with "Break when an exception is Thrown" is turned on for CLR exceptions. – Stephen Feb 13 '15 at 1:39

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
  • 1
    Optional<Optional<String>> looks evil, although it has some charm I have to admit : ) – Konrad Morawski Feb 12 '15 at 17:23
  • Can you explain further why it looks evil? It is actually the same pattern as List<List<String>>. You can of course (if the language allows) make a new type like DBConfigKey which wrapps the Optional<Optional<String>> and makes it more readable if you prefer that. – valenterry Feb 12 '15 at 18:30
  • 2
    @valenterry It's very different. A list of lists is a legitimate way of storing some kinds of data; an optional of an optional is an atypical way of indicating two kinds of problems the data could have. – Ypnypn Feb 12 '15 at 22:20
  • An option of an option is like a list of lists where each list has one or no elements. It's essentially the same. Just because it is not used often in Java does not mean it is inherently bad. – valenterry Feb 12 '15 at 22:39
  • 1
    @valenterry evil in the sense that Jon Skeet means; so not outright bad, but a bit wicked, "clever code". I guess it's just somewhat baroque, an optional of an optional. It's not explicitly clear which level of optionalness represents what. I'd have a double take if I encountered it in somebody's code. You might be right that it feels weird just because it's unusual, unidiomatic. Perhaps it's nothing fancy for a functional language programmer, with their monads and all. While I wouldn't go for it myself, I still +1'd your answer because it's interesting – Konrad Morawski Feb 13 '15 at 11:44

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);
}

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

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.

  • This, IMHO, is the best answer. – fgp Feb 14 '15 at 13:05

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

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

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

  • A covariance-friendly alternative to the Dictionary pattern would be T TryGetValue(TKey, out bool). – supercat Mar 16 '15 at 23:36

Your Answer

 

By clicking "Post Your Answer", you acknowledge that you have read our updated terms of service, privacy policy and cookie policy, and that your continued use of the website is subject to these policies.

Not the answer you're looking for? Browse other questions tagged or ask your own question.