Reputation: 1670
Ok I know catching throwable is not a good idea:
try {
// Some code
} catch(Throwable e) { // Not cool!
// handle the exception
}
But recently I was reading through an open sourced code and I saw this interesting (at least to me) piece of code:
try {
// Some Code
} catch (Throwable ex){
response = handleException(ex, resource);
}
private handleException(Throwable t, String resource) {
if (t instanceof SQLEXception) {
// Some code
} else if (t instanceof IllegalArgumentException) {
//some code
} //so on and so forth
}
This doesn't seem to be that bad? What is wrong with this approach?
Upvotes: 25
Views: 15435
Reputation: 417
You can always catch different type of exceptions and perform some operations based on the type of the exception you got.
Here is an example
try{
//do something that could throw an exception
}catch (ConnectException e) {
//do something related to connection
} catch (InvalidAttributeValueException e) {
// do anything related to invalid attribute exception
} catch (NullPointerException e) {
// do something if a null if obtained
}
catch (Exception e) {
// any other exception that is not handled can be catch here, handle it here
}
finally{
//perform the final operatin like closing the connections etc.
}
Upvotes: 0
Reputation: 15212
You are right when you say that catching Throwable
is not a good idea. However, the code that you present in your question is not catching Throwable
in an evil way but let's talk about that later. For now, the code that you present in your question has several advantages :
1. Readability
If you look at the code carefully, you will notice that even though the catch block is catching a Throwable
, the handleException
method is checking the type of exception thrown and possibly taking different actions based on the exception type.
The code presented in your question is synonymous to saying:
try {
doSomething();
} catch (SQLEXception ex){
response = handleException(resource);
} catch(IllegalArgumentException ex) {
response = handleException(resource);
} catch(Throwable ex) {
response = handleException(resource);
}
Even if you have to catch 10+ exceptions only, this code can easily take up a lot of lines of code and the multi-catch construct is not going to make the code any cleaner. The code that you present in your question is simply delegating the catch
to another method to make the actual method that does the work more readable.
2. Reusability
The code for the handleRequest method could easily be modified and placed in a utility class and accessed throughout your application to handle both Exception
s and Error
s. You could even extract the method into two private
methods; One that handles Exception
and one that handles Error
and have the handleException
method that takes a Throwable
further delegate the calls to these methods.
3. Maintainibility
If you decide that you want to change the way you log an SQLException
s in your application, you have to make this change in a single place rather than visiting every method in every class that throws an SQLException
.
So is catching Throwable
a bad idea?
The code that you present in your question is not really the same as catching Throwable
alone. The following piece of code is a big no-no:
try {
doSomething();
} catch(Throwable e) {
//log, rethrow or take some action
}
You should catch Throwable
or Exception
as far away in the catch
chain as possible.
Last but not the least, remember that the code you present in your question is framework's code and there are certain errors that the framework can still recover from. See When to catch java.lang.Error for a better explanation.
Upvotes: 19
Reputation: 65793
Just to provide balance - there is one place where I will always catch (Throwable)
:
public static void main(String args[]) {
try {
new Test().test();
} catch (Throwable t) {
t.printStackTrace(System.err);
}
}
At least something shows somewhere that something went wrong.
Upvotes: 2
Reputation: 45654
There are exactly two valid uses for using a huge net:
If you will handle everything uniformly, like a top-level catch for logging/reporting, possibly followed by an immediate exit.
To reduce duplication, by exporting all the handling into its own method.
Catch the most derived common ancestor there is to avoid extra-work and increase clarity.
DRY is an important design principle.
In both cases, unless you expected that exception and handled it completely, rethrow.
Upvotes: 3
Reputation: 3307
You posted a link to Jongo, which demonstrates one possible use for this technique: re-using error handling code.
Let's say you've got a large block of error handling code that naturally repeats itself in various places in your code - for example Jongo produces standard responses for some standard classes of errors. It may be a good idea to extract that error handling code into a method, so you can re-use it from all the places it's needed.
However, that's not to say that there's nothing wrong with Jongo's code.
Catching Throwable
(rather than using multicatch) is still suspicious, as you're likely to catch Error
s that you're not really in a position to handle (are you sure you meant to catch ThreadDeath
?). In this situation, if you absolutely have to catch Throwable
, it'd be better to "catch and release" (i.e, rethrow anything that you didn't mean to catch). Jongo doesn't do this.
Upvotes: 3
Reputation: 49724
Catching Throwable
s out of laziness is a bad idea.
This was particularly tempting before try-multi-catch
was introduced.
try {
...
} catch (SomeException e) {
//do something
} catch (OtherException e) {
//do the same thing
} ...
Repeating catch blocks is tedious and verbose, so some people decided to just catch Exception
or Throwable
and be done with it. This is what should be avoided because:
Throwable
in the catch block. (And we've all seen code that does that...:))But catching Throwable
s when it is absolutely necessary is fine.
When is it necessary? Very rarely. In framework-style code there are various scenarios (dynamically loading an external class is the most obvious one), in a standalone application a typical example is to attempt to display/log some kind of error message before exiting. (Bearing in mind that the attempt may fail, so you don't want to put anything critical there.)
As a rule of thumb, if there's nothing you can do about an exception/error, you shouldn't catch it at all.
Upvotes: 9
Reputation: 743
Java 7 solves a bit of the tedium that is multi-catching of similar exceptions with similar handling. You definitely should not be doing what the person has done here. Just catch the appropriate exceptions as needed, it may look ugly but then that's what throws
is for, pass it to the method that should catch it and you shouldn't be wasting too much code space.
Check out this link for more information.
Upvotes: 2
Reputation: 271
First of all, catching Throwable makes your application rather intransparent. You should be as explicit as possible on catching exceptions to enable good traceability in exceptional cases.
Let's have a look at the method handleException(...) and see some of the problems that occur by this approach:
From my point of view, catch-blocks are exactly made for the functionality that are tried to cover in handleExceptions(...), so use them.
Upvotes: 2
Reputation: 10652
There are various reasons why you should not catch a Throwable. First of all is, that Throwable
includes Error
s - and there's normally not much an application can do if one of these appears. Also Throwable reduces your chances of finding out, WHAT has happened. All you get is "something bad has happened" - which might be a catastrophe or just a nuisance.
The other aproach is better but of course I still would not catch Throwable, but try to catch more specific Exceptions, if possible at all. Otherwise you are catching everything and then try to sort out which kind of bad thing happened. Your example could be written as...
try {
...
} catch (SQLEXception ex){
response = ... ;
} catch (IllegalArgumentException ex){
response = ...;
}
...which would reduce the amount of if ( ... instanceof ... )
blocks (which are only needed because the author first decided to catch everything in one big bucket). It something actually throws Throwable
, then you don't have much choice, of course.
Upvotes: 21