1

I saw lots of questions and explanations regarding empty catch clauses but I was never actually sure what "empty" means.

Is a catch clause like this empty? It clearly isn't empty for the eye because there is one line of code. But the catch is not really handled. Instead the error is just logged.

try {
    // do some work
} catch (SomeException e) {
    logger.error("Some error", e);
}

I see this fairly often because there are a lot exceptions where you just can't do something about it if it fails. The consequence is that the error is not fully swallowed but at least logged.

So the question is: Is this bad code if you "just" log the error and don't do anything about?

Joachim Sauer
  • 11,006
Playerwtf
  • 21
  • 4

3 Answers3

5

This is an empty catch:

 catch (SomeException e) {
 }

It is a very bad idea, because you're silently ignoring the exception. And even if that is the actual intention, then you should have at least a comment in there to tell future-you (or any other developer) why it should be empty.

Arguably, this one is still empty:

 catch (SomeException e) {
   // nothing to do because we frobnicated the foo before
 }

But at least this time we know that some thought went into it and the author thought that a do-nothing catch block is correct here.

Anything that adds logging or other exception handling is not "empty" in any meaningful way.

Whether or not some handling/logging code is sufficient at any given place, is an entirely different question: sometimes simply logging the exception is the right thing, at other times more drastic steps must be taken.

Joachim Sauer
  • 11,006
  • 2
    PSA: A frobnication must only be used in a Snafucated class, or it doesn't make sense. – Fabinout Jul 22 '13 at 09:16
  • it's a sign that something is wrong with the design (either the exception or the calling code) if you find yourself catching a checked exception and just adding comment that it should not happen. – jramoyo Jul 22 '13 at 09:18
  • 1
    @jramoyo just because it can't happen doesn't mean it won't :P – Casey Jul 22 '13 at 10:39
  • @jramoyo: I don't think I mentioned that case. I only mentioned that if you want to do nothing on catch, it should be commented. That's different from "I don't expect this ever to happen". And sadly there are cases where you have to have a catch block even though you know it can't throw (using "UTF-8" in the string constructor is a classic case: the JVM is required to support that, so it can't throw an UnsupportedEncodingException). – Joachim Sauer Jul 22 '13 at 10:42
  • @Casey: that's the point, so it's best to avoid ignoring exceptions, even with comments – jramoyo Jul 23 '13 at 03:37
  • @JoachimSauer: sorry, i'm not implying that you did. i'm just stating that there's likely a design flaw if you need to do that. from your example, it's probably better to have a String constructor which accepts an enum of charsets required to be supported by all JVMs. – jramoyo Jul 23 '13 at 03:54
  • @jramoyo: yes, but that was only added in Java 7! – Joachim Sauer Jul 23 '13 at 04:16
2

I guess it depends whether the exception is checked or is a runtime exception.

The idea behind checked exceptions (the ones you are required to "catch") is that they are errors which the application can recover from. So when caught, you should ideally provide a logic to recover from these exceptions. While you may rethrow them up the call heirarchy, it is best to handle them as quickly as possible (never let your clients do what you can do for your clients).

Runtime exceptions on the other hand, are normally not recoverable. They leave the application in a bad state. More often than not, the only way to handle these exceptions is to log as much information to aid your investigation. Typical applications would have "catch-all" block to handle all runtime exceptions.

jramoyo
  • 610
0

I think one would have to argue that it will not often be good code - its rather better than swallowing the error completely (or at least its better than doing so without clear explanation).

The basic problem is that in the same way that the code never throws, in my experience no-one will look at the logs unless there is an issue (or until the issue that its been hiding is uncovered by other means). I speak from practical experience as I have code that's not dissimilar to the above.

That said if you're catching and swallowing specific exception (as the code implies) and you explain why you're doing so then it may be perfectly reasonable.

What is not reasonable is a generic catch-all that does that - because then the you're getting into unforeseen consequences i.e. whilst it will swallow the trivial not critical error its there to deal with it will also swallow the show-stopper that's gently corrupting your data...

Murph
  • 7,813