Analyze This - Put your code on the couch!

I love it when tools find problems in my code rather than waiting for me or QA to find them. It helps my code to have less bugs, be more performant, more "hygienic" and I can usually fix the problems found faster than if it's later in the dev cycle.

So this blog entry is a plug for static code analyzers such as Checkstyle, PMD, FindBugs etc. and focuses on some of the issues that they spot in my own code and that of others.

Best of all - if you use these tools regularly, little by little you start writing code that will pass their checks without thinking about it - and *bam!* you're a better programmer!

Anyway, I use Checkstyle, PMD and FindBugs regularly- as each of them has slightly different coverage ranges for different issues (I've still to get around to checking out Hammurapi) and here I list some of the more helpful issues that each one spots for me.

Checkstyle

1. Missing Package Documentation - Just a basic reminder to document one's code. Nothing fancy - it's a good practice to be kind to other developers!
2. Method Parameter should be final - A good practice to ensure your contract / interface is explicit about what's an "in" parameter versus "in-out"
3. Spotting Magic Numbers - Such things should be either named constants or defined in some config / database somewhere. So easy to forget such "hard coding" beasts!
   

PMD

1. Excessive class length - as I've mentioned elsewhere, classes that are too long are probably guilty of trying to do too much and should be refactored to smaller classes that "do one thing and do it well"
2. Missing break in switch - one of those nasty ones where you can stare at the code for hours until it hits you that one of the lines doesn't have a break!
3. Empty Catch block - aaaaggghhhhh
4. Avoid catching Throwable - again, aaaaaggghhhhh - why do you want to catch Errors (e.g. out of memory problems) in addition to Exceptions?
5. Non-Thread-Safe Singleton - boy was that a great addition to PMD! Saved my ass more than once!
6. If all methods are static, consider using Singleton or a private constructor - just goodole code hygiene
7. Unused Imports - again just good hygiene
8. Avoid Protected Field in a final class - good point - the class is either extensible or not!
9. Insufficient StringBuffer Declaration - again just a nice, neat, small performance booster
10. Excessive method length - here a method is probably trying to do too much and should be refactored for maintainability
11. Switch Statements should have default - good point if only to log an error or throw an Assertion Error
12. Return from a finally block - finally blocks aren't intended for that!
    

FindBugs

1. Equals method assumes argument is correct type - the signature of equals is boolean Equals(Object o) so if you override it, it's always a good idea to check the instanceof right after checking for null
2. Inconsistent Synchronization on methods - Some objects are accessed by synchronized methods other's aren't exposing a multi-threading issue
3. Unconditional wait() - so your code doesn't mind waiting forever? I bet your users do!
4. Beware Tests for Floating Point Equality - you might want to consider Double or BigDecimal to avoid rounding errors
5. Switch Statement found where one case falls through to next - are you sure you mean that?

And that's only the ones that I stop me in my tracks! There's so many others that are still quite useful.

My day-to-day dev environment set-up is that I use the Checkstyle plug-in with Eclipse to check my code as I write it, and then before I check-in code, I run PMD & FindBugs using Ant (FYI the PMD plugin to Eclipse is just too slow) to generate a few reports to scan and fix some of the biggest problems.

These tools are great and well-worth the investment of time to integrate to your development process! Once integrated it takes almost no effort or time to run them. Oh and since they're open source they are free too! It's like pair programming in XP without the other person ;-)