Don't catch and throw exceptions
catch (final Exception e) {
Logger.log("DatabaseSearch - createFeature - Exception" +
" whilst creating the xml file to return " + e.getMessage(),
Logger.ERROR);
throw new Exception(e.getMessage());
}
because I have turn PMD back on it told me I was being a fool, to create a new Exception and then throw it. I have to admit I am not sure of the best practise of exception handling because it is usually a bit of an after thought or because the way Java works, it makes you catch, throw an error so I usually do enough just to get the code to compile. I know this isn't right but I usually put down as something I will do later and then usually forget. I have seen a few blogs complaining about this and I think they have a point but on the other hand it does make you do something. I suppose the question is does it make you do something to catch the Exception or enough to stop the JVM complaining. I will leave that discussion for another time.
This is one of the main benefits of things like PMD, Checkstyle, fingbugs, it raises warnings on your code and then explains why. For someone like myself who is trying to write better code. It's really useful to have good coding standards shown to you using your own code and then you get a description on why the code is not to a standard and what rule it is breaking. Sometimes the rules are a bit over the top but even if I disagree it makes me think about my code more.
So back to the
"Caught Exception is rethrown, original stack trace may be lost"
and
"avoid throwing raw exceptions"
Now looking at this legacy code, okay I admit it I wrote it, I have to admit it does seem rather odd thing to do , catch it and then throw it with the same error message. I can see why it was done though because it has allowed the code to log out some useful information like class, method and a message saying "error whist creating xml file". The stack trace would show the information and if the exception was thrown then the error message which is probably the most important thing will still be the same. So after looking at this I think I should listen to PMD's advice and just throw the error rather than catching it logging it and throwing a new exception just like it.
Basically I think you should either catch or throw an exception, you don't need to do both. Pretty basic stuff I know but at least I know why now.
posted by The Hosk at
2:06 pm
8 Comments:
I admit that catching throwing an exception does sound like a silly thing to do. However, I can think of a situation where it would be appropriate. That is when you want to nest generated exceptions with a new exception that could be dealt with further up the code stack.
I've seen many cases where exceptions have been trapped and not dealt with properly. This can lead to problems being hidden and unexpected results.
By
Anonymous, at Wed Jun 14, 04:34:00 pm 2006
Well, creating a new Exception to throw—stripped of the stack trace—is the biggest mistake here. In this example there doesn't seem any reason not to have just used "throw e" which would have retained all of the original information about the exception.
Occasionally one catches an exception and wraps it in a new exception specifically to change the exception type. A common example is when the exception being caught is associated with implementation details that you don't want to expose.
Logging and rethrowing is a bit of a question mark because it's real easy to end up with a half-dozen levels of catch/log/rethrow repetitively logging the same exception. It's usually a better plan to log the exception only at the point that it is finally dealt with.
By
Doug, at Wed Jun 14, 05:18:00 pm 2006
I do things similar to this often. Usually it is because I want to log some data that won't be available wherever the exception is finally handled further up the stack (like queries or local variables). The primary difference when I do it is that I'll just throw on the original exception.
Like:
String qry = "SOME QUERY";
try {
//run query here
} catch (SQLException e) {
Log.log("Error running query: "+qry);
throw e;
}
By
Anonymous, at Wed Jun 14, 07:56:00 pm 2006
I do things similar to this often. Usually it is because I want to log some data that won't be available wherever the exception is finally handled further up the stack (like queries or local variables). The primary difference when I do it is that I'll just throw on the original exception.
Like:
String qry = "SOME QUERY";
try {
//run query here
} catch (SQLException e) {
Log.log("Error running query: "+qry);
throw e;
}
By
Anonymous, at Wed Jun 14, 07:56:00 pm 2006
PMD isn't complaining that the exception is being thrown, it's complaining that you've created a new Exception object when it would be better to just "throw e;".
(i.e. Doug has the right answer)
By
Anonymous, at Thu Jun 15, 08:07:00 am 2006
thanks for the comments Brian and Rob, you are indeed absolutly 100 percent correct.
it was indeed the creating of the new exception that was the mistake in the code and thanks for explaining that to me. Although I wonder if it is worth just throwing the exception rather than catching it, logging it and then throwing the same exception. That is probably a good idea to log it and throw it but probably the most of the time do I really need that extra information.
By
The Hosk, at Fri Jun 16, 10:25:00 pm 2006
Even if you need to catch it to change the exception type, you should still pass in the old exception as the cause rather than logging extra debug at that point :)
printing out different parts to the same error in different places of a log file just gets really confusing, especially if your one-line hint to why something realled failed is buried 2000 lines above where the stacktrace appears.
...
catch (Exception e)
{
throw new MyApplicationException("Error running database query - " + query, e);
}
then the stacktrace output to the log will have your debug data somewhere inside it, plus all the intermediate steps down to the real cause of the problem in a single place...
By
Anonymous, at Sat Jun 17, 09:49:00 am 2006
catch and throw happens quite often, if your application is layered. you catch the lower level exceptions like DatabaseExceptions and throw your own Exceptions (always nesting the lower one).
If you have to pass exceptions through multiple layers it is the only way to go to not having a dependency in your highest layer to your lower layers. Also the databaseexception can be ignored sometimes. the lower and intermediate levels shouldn't have to know about the higher level "sometimes" so every level catchs and throws if it doesn't know for sure.
catching java.lang.Exception is plain wrong at all in 99.9% of all cases. Cause you build up a one way handling path. Having a nice exception hierarchy for all your layers makes exception handling much more usefull and the pain is near nothing. Especially since all Exceptions arer Nestable since 1.4 and you do not need jakarta-commons-lang anymore.
By
Anonymous, at Mon Jun 19, 06:09:00 pm 2006
Post a Comment
<< Home