Through the eyes of sonar: Exception handling

Sonar is an open platform to manage code quality. Sonar is combining various java quality tools and collect these possible defects as violations. Let’s see some exception handling worst practices detected by these tools.


Avoid Print Stack Trace

Instead of :

catch (Exception e) {
  e.printStackTrace();
}

This exception will often get logged in a separate file (System.err) making debugging more complex than it needs.
Use a logger call instead.


catch (Exception e) {
  logger.warn("failed to ...." ,e);
}

Note the ,e this will tell log4j/slf4j/… to log the full stacktrace. Note that you can configure sfl4j to log the root cause first

Avoid logging only exception message

catch (Exception e) {
  logger.warn("failed to ...."+e);
}

Use the “,” variant of the logging method. In case of null pointer… the log will looks like :

 failed to ....null

This isn’t really helpful for debugging. So please use the other method :

catch (Exception e) {
  logger.warn("failed to ...." , e);
}

UseCorrectExceptionLogging and other pmd rules

Preserve Stack Trace

Intead of loosing track of the InstantiationException

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName());
 }

Preserve the stacktrace with a nested exception constructor :

 } catch (InstantiationException e) {
     throw new HibernateException("Could not instantiate resultclass: " + resultClass.getName() ,e);
 }

Avoid Catching Throwable

A catch statement should never catch throwable since it includes errors (OutOfMemoryError,…) :

 catch (Throwable t) {
  log.warn("unmonitored problem",t);
  success = false;
 }

So do this only in rare case (dictated by an api) where severe problem should catched and logged. If you are forced to do so ensure that the error is logged !

Prefer runtime exception, avoid signature that declare throws Exception

It is unclear which exceptions can be thrown from the methods. It might be difficult to document and understand the vague interfaces.

public Set getChildrenNames(Fqn arg0) throws Exception {

Use a class derived from RuntimeException. This also often force the user of your API to catch these exception and as you can saw it’s no easy to get right !
Checked exceptions I love you, but you have to go

A history of exceptions — Exceptions and how they should be handled always end in heated debates between Java developers. It isn’t surprising that Hibernate has some noteworthy history as well. Until Hibernate 3.x, all exceptions thrown by Hibernate were checked exceptions, so every Hibernate API forced the developer to catch and handle exceptions. This strategy was influenced by JDBC , which also throws only checked exceptions. However, it soon became clear that this doesn’t make sense, because all exceptions thrown by Hibernate are fatal. In many cases, the best a developer can do in this situation is to clean up, display an error message, and exit the application. Therefore, starting with Hibernate 3.x, all exceptions thrown by Hibernate are subtypes of the unchecked Runtime Exception, which is usually handled in a single location in an application. This also makes any Hibernate template or wrapper API obsolete.

Avoid Instanceof Checks In Catch Clause

Each caught exception type should be handled in its own catch clause. For example SQLException a subclass DataTruncation that contains more information and in separate catch you can log the parameter that exceeded the db length, but you will probably share a common method that will handle the rollback of the transaction.

Avoid Throwing Null Pointer Exception

NPE means programmer’s mistake and if you throw NPE you are not a nice person.
NPE may be a convention in the JDK, but perhaps it should stay there too.

if (flowName == null) {
   throw NullPointerException("FlowName cannot be null.");

Throwing NPE is really confusing, prefer IllegalArgumentException :

if (flowName == null) {
   throw IllegalArgumentException("FlowName cannot be null.");

A shorter version using spring assert :

import org.springframework.util.Assert;
Assert.notNull(flowName, "FlowName cannot be null.");

Check also commons-lang Validate or Guava Preconditions

Avoid Throwing Raw Exception Types

Rather than throw a raw RuntimeException, Throwable, Exception, or Error, use a subclassed exception or error instead.

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
      throw new RuntimeException("Bad Configuration for smtp.config");
    }

Prefer throwing specific exception like :

    if (StringUtils.isEmpty(props.getString("smtp.config"))) {
         throw new BadConfigurationException("smtp.config");
    }

Correctness – Exception created and dropped rather than thrown

Silly mistakes. We are human after all… luckily sonar is there to spot it !

This code creates an exception (or error) object, but doesn’t do anything with it. For example, something like

    if (x < 0)
      new IllegalArgumentException("x must be nonnegative");

It was probably the intent of the programmer to throw the created exception:

    if (x < 0)
      throw new IllegalArgumentException("x must be nonnegative");

Empty Try Block

The less you have code to maintain… the less work you have. just drop this kind of code

public void run() {
	try {
	} finally {
		try {
		} catch (Exception e) {
		}			
	}		
}

Bad practice – Method may fail to close stream on exception

Stream from java.io.File access,URLConnection,… should be closed whatever happens in a finally clause.
This will prevent also java.io.IOException: Too many open files

  File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    try {
	BufferedWriter bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
	bw.close();
    } catch (IOException e) {
	throw new RuntimeException(e);
    }

To fix use the org.apache.commons.io.IOUtils to finally close the writer

    File f = new File(parent.getAbsolutePath(),"TEST-"+getTestSuiteResult().getModuleName()+".xml");
    BufferedWriter bw = null;
    try {
	bw = new BufferedWriter(new FileWriter(f));
	bw.write("<!--?xml version=\"1.0\" encoding=\"UTF-8\" ?-->" + ENDL);
	bw.write(getTestSuiteOpenTag() + ENDL);
	for (UnitTestCaseResult tc : getTestSuiteResult().getTestCaseResults()) {
 	    bw.write(getTestCaseTag(tc) + ENDL);
	}
	bw.write(getTestSuiteEndTag() + ENDL);
	bw.flush();
    } catch (IOException e) {
	throw new UnitTestCaseGenerationException(e);
    } finally {
        IOUtils.closeQuietly(bw);
    }

If you are lucky to use jdk7 take a look at the The try-with-resources Statement.

Advertisements
  1. #1 by mestachs on October 24, 2012 - 5:14 pm

    Guava equivalent to IOUtils.closeQuietly :
    http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/io/Closeables.html#closeQuietly%28java.io.Closeable%29

    interesting thing is that it gets logged if necessary

  1. Through the eyes of sonar: Immutable Objects. « Don't Make the Same Mistake Twice
  2. Through the eyes of sonar : Complexity « Don't Make the Same Mistake Twice
  3. Through the eyes of sonar : Naming & Comments. « Don't Make the Same Mistake Twice
  4. Through the eyes of sonar : Collections. | Don't Make the Same Mistake Twice
  5. Through the eyes of sonar: recap. | Don't Make the Same Mistake Twice

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

%d bloggers like this: