Exception-Handling Antipatterns Blog Re-post

This is a repost of an article I came across describing Exception-Handling antipatterns:

Log and Throw

Example:

catch (NoSuchMethodException e) { LOG.error("Blah", e); throw e; }

or

catch (NoSuchMethodException e) { LOG.error("Blah", e); throw new MyServiceException("Blah", e); }

or

catch (NoSuchMethodException e) { e.printStackTrace(); throw new MyServiceException("Blah", e); }

All of the above examples are equally wrong. This is one of the most annoying error-handling antipatterns. Either log the exception, or throw it, but never do both. Logging and throwing results in multiple log messages for a single problem in the code, and makes life hell for the support engineer who is trying to dig through the logs.

Throwing Exception

Example:

public void foo() throws Exception {

This is just sloppy, and it completely defeats the purpose of using a checked exception. It tells your callers “something can go wrong in my method.” Real useful. Don’t do this. Declare the specific checked exceptions that your method can throw. If there are several, you should probably wrap them in your own exception (see “Throwing the Kitchen Sink” below.)

Throwing the Kitchen Sink

Example:

public void foo() throws MyException, AnotherException, SomeOtherException, YetAnotherException {

Throwing multiple checked exceptions from your method is fine, as long as there are different possible courses of action that the caller may want to take, depending on which exception was thrown. If you have multiple checked exceptions that basically mean the same thing to the caller, wrap them in a single checked exception.

Catching Exception

Example:

try { foo(); } catch (Exception e) { LOG.error("Foo failed", e); }

This is generally wrong and sloppy. Catch the specific exceptions that can be thrown. The problem with catchingException is that if the method you are calling later adds a new checked exception to its method signature, the developer’s intent is that you should handle the specific new exception. If your code just catches Exception (or worse, Throwable), you’ll probably never know about the change and the fact that your code is now wrong.

Destructive Wrapping

Example:

catch (NoSuchMethodException e) { throw new MyServiceException("Blah: " + e.getMessage()); }

This destroys the stack trace of the original exception, and is always wrong.

Log and Return Null

Example:

catch (NoSuchMethodException e) { LOG.error("Blah", e); return null; }

or

catch (NoSuchMethodException e) { e.printStackTrace(); return null; } // Man I hate this one

Although not always incorrect, this is usually wrong. Instead of returning null, throw the exception, and let the caller deal with it. You should only return null in a normal (non-exceptional) use case (e.g., “This method returns null if the search string was not found.”).

Catch and Ignore

Example:

catch (NoSuchMethodException e) { return null; }

This one is insidious. Not only does it return nullinstead of handling or re-throwing the exception, it totally swallows the exception, losing the information forever.

Throw from Within Finally

Example:

try { blah(); } finally { cleanUp(); }

This is fine, as long as cleanUp() can never throw an exception. In the above example, if blah() throws an exception, and then in the finally block,cleanUp() throws an exception, that second exception will be thrown and the first exception will be lost forever. If the code that you call in a finally block can possibly throw an exception, make sure that you either handle it, or log it. Never let it bubble out of the finally block.

Multi-Line Log Messages

Example:

LOG.debug("Using cache policy A"); LOG.debug("Using retry policy B");

Always try to group together all log messages, regardless of the level, into as few calls as possible. So in the example above, the correct code would look like:

LOG.debug("Using cache policy A, using retry policy B");

Using a multi-line log message with multiple calls tolog.debug() may look fine in your test case, but when it shows up in the log file of an app server with 500 threads running in parallel, all spewing information to the same log file, your two log messages may end up spaced out 1000 lines apart in the log file, even though they occur on subsequent lines in your code.

Unsupported Operation Returning Null

Example:

public String foo() { // Not supported in this implementation. return null; }

When you’re implementing an abstract base class, and you’re just providing hooks for subclasses to optionally override, this is fine. However, if this is not the case, you should throw an UnsupportedOperationException instead of returning null. This makes it much more obvious to the caller why things aren’t working, instead of her having to figure out why her code is throwing some random NullPointerException.

IgnoringInterruptedException

Example:

while (true) { try { Thread.sleep(100000); } catch (InterruptedException e) {} doSomethingCool(); }

InterruptedException is a clue to your code that it should stop whatever it’s doing. Some common use cases for a thread getting interrupted are the active transaction timing out, or a thread pool getting shut down. Instead of ignoring theInterruptedException, your code should do its best to finish up what it’s doing, and finish the current thread of execution. So to correct the example above:

while (true) { try { Thread.sleep(100000); } catch (InterruptedException e) { break; } doSomethingCool(); }

Relying on getCause()

Example:

catch (MyException e) { if (e.getCause() instanceof FooException) { ...

The problem with relying on the result of getCauseis that it makes your code fragile. It may work fine today, but what happens when the code that you’re calling into, or the code that it relies on, changes its underlying implementation, and ends up wrapping the ultimate cause inside of another exception? Now calling getCause may return you a wrapping exception, and what you really want is the result ofgetCause().getCause(). Instead, you should unwrap the causes until you find the ultimate cause of the problem. Apache’s commons-lang project provides ExceptionUtils.getRootCause()to do this easily.

Source: Exception-Handling Antipatterns Blog | Oracle Community

Leave a Reply

Your email address will not be published. Required fields are marked *