1

Consider the two following examples:

public Something fetchSomething(String key) {
    if(somethingsMap.containsKey(key)) {
        return somethingsMap.get(key);
    }

    throw new IllegalStateException("key is missing");
}

versus

public Something fetchSomething(String key) {
    if(!somethingsMap.containsKey(key)) {
        throw new IllegalStateException("key is missing");
    }

    return somethingsMap.get(key);
}

Is there any general consensus which is considered "better"/more clean/preferred. Ending in normal flow or ending in exception flow. Or is this solely opinionated?

3
  • Perhaps this is meant to be a trivial example, but what happens if you get a key that doesn't exist in the map? Unless your exception adds semantic meaning that calling code can handle, just let somthingsMap throw the exception. Commented Apr 26 at 14:11
  • Especially if the end result either way makes the application crash because you cannot recover from the exception, rendering this question moot. Commented Apr 26 at 14:12
  • Bit sad that it got closed as opinion based when more answers were added that prove it's not. I checked the first answer that came in.
    – steros
    Commented Apr 28 at 6:40

5 Answers 5

4

Rephrased this question is make exception the de facto behaviour or make happy path the de facto behaviour the method implement.

This:

public Something fetchSomething(String key) {
    if(!somethingsMap.containsKey(key)) {
        throw new IllegalStateException("key is missing");
    }

    return somethingsMap.get(key);
}

is a fail-fast implementation.

In systems design, a fail-fast system is one that immediately reports at its interface any condition that is likely to indicate a failure. Fail-fast systems are usually designed to stop normal operation rather than attempt to continue a possibly flawed process. Such designs often check the system's state at several points in an operation, so that any failures can be detected early. The responsibility of a fail-fast module is detecting errors, and then letting the next-highest level of the system handle them.

from wikipedia.

If the question is just about the code style then it should be answered by the code style guide lines the team follows 'cause once decided after a while either way of writing the code is readable, clean, preferred.

3

if is supposed to do something when a condition is true, and else something else when it is false. Therefore, the general recommendation is that you should write

if(condition) {
    action
} else {
    alternative
}

rather than

if(!condition) {
    alternative
} else {
    action
}

It makes the code easier to understand, and in your case there's no other concerns that might interfere (such as one of the two blocks being much longer than the other).

4
  • Thank you for your answer. My question might not be clear enough. I'm not really asking about the way to write the if clause.
    – steros
    Commented Apr 26 at 6:02
  • I'm specifically asking about how to end the method. End with exception or end with return statement.
    – steros
    Commented Apr 26 at 6:03
  • But that isn't really the general recommendation. Early return is a fine thing to do and will make your code much clearer. (By amonst other things avoiding nesting, putting your result at the bottom rather than the middle, and more)
    – inaba
    Commented Apr 26 at 8:01
  • @inaba Early return is a fine thing in languages, like Java, that let you build something like a finally block to give you a single place to clean up resources. Not every language has that. Commented Apr 26 at 12:55
3

It really depends on your use-case. In general I would say that @Kilian Foth's answer is perfectly valid, but there might be cases where it would be more readable the other way around.

Consider this:

public Something fetchSomething(String key) {
    if (!isKeyFormatOk(key)) {
        throw new IllegalArgumentException("key is incorrectly formatted");
    }
    if (revokedKeys.contains(key)) {
        throw new IllegalArgumentException("key is revoked");
    }
    if (!somethingsMap.containsKey(key)) {
        throw new IllegalStateException("key is missing");
    }
    return somethingsMap.get(key);
}

The alternative would not be very pretty, and returning a correct error would be harder.

I try to stick to the rule that I first check the error cases, then I write my method knowing that my input is correct.

5
  • I also have it on my mind that I first should check for errors and then handle the normal flow. As such end a method with a return statement rather than an exception.
    – steros
    Commented Apr 26 at 6:19
  • But I searched a bit and couldn't find something to support this. Might have just been the standard proposed in books I read.
    – steros
    Commented Apr 26 at 6:20
  • 2
    The top answers of this question have valid arguments: softwareengineering.stackexchange.com/questions/18454/…
    – Mathieu
    Commented Apr 26 at 6:52
  • 1
    Thanks @Mathieu I might need to flag this as duplicates of that one.
    – steros
    Commented Apr 26 at 7:03
  • actually, could you add this as an answer rather than a comment. According to that question it seems opinionated, as the question was closed as such. So that would answer my question if there are no other answers with some kind of proof of an existing standard. And I would check your answer then. Thanks!
    – steros
    Commented Apr 26 at 7:42
2

Guard conditions are an exceptionally well know pattern, so well know that it's known outside of the "Programming Pattern's" world.

Guard conditions generally make your code easier to read as they allow one to skip to the happy path, while still acknowledging that there could be difficulties. And if there are difficulties, they make it easy to identify them and see what else should be guarded against.

If you put the check at the bottom, it's no longer a guard condition, it's now just another bit of cyclomatic complexity that you have to grind through to figure out what is happening.

While longer functions are discouraged, that doesn't mean they don't exist, and if you have 50-100 or even a 1000 lines of code before your "else throw", it's kind of hard to examine the condition without doing too much scrolling. Also, I would consider it bad practice, but there's not much to discourage someone from writing:

if (happypath)
{
}
else 
{
logError()
throw
}

DoMoreHappyPath()

I recommend that you put the guard condition at the top.

0

It just depends on your problem. Sometimes you make checks that determine there is no exception, sometimes your checks determine there is an exception.

Say you have a translator that can translate French, German and Italian into English. So you get

If French -> no exception
Else if German -> no exception 
Else if Italian -> no exception
Else -> throw exception

In other cases you check for things that can go wrong and each throws an exception.

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