Bear up against static code analysis

One of the first things we do when we join a team on a new (or existing) project is to set up a whole barrage of static code analysis tools, like Findbugs, Checkstyle or PMD for java (or any other for virtually every language around). Most of these tools spit out tremendous amounts of numbers and violated rules, totally overwhelming the team. But the amount of violations, (nearly) regardless how high it might be, is not the problem. It’s the trend of the violation curve that shows the problem and its solution. If 2000 findbugs violations didn’t kill your project yet, they most likely won’t do it in the future, too. But if for every week of development there are another 50 violations added to the codebase, it will become a major problem, sooner or later.

Visibility is key

So the first step is always to gain visibility, no matter how painful the numbers are. After the initial shock, most teams accept the challenge and begin to resolve issues in their codebase as soon as they appear and slowly decrease the violation count by spending extra minutes with fixing old code. This is the most valuable phase of static code analysis tools: It enables developers to learn from their mistakes (or goofs) without being embarrassed by a colleague. The analysis tool acts like a very strict and nit-picking code review partner, revealing every flaw in the code. A developer that embraces the changes implied by static analysis tools will greatly accelerate his learning.

But then, after the euphoric initial challenges that improve the code without much hassle, there are some violations that seem hard, if not impossible to solve. The developer already sought out his journey to master the tool, he cannot turn around and just leave these violations in the code. Surely, the tool has flaws itself! The analysis brought up a false positive here! This isn’t faulty code at all, it’s just an overly pedantic algorithm without taste for style that doesn’t see the whole picture! Come to think about it, we have to turn off this rule!

Leave your comfort zone

When this stage is reached, the developers have a deep look into the tool’s configuration and adjust every nut and bolt to their immediate skill level. There’s nothing wrong with this approach if you want to stay on your skill level. But you’ll miss a chance to greatly improve your coding skills by allowing the ruleset to be harder than you can cope with now. Over time, you will come up with solutions you now thought are impossible. It’s like fitness training for your coding skills, you should raise the bar every now and then. Unlike fitness training, nobody gets hurt if the numbers of your code analysis show more violations than you can fix up right now. The violations are in the code, if you let them count or not.

Once, a fellow developer complained really loud about a specific rule in a code analysis tool. He was convinced that the rule was pointless and should be switched off. I asked about a specific example where this rule was violated in his code. When reviewing the code, I thought that applying the rule would improve the code’s internal structure (it was a rule dealing with collapsible conditional statements). In the discussion on how to implement the code block without violating the rule, the real problem showed up – my colleague couldn’t think about a solution to the challenge. So we proceeded to implement the code block in a dozen variations, each without breaking the rule. After the initial few attempts that I had to lead program for him, he suddenly came up with even more solutions. It was as if a switch snapped in his head, from “I’m unable to resolve this stupid rule” to “Hey, if we do it this way, we even can get rid of this local variable”.

Embrace challenges

Don’t trick yourself into thinking that just because your analysis tool doesn’t bring up these esoteric violations anymore after you switched off the rules, they are gone. They are still in your code, just hidden and without your awareness. Bear up against your analysis tool and fix every violation it brings you, one after the other. The tools aren’t there to annoy you, they want to help you stay clear of trouble by pointing out the flaws in a clear and precise manner. Once you meet the challenges the tool presents you with, your skill level will increase automatically. And as a side effect, your code becomes cleaner.

Beyond clean code

Even if every analysis tool approves your code as being clean, it can still be improved. You might have a look at Object Calisthenics or similar code training rulesets. They work the same way as the analysis tools, but without the automatic enforcement (yet). The goal is always cleaner code and higher skilled developers.

FindBugs-driven bughunting in legacy projects

I have been working on a >100k lines legacy project for a while now. We have to juggle customer requests, bug fixes and refactoring so it is hard to improve the quality and employ new techniques or tools while keeping the software running and the clients happy. Initially there were no unit tests and most of the code had a gigantic cyclomatic complexity. Over the course of time we managed to put the system under continuous integration, employed quite some unit tests and analyzed code “hotspots” and our progress with crap4j.

Normally we get bug reports from our userbase or have to test manually to find bugs. A few weeks ago I tried a new approach to bughunting in legacy projects using FindBugs. Many of you surely know this useful tool, so I just want to describe my experiences in that project using FindBugs. Many of the bugs may be in parts of the application which are seldom used or only appear in hard to reproduce circumstances. First a short list of what I encountered and how I dealt with it.

Interesting found bugs in the project

  • There was a calculation using an integer division but returning a double. So the actual computation result was wrong but yet the error would have been hard to catch because people rarely recalculate results of a computer. When writing the test associated to the bugfix I found a StackOverFlowError too!
  • There were quite some null dereferences found, often in contructs like
     if (s == null && s.length() == 0)
     

    instead of

    if (s == null || s.length() == 0)
    

    which could be simplified or rewritten anyway. Sometimes there were possibilities for null dereferences on some paths despite of several null checks in the code.

  • Many performance bugs which may or may not have an effect on overall performance of the system like: new String(), new Integer(12), string concatenation across loops, inefficient usage of java.util.Map.keySet() instead of java.util.Map.entrySet() etc.
  • Some dead stores of local variables and statements without effect which could be thrown away or be corrected to do the intended things.

Things you may want to ignore

There are of course some bugs that you may ignore for now because you know that it is a common pattern in the team and abuse and thus errors are extremely unlikely. I, for example, opted to ignore some dozens of “may expose internal representation” found bugs regarding arrays in interfaces or accessibly via getters because it is a common pattern on the team not to tamper existing arrays as they are seen as immutable by the team members. It would have taken too much time to fix all those without that much of a benefit.

You may opt to ignore the performance bugs too but they are usually easy to fix.

Tips

  • If you have many foundbugs, fix the easy ones to be able to see the important ones more easily.
  • Ignore certain bug categories for now, fix them later, when you stumble upon them.
  • Concentrate on the ones that lead to wrong behaviour and crashes of your application.
  • Try to reproduce the problem with unit test and then fix the code whenever feasible! Tests are great to expose the bug and fix it without unwanted regressions!
  • Many bugs appear in places which need refactoring anyway so here is your chance to catch several flies at once.

Conclusion

With FindBugs you can find common programming errors sprinkled across the whole application in places where you probably would not have looked for years. It can help you to understand some common patterns of your team members and help you all to improve your code quality. Sometimes it even finds some hard to spot errors like the integer computation or null dereferences on certain paths. This is even more true in entangled legacy projects without proper test coverage.