Wednesday, July 13, 2005

Don't Code Like This!

WARNING: This blog entry was imported from my old blog on (which used different blogging software), so formatting and links may not be correct.

I was trying an open source web framework yesterday, and couldn't get it to work so I stepped through some of the code on the server with a debugger, and came across some code fragments that I thought I would submit to the Hall of Shame.

Besides, it's fun to blog about this, because each time I mention coding style I invariably get passionate responses from people defending their beloved style.

In the following, the class and method names have been changed to protect the guilty.

At first, I simply got a generic JspException. The code was doing this:

} catch (ConfigNotFoundException ex) {
throw new JspException(msg);

The original stack trace is not chained to the new exception! Please pass it in - JspException
has a constructor which takes both a message and the root cause exception, ex in the above.
If you're using some Exception class which is older and doesn't let you pass in the root cause,
such as DOMException,
then first create the exception, then call the
method on it, and finally throw the exception object.

The second code fragment I came across was this:

try {
return findConfig(servletContext).getResult(foo, servletContext);
} catch (NullPointerException ex) {
throw new ConfigNotFoundException("Can't find configuration");

The above code is using exceptions for flow control. Rather than first
calling findConfig, then checking if it is null, it just goes
ahead and tries calling it anyway, relying on the NullPointerException to
handle this case.

The code also has the bad effect of catching all unintended null pointer
exceptions in the subsequent getResult method call. These are
unrelated to the ability to find the configuration, so the error message would
be misleading. (And again the original exception is not chained).


  1. re style ... is it just me (being old school) or has including multiple return statements w/in a method become quite vogue? to me, chasing down scattered returns just feels ... well ... dirty. that and i think always having one include at the bottom of a method results in more normalized code, or so goes my hunch.

  2. Well I disagree with you regarding return statements - I've already written about those:

  3. ahhh, thx. i'm still waiting to be "enlightened" on this one :)

    is see there are 9 comments on that blog'o interest so i'll pop a cap from my favorite bottle'o beer and take it all in.

  4. k ... my take away post reading the "OnlyOneReturn Considered Harmful" thread ... balance is the key. guess i'm in a habit of returning only once (old habits die hard) but i can see for trivial modules where it can help. not sure i'll be retrofitting any code though to this new fangled coding style all the kidz r tawlk'n 'bout.

    rock on!