Going through the statistics of rules’ usage on Techdebt.org I was surprised to find critical PMD rules that seem to be disregarded. Here is a presentation of 5 of them, with an explanation on what they check. Those rules all have two things in common:

• Their non-respect can bring huge problems into your application ;
• Their usage statistic is surprisingly low.

For each rule you will also find its usage statistics (the percentage of project where the rule is used, according to Techdebt.org) and the PMD’s ruleset where you can find the rule. You may not learn anything new, but don’t forget to check your quality profile to make sure you use them

ReturnFromFinallyBlock

This rule finds return in finally blocks. This is definitely a critical mistake since it can discard exceptions. If an exception, checked or unchecked, is thrown in the try block, a return in the finally block will discard it.

Example : the following code only prints “Finally”.

   public static void main(final String[] args) {
        try {
            foo();
        }
        catch (final Exception e) {
            System.out.println("Catch");
        }
    }
    public static int foo() throws Exception {  
        try {
            // some clever code that throws an Exception
            throw new Exception();
        }
        finally {
            System.out.println("Finally");
            return -1;
        }
    }

DoNotThrowExceptionInFinally

Throwing an exception in a finally block can be really confusing and makes the code hard to debug. It will also discard any previous exception raised. For example, if an unexpected exception, like a NullPointerException, is thrown in the try block, it will be discarded. This can hide bugs, resulting in painful debugging tasks…

Example : the following code prints “Gotcha!” when you would like it to crash with a NPE.

    public static void main(final String[] args) {  
        try {
            foo();
        }
        catch (final IOException e) {
            System.out.println("Gotcha!");
        }
    }
    public static void foo() throws IOException {
        try {
            // some clever code...
            // unexpected exception
            throw new NullPointerException();
        }
        finally {
            // do something
            // throw IOException, the NullPointerException is discarded.
            throw new IOException();
        }
    } 

DontCallThreadRun

The run() method is executed in the current thread. To create a new thread, the method start() must be used, which is usually the intended behavior. This rule is also present in Findbugs with the name “RU_INVOKE_RUN”. One of this rule should be activated.

Example : this code illustrate the behavior of thread.run() and thread.start().

    public static void main(final String[] args) {
        System.out.println("Main thread: " + Thread.currentThread().getId());
        final FooThread thread = new FooThread();
        thread.run();
        thread.start();
    }
    public static class FooThread extends Thread
    {
        @Override
        public void run() {
            System.out.println("I'm executing from thread " + Thread.currentThread().getId());
            super.run();
        }
    }

AvoidStringBufferField

The use of a StringBuffer as a class’ field should be avoided. A StringBuffer can use a lot of memory. If the class has a long time life, this can lead to memory overflow. As long as it is possible, prefer to use local variables.

Example : don’t do that

   public class ExampleClass
   {
       private StringBuffer output;

FinalizeShouldBeProtected

Finalize() is called by the garbage collector when an object is collected. You should not rely on finalize to perform tasks other than freeing resources. Finalize is not meant to be called by anyone other than the garbage collector. For this reason, finalize() should not be public but protected. This rule is also in Findbugs, with the name “FI_PUBLIC_SHOULD_BE_PROTECTED”. At least one of this rule should be used.

Example : again, don’t do that

    @Override
    public void finalize() throws Throwable {
        ...
    }

That’s the end of this article, as we have seen, violating these rules can bring some trouble. I was surprised to see that they are rarely used, so tell me : did you know about these rules? Do you think they’re worth checking?