I've been a zone leader with DZone since 2008, and I'm crazy about community. Every day I get to work with the best that JavaScript, HTML5, Android and iOS has to offer, creating apps that truly make at difference, as principal front-end architect at Avego. James is a DZone Zone Leader and has posted 639 posts at DZone. You can read more from them at their website. View Full User Profile

Coding Crimes: Ignoring Exceptions

06.14.2010
| 11091 views |
  • submit to reddit

Templates in IDEs such as Eclipse and NetBeans are a great feature. In particular, quick fix items such as "surround with try/catch" can save a lot of typing time. But, as with all things, the power given to the developer can be mis-used. This has got to be one of the worst coding crimes. 

Depending on your IDE and the templates you use, you'll probably get this: 

try 
{

} catch (Exception e)
{
// TODO: handle exception
}

Or, maybe you'll at least get the exception written out to console.

try 
{

} catch (IOException e)
{
// TODO Auto-generated catch block
e.printStackTrace();
}

For a start, the exception should be logged at the very least, not just written out to the console.  Also, in most cases, the exception should be thrown back to the caller for them to deal with. If it doesn't need to be thrown back to the caller, then the exception should be handled. And some comments would be nice too. 

The usual excuse for this type of code is "I didn't have time", but there is a ripple effect when code is left in this state. Chances are that most of this type of code will never get out in the final production. Code reviews or static analysis tools should catch this error pattern. But that's no excuse, all this does is add time to the maintainance and debugging of the software.  This also makes you question TODOs, and brings me back to my previous thoughts on writing the best code from the very start. 

Tags:

Comments

Wujek Srujek replied on Mon, 2010/06/14 - 9:38am

"For a start, the exception should be logged at the very least, not just written out to the console.  Also, in most cases, the exception should be thrown back to the caller for them to deal with."

Either log, or rethrow. Catching the exception just to log it doesn't make much sense. If you can gather some additional context, create a new exception with new info and wrap the cause, and throw this.

Also, if you log, and rethrow, and then the caller catches it, logs, rethrows, and so on (well, you do it, why others would not?) you get the stack trace with the same exception multiple times. Not nice. I have worked with a multitude of libraries that use this logging anti-pattern.

Sergey Lastname replied on Mon, 2010/06/14 - 9:40am

James, I must say at least in Netbeans 6.8 catch block generated section also contains call to JDK Logger.

Alessandro Santini replied on Mon, 2010/06/14 - 9:47am

Javalobby crimes: writing an article like this. Why not adding some real professional advice on how to propertly treat exceptions? Instead of easily pointing out what NOT to do, why not hinting at what TO DO?

Mladen Girazovski replied on Mon, 2010/06/14 - 10:25am

The generated catch block with the TODO comment is IMHO the best solution, no way in the world a template can generate proper exception handling, logging to the console (and swallowing the stacktrace) can be as bad as using crappy java.util.logging.

Sometimes, just ignoring exceptions is the way to go, as with checked exceptions that can never occure:

         try {
address = InetAddress.getLocalHost().getHostAdress();
} catch (final UnknownHostException ignore) {
// impossible to get here, checked execptions force the use of
// try/catch
}

Dushan Hanuska replied on Mon, 2010/06/14 - 11:26am

Templates are great time savers.

I agree with 'log or rethrow' strategy. Most typically I log exceptions so it would make sense to have the code generated for it. However to force myself to think and not blindly accept the generated code I deliberately introduce a compilation error to the code - notice the missing semicolon after the call to the logger:

try {
    ...
}
catch (Exception e) {
    log.error(e)
}

That way I either need to type a semicolon or do something else, whatever is more appropriate.

On the sidenote, remember not to introduce a FindBugs REC_CATCH_EXCEPTION bug - Exception is caught when Exception is not thrown.

This method uses a try-catch block that catches Exception objects, but Exception is not thrown within the try block, and RuntimeException is not explicitly caught. It is a common bug pattern to say try { ... } catch (Exception e) { something } as a shorthand for catching a number of types of exception each of whose catch blocks is identical, but this construct also accidentally catches RuntimeException as well, masking potential bugs.

 

Senthil Balakrishnan replied on Mon, 2010/06/14 - 11:36am

The usual excuse for this type of code is "I didn't have time"

General developer's perception, exception is something to be caught or thrown. But it's an ultimate clue to your problem, You have a choice to recover from it or to die with it based on the severity. If one starts thinking this  way, there wont be any excuse or regret.

Most interesting is the layer you are in, if you are in the inner most layer (dao) you would propagate nonsense and push the whole burden to the web handler to explain the user on the inability of the system which he was informed very little about.  

Eric Jablow replied on Mon, 2010/06/14 - 1:54pm

You ignored the best way to handle most exceptions: don't catch them. Simply allow them to bubble up the call stack until they reach a point where the program can handle the problem. If a low-level routine five levels deep in a service throws a SQLException, let the service method itself do the handling; it has the data needed to report the problem.

"Impossible" exceptions should be handled with assert false or throw new AssertionError().

When an exception reaches the boundary between two modules, and there still isn't enough context to handle the exception, the boundary class should translate the exception:

try {
  saveGame(board);
}
catch (SQLException se) {
  throw new ApplicationException("Could not save game position", se);
}

After all, the players of the game should not need to know that the game is being stored in a relational database at all.

Alex Santoro replied on Mon, 2010/06/14 - 3:21pm

When developers in my team claim an exception will never happen, I encourage them to put a logging message in the catch block stating that they will give me $100 if that line is ever found in a log file. Usually this is enough for them to rethink and at least log the exception. This way, when the underlying code changes and the exception does happen it will not be swallowed without leaving a trace.

 

Colin Brown replied on Mon, 2010/06/14 - 6:38pm

The *only* acceptable templated exception catch is this:
try {
   aCallThatUnfortunatelyThrowsACheckedException3();
} catch( Exception e ) {
   throw new ApplicationException(e);
}
Where ApplicationException is the root of your company's runtime exception hierarchy. Until java adds a @Rethrow( ApplicationException.class ) or the ilk, we must do the above.

This template also covers the 'this exception will never happen' case in the best possible way.

This template has an added bonus that, if you want to return a value from inside the try/catch you don't have to template a 'return null;' since this template breaks the program control (as it should). Logging is not an acceptable template because:

  • there is no added 'information' as to what was happening and/or expected and/or how to remedy the issue
  • when under load, what is effectively an anonymous log statement sucks
  • you effectively *hide* from the caller that their request was unfulfillable while failing the deterministic program test
  • Walter Bogaardt replied on Mon, 2010/06/14 - 10:04pm

    Well a crime is a bit extreme, but exceptions should be handled and at the very least logged in an error log. The most ideal obviously is someone reviews those error logs for runtime exceptions to keep the errors at a minimum.

    Mladen Girazovski replied on Tue, 2010/06/15 - 2:43am in response to: Eric Jablow

    Eric, the way you're handling the SqlException is breaking the boundary, the next module has to be aware of SQLExceptions, also called "leaky abstraction".

    Just imagine your Exception wasn't a SqlExxception but a JPA/Hibernate Exception, the other module would need those libs in the classpath, not much point in boundaries then.

    Exception translation needs to make sure that internal implementation details stay internal, especially when chaining Exceptions.

    Thor Åge Eldby replied on Tue, 2010/06/15 - 5:34am in response to: Mladen Girazovski

    @Mladen Girazovski
    If the checked exception can never occure, then why don't you just show your faith by doing a 'System.exit(0);' in the catch-block instead ;-D

    Surely, when bugs are made it is often because we didn't fully understand the API. In that aspect there is no such thing as 'cannot happen'. The practice you outline will lead to a later exception (in this case a NPE) which is more difficult to understand or even worse, no exception, often leading to loss of effect (i.e. data loss).

    If you can't deal with the checked exception wrap and rethrow it. In the few cases where that doesn't work (typically concurrent applications) at least log the error (and please read the logs).

    Mladen Girazovski replied on Tue, 2010/06/15 - 6:35am in response to: Thor Åge Eldby

    Thor,

    not sure if you read my post or the API ;)

    There is several cases where checked exception force us to use a try/ctach block, despite of the fact that they can never occure.

    There is a couple of popular examples of that fact, her is one, another i have already posted:

    ByteArrayOutputStream out = new ByteArrayOutputStream();

    try {   out.write(bytes);   out.close()   return out.toSring(); } catch (IOException e) {   // This can never happen!   // Should I rethrow? Eat it? Print Error? }

     I have no idea how you come to your conclusion regarding NPE.

    Thor Åge Eldby replied on Tue, 2010/06/15 - 6:42am in response to: Mladen Girazovski

    Eating the exception in your try-catch example above will most likely leave your 'address' variable with a null value. I simply assumed that you were going to use it afterwards.

    I think you should rethrow the IOException. What easily might happen here is that your colleague discover your method and thinks to himself: Hey, this is just the method I need. I'll only refactor out the stream to a parameter and generalise it to a InputStream. He'll probably not even notice your exception-eating.

    Rethrowing is almost always the best way to deal with checked exceptions in my view.

    Developers assuming to know that something can't happen is in my experience a great source of ridicule among testers.

    Mladen Girazovski replied on Tue, 2010/06/15 - 7:28am

    Thor, this IOException cannot occur, and that was the whole point: Checked Exceptions force the use of try/catch, even if it cannot be thrown.

    Same with the last Example, ByteArrayOutputStream cannot throw an IOException, no matter what. Check the implementation if you want to.

    If you think otherwise, please provide a concrete example.

    Thor Åge Eldby replied on Tue, 2010/06/15 - 8:10am in response to: Mladen Girazovski

    I haven't denied that these checked exceptions can't be thrown by these API functions. What I say is that you don't how your colleagues will expand or change this code.

    There is no static way to assert that methods in the try-block still can't throw exception after someone has changed the implementation. Therefore, in my opinion, it is wiser to rethrow.

    Mladen Girazovski replied on Tue, 2010/06/15 - 8:24am

    I agree that it is wiser to rethrow a checked exception, even if you are certain (or hope) that a specific error condition cannot occur, if it wasn't for well known Java SE API methods.

    With your own API or some 3rd party API, where the implementation could change over time, it certainly is not a good idea to swallow exceptions, it leads to frustrating debugging session, the Tiles Framework had such a quirk in its first version, but unfortunately it is not uncommon to find such a crime in Production Code.

    Findbugs and other tools can find these kind of problems(ignored exceptions), however, it is imho much harder to find improper exception handling, since exception handling should be part of design if not the architecture.

    Comment viewing options

    Select your preferred way to display the comments and click "Save settings" to activate your changes.