I recently started using Sonar to automatically check the quality of the code I write – turns out it’s mostly ok with a few systematically bad points that I will work on improving. One of the problems I faced though was what to do about a barrage of false positives from a small number of the default rules that ship with Sonar. Here’s how I fixed the problem.
First off I’d like to say that I think going around switching off checks because you can’t be bothered to fix the code is a good idea, it’s much better to leave the violations in place and being reported so that one day you’ll get round to doing something about the problem. There are, however, certain situations where switching off a check is a valid thing to do and I’ll describe two situations below where I think it is valid:
The first concerns the PMD CloseResource check which checks to make sure you have closed database connections, result sets and statements. This is a very valuable check as anyone who has worked with databases knows – one of my early jobs was cleaning up an application that played very fast and loose with database resources and I never want to go back there. For those places where I have to interact with the database directly (normally I’m using JPA or some other data access layer) I’ve written myself a little utility class that takes a connection, result set and statement and ensures that they are closed as well as possible. This means that I don’t need to have the same boiler plate code everywhere I get a connection I just need to write: SQLUtilities.close( stmt, rs, conn );.
I find this utility method to be a great saving in time and removes the possibility of a typo causing resources to not get closed. The trouble is that PMD:CloseResource can’t be configured, as far as I know, to treat my utility method as a closer of resources so it’s being triggered all over the place as a major violation when in actual fact there isn’t really a problem. The solution is really actually very simple. PMD is able to understand Java 5 style annotations so you can turn off particular checks for particular classes and described here. In my case the annotation was just: @SuppressWarnings(“PMD.CloseResource”) at the top of the class. You can also turn off all PMD checks (a bad thing normally) and PMD understands regular Java suppression annotations.
My second violation suppression requirement turned out to be a bit harder to figure out due to Google turning up a lack of examples. I’ll state up front that I’m actually in two minds whether I want this particular rule at all since complying with it doesn’t really seem to improve the code much but it does mean quite a lot of going backwards and forwards while refactoring.The rule in question here is the Checkstyle DesignForExtensionCheck. Designing for extension is a good thing but to comply with this rule it’s generally necessary to make almost all of your public methods or classes final. This might be desirable if your releasing code that other third parties will rely on but when the code is only used in-house it feels a lot less desirable.
Normally I’d probably just quickly give up and make everything final to shut the rule up but I found myself with 700+ violations of this rule and in the position where I couldn’t make most of the classes final because they were CDI backing beans for JSF 2 pages. Backing beans like this can’t be made final because they have to be proxiable by the container, I suppose I could probably make the methods final but there was no way I was going through adding 700+ final modifiers – life’s too short.
The solution is to add a Checkstyle structured comment to the offending class to suppress a particular check. The reason this one was harder to figure out was because I couldn’t find a single example on line of how to suppress just one check in a class – every example showed how to suppress all Checkstyle checks in a class. The suppression comment (SuppressionCommentFilter) format required is like this:
//CHECKSTYLE:OFF DesignForExtensionCheck ... some code that can't be extended ... //CHECKSTYLE:ON
I put the comments around the whole class but it is possible to suppress just a single line. The format of the comments can be changed if they aren’t to your liking and there are two other suppression methods for Checkstyle as well if this isn’t enough – a configuration file (SuppressionFilter) and a single comment system (SuppressWithNearbyCommentFilter). I haven’t yet found myself needing to suppress two Checkstyle rules in a single class but presumably it is done by providing a comma separated list.