Saturday, October 27, 2007

Checkstyle vs Nonsense code

Recently we have started to use checkstyle to verify our code. Although first intention of checkstyle is to check agreed code conventions, checkstyle is able to check for some known code structures that often mean wrong design of code. This is useful because you can catch common mistakes very early and therefore avoid bugs later.

Among others, one of our checks is that there should not be empty block of code, like catch block.
And then we had our first rule violation. For testing of course we are using JUnit. Standard design of JUnit 3.8 test to verify that certain part of code throws exception looks like this:

try {
performExceptionThrowingOperation(...)
fail();
} catch(ExpectedException excepiton) {
// expected exception
}


Well as you can see catch block is empty, there is no java code in it. And of course checkstyle complained about this.
I see two options here:
  • leave code as it is and do not fix checkstyle violation
  • add nonsense line of code like assertTrue(true)
At the end I have selected third option :), change checkstyle rule so it accepts in the block also plain text e.g. comment (not only java code) and therefore checkstyle violation disappeared.

But now I am getting to the point of my thoughts. As code grows, there will be more and more places where maybe it will be strange (e.g. adding assertTrue(true)) to fix checkstyle violation and not possible to solve it with simple additional configuration of checkstyle rule.

Question is: should we leave checkstyle violation as it is or add strange code of assertTrue(true) style?

My opinion is that we should add strange code and fix checkstyle violation. If we start to leave checkstyle violations open, number of opened checkstyle violations will grow with time and checkstyle violations warnings will loose their intention warning about bad code design. Because if there is long existing list of false checkstyle warnings we may miss the new, but important one. And then, instead of fixing the bug immediately, maybe we will fix it in later testing phases. From my point of view, it is better to have some strange but harmless code lines than to miss one important bug because of long list of existing checkstyle warnings.

If somebody will ever read this :) it would be nice to hear your opinion.

1 comment:

Anonymous said...

exception = null; works for me.