I came across the following code in WSIT today:
> private CallbackHandler configureServerHandler(Set configAssertions) {
> Properties props = new Properties();
> String ret = populateConfigProperties(configAssertions, props);
> try {
> if (ret != null) {
> Class handler = loadClass(ret);
> Object obj = handler.newInstance();
> if (!(obj instanceof CallbackHandler)) {
> throw new RuntimeException("The specified CallbackHandler class, " +
> ret + ", Is not a valid CallbackHandler");
> }
> return (CallbackHandler)obj;
> }
> return new DefaultCallbackHandler("server", props);
> } catch (Exception e) {
> throw new RuntimeException(e);
> }
> }
This is not a rare example. You can see a lot of those in many parts of
the code. I'm also not trying to blame the person who wrote the code. I
understand that everyone was under the JavaOne pressure. But
nevertheless it should be clear that code like this needs to be fixed.
I think there are a few problems with writing error handling code like this.
The first is "catch(Exception e)", which is considered by many to be a
bad practice. There are many reasons behind it, but one of the reasons
is that you should be making a conscious decision about how you handle a
particular kind of error. That would help you and maintenance people to
understand the code better. You should also consider handling errors
differently depending on the error.
The second bad thing about code like this is that it throws a bare
"RuntimeException", without any additional explanation. One of the
reasons of writing good error handling code is to make it easy to
diagnose problems, when they occur. A good error message can easily save
hours of frustration from users and our field engineers. It also save
YOUR time, because those people won't have to bother you to get the
problem fixed. So a particular attention should be paid to report a good
error message that explains the problem.
To this end, when you wrap an exception like this, (1) try to use the
right type, at least WebServiceException for cases like this, and (2)
try to put some human readable message in the outer exception so that
the user can put the ultimate cause of the problem into the context.
This is also related to why "catch(Exception)" is bad --- it makes it
hard to come up with a meaningful explanation.
For example, the reason I am looking at this code is because Glassfish
fails to deploy my web application with "java.lang.RuntimeException:
System Property WSIT_HOME not set". This is wrapped into two more
RuntimeException classes but without any additional message. I have no
idea which part of the system is refering to WSIT_HOME, and why it's
need in which part of WSIT. It just doesn't give me any clue. So this is
not a good error message.
A good exception would say something like...
WebServiceException: Failed to parse
"/home/kk122374/ws/wsit/foobar/server-security-env.properties"
at com.sun.xml...
at com.sun.xml...
Caused by: WebServiceException: Unable to load keystore
"$WSIT_HOME/somewhere/some.keystore"
at com.sun.xml...
at com.sun.xml...
Caused by: WebServiceException: System Property WSIT_HOME not set
at com.sun.xml...
at com.sun.xml...
I hope you see what I mean by "putting the ultimate cause into context".
Lastly, it's time to take i18n seriously. Fabian and I even proposed a
way to do i18n painlessly.
To conclude, our FCS-quality products deserve better error handling. I
hope I explained a few tips to get us closer to the goal.
--
Kohsuke Kawaguchi
Sun Microsystems kohsuke.kawaguchi_at_sun.com