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?

steros
  • 121

5 Answers5

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

Kilian Foth
  • 110,899
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.

Mathieu
  • 1,039
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.

jmoreno
  • 11,238
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.

gnasher729
  • 49,096