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?
0
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.
7
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.
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…