11

What is the best way to handle errors that shouldn't ever happen?

My current way to do this is to throw an exception if the 'thing that shouldn't happen' does happen, like so:

/*
 * Restoring from a saved state. This shouldn't be 
 * null unless someone in the future doesn't set it properly, in which 
 * case they will realize they did something wrong because it may crash.
 */
Object foo = bundle.getSerializable("foo");
if (foo != null) {
    doSomethingWith(foo);
} else {
    // This should never happen.
    if (BuildConfig.DEBUG) {
        throw new RuntimeException(
            "Foo is null!");
    }
    foo = new Object();
    doSomethingWith(foo);
}

This is somewhat pointless, because this should never happen. But just in case the code gets screwed up and is released into production with foo being null, should I leave these types of checks in the code?

gnat
  • 20,543
  • 29
  • 115
  • 306
elimirks
  • 275

5 Answers5

18

It's amazing how often things that should never happen do.

You should throw a RuntimeException with a descriptive error message and let the application crash. If that ever happens, then you know that something is deeply wrong.

Rory Hunter
  • 1,747
  • 9
  • 15
8

It is best to not handle an exception if you cannot handle it intelligently.

This gives the opportunity for something earlier in the stack to handle the exception, since it probably has more context of what is trying to be accomplished.

Alternatively, if you know that this should never happen, this is an assertion, and would probably benefit from having an assert statement.

Object foo = bundle.getSerializable("foo");

assert foo != null : "Foo was null"

doSomethingWith(foo);

To re-iterate my comment, if your application is in an indeterminate state (caused by a flow of logic that you did not intend) it is best to kill the application as soon as possible to prevent corruption or unintended results.

Matthew
  • 2,026
3

Why are you handling an error that never happens?

Computer programming deals with absolutes. There is no such thing as should, could and maybe. Either the error will happen or it will not. Adding error handling code for something that has a 0% chance of occurring is code smell. Good code handles that off change of 0.01% when it does happen.

A good way to think of it is like this.

If you only add error handling for 99% of the possible problems, then there only needs to be 100 problems for the source code to have an unhandled problem.

Overtime source code will grow to deal with thousands of problems...

Don't mix error handling with business logic

One problem I see all the time is source code that has been shotgun blasted with error handling. Especially in Java where exception handling is enforced by compiler error.

It makes it difficult to read what the intent of a function is when it branches many times to handle all the different error cases.

Instead, separate your error handling into specialized functions. Keep the original function focused on a single intent.

For example;

public Object getSerializableSafely(Bundle bundle, String name)
{
     Object foo = bundle.getSerializable(name);
     if (foo != null)
     {
        return foo;
     }
     else if (BuildConfig.DEBUG) {
        throw new RuntimeException("Foo is null!");
     }
     return null;
}

public boolean Worker()
{
    Object foo = getSerializableSafely(bundle, "foo");
    if (foo == null)
    {
        return false;
    }
    // do work with foo
    return true;
}

Now, there is nothing wrong with throwing a runtime exception in debug builds. They can be great time savers because they take the debugger right to where the problem is.

The problem is trying to create functions like Worker and expect them to never fail. Reporting true/false as the result of an operation is much better than wrapping everything in try/catch.

If you were using an XML parsing API. Would you want it to throw an exception if you feed it invalid XML or would you want the parse function to return false? Keep your exception handling related to things that are the exceptional case, and use return results to communicate the result of a function.

Clement Cherlin
  • 1,263
Reactgular
  • 13,120
  • 4
  • 50
  • 81
2

The one I always trip over is "Unsupported Encoding Exception" when I change a string to or from UTF-8 bytes. When is UTF-8 going to be Unsupported!?

Be careful, though... there are two levels of "should never happen." There is the level at which UTF-8 is unsupported. Then there is the "should never happen" when the developer stands looking over the shoulder of the tester, blinking and mumbling, "that should never happen."

  • What I usually do for the first level is roll that logic up in a separate class that does what you did in your example ... catch it and convert it to a RuntimeException. That way the nasty and irrelevant try ... catch block is hidden, and tested separately from, the rest of the code.

  • For the second case, a developer's "should never happen", those are really exceptions, in the proper meaning of the word "exception". But it's cleaner to treat them as guard conditions, because if that case happens, all bets are off and the code can't complete it's use case.

    Object foo = bundle.getSerializable("foo");

// Guard: This should never happen.
if (foo == null) {
    throw new RuntimeException("Foo is null!");
}

foo.doSomething();

  • The rule is "fail fast", so the moment when we detect that our assumptions are wrong, the code must halt. Note that a null pointer example is an example of "fail fast" because the code will halt as soon as you try to dereference the value, but it's one of those errors that's hard to pin down.

So in this case, instantiating an empty foo and driving on is a violation of "fail fast", so don't do that.

  • It's also fair to rely on client-server contract, so that every call you make doesn't have to be followed by a null check. As long as you're making internal calls (and not validating data that came from outside), you don't have to validate every assumption in the client-server contract. If a function states it will return a non-null value, then trust it. If it fails, fix the function, not the client.

  • For null pointer exceptions specifically, Guava has a really nice Optional class that wraps a type and makes sure the value is either non-null or else not present.

sea-rob
  • 6,911
0

Throwing an exception is expensive, and it can be rather shocking to the user based on the OS and platform. In addition, you have a blocking error here which is easy to check for, namely a nullvalue. Instead of throwing an exception, you should gracefully terminate, showing a message to the user and Closing() and Disposing() of any resources that need those actions.

Nzall
  • 1,416