Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

This question already has an answer here:

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?

share|improve this question

marked as duplicate by gnat, Bart van Ingen Schenau, Jimmy Hoffa, amon, GlenH7 Apr 29 at 15:42

This question has been asked before and already has an answer. If those answers do not fully address your question, please ask a new question.

1  
most languages have assert(false) which crashes the app when reached –  ratchet freak Apr 29 at 13:37
1  
@ratchetfreak at Java, relying on assert would be a bad idea, as simple invocation with -da (--disableassertions) would ignore all asserts –  gnat Apr 29 at 13:38
7  
If it should never happen, and you cannot handle it, you should let the application crash. It is best to let an application die instead of letting it continue in an undetermined state. –  Matthew Apr 29 at 13:38
    
@gnat I think assert false is handled specially –  ratchet freak Apr 29 at 13:39
1  
I'm inclined to think this will only generate debate and opinion, even logical criticism of each possibility doesn't change the fact that the criticisms on any answer to this question will be just as valid as each answer; there can be no authoritative answer here. It's like asking what language is best, you can make logical arguments to and fro but you cannot make an authoritative answer (unless your answer is Haskell) –  Jimmy Hoffa Apr 29 at 14:40

5 Answers 5

up vote 11 down vote accepted

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.

share|improve this answer
2  
Even better, an AssertionError, which is far less likely to be caught. –  Doval Apr 29 at 14:13
1  
Catching the thing that shall not be named is usually only useful if you like hiding and masking it with your soon-to-be-outdated error messages that are usually overly-general/friendly. Better to just let it occur and knock your system down with it's finger prints all over the mess so you can know it happened. –  Jimmy Hoffa Apr 29 at 14:35
2  
@JimmyHoffa ERROR: This comment can not be displayed. Unhandled exception. –  Mathew Foscarini Apr 29 at 14:54
1  
@MathewFoscarini that sounds like one of the friendly error messages people hide things in; the real error for such would say something about a connectivity or IO or some technical problem far beneath the covers - which is exactly what you'd want to get instead of a vague handwavy "I can't do that, Dave." error message. –  Jimmy Hoffa Apr 29 at 15:43

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.

share|improve this answer
    
the trouble with using an assert is it won't trigger in production, which for most of these things is exactly when the impossible thing happens –  jk. Apr 29 at 16:01

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.

share|improve this answer
    
You can avoid having to deal with the UnsupportedEncodingException by using the overloaded methods that take a Charset Object instead of a String to specify the encoding. And I actually once ran into an UnsupportedEncodingException for UTF-8. That was a long time ago, though, and it turned out the machine ran some beta release of the Java 5 JDK. –  Michael Borgwardt Apr 29 at 14:30

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 function getSerializableSafely(Bundle bundle, string name)
{
     Object foo = bundle.getSerializable(name);
     if(foo != null)
     {
        return foo;
     }
     if (BuildConfig.DEBUG) {
        throw new RuntimeException("Foo is null!");
     }
     return null;
}

public bool function 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.

share|improve this answer
    
As for "shoulds" and uncertainty, what about if the system doesn't have enough memory, or something else like that? Without explicit Preconditions like "Given that the system has..." all over, the best we can describe is what it should do in the "happy path". –  jordan May 21 at 22:47
    
@jordan That depends on the situation. If photoshop is opening a large compressed image, and doesn't know how big the uncompressed image will be, then it should catch out of memory exceptions. On the other hand. When an app starts up the developer should check the system requirements to ensure it meets the needs for the app (i.e. video games do this all the time). –  Mathew Foscarini May 22 at 10:29

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.

share|improve this answer
    
The exception is wrapped within a check for a debug build, so end users wouldn't see this exception. –  GlenH7 Apr 29 at 15:41

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