Where Are The Wise Men?

My co-workers have been buzzing about FindBugs for few weeks and it seems cool in theory.  Well, our Lead sent an email out yesterday that we all need to start using it in our development cycle and, since I was waiting for other people to get back to me, I hesitatingly gave it a try.

Hesitatingly, you say?  Is this because you want to face your bugs?  Not at all — I freely admit that I write buggy programs and I will say that all other programmers do as well.  But I was hesitate because I knew that it would find tons of problems.  You see, I’m just the latest developer on this application and, from what I gather from the source, there has been at least six programmers before me working on this.  Quite a few of them have at zero idea of what the overall picture was of this app and so they just put code in and hoped for the best.  And some of those developers decided to take that code they just put in and copy-and-paste it all over the place.  One section of bad code can be dealt with, but when the exact same thing is repeated ten times, it gets hard to go through.  But the code works and so it’s difficult to try to refactor it into something more manageable.  If it ain’t broke don’t fix it.

So I ran FindBugs and it was just as bad as I thought it was going to be.  A few things were “Oh, I would have never caught that.” and some were honestly no big deal, but most of them were “Why would you do that?”  And most of those were caused by bad code someone had written and the copied-and-pasted it several times. In a few cases, about 50 times.

Here are three of my favorite bad patterns that I have corrected:

• if (nodelist==null && nodelist.getLength()<=0)  This doesn’t look bad at first until you realize that if the first condition is true, you will get a NullPointerException when the second condition is executed.  This was an easy fix — change the && to ||.

• if (item==null) {
       object.setValue(item);
     }

  Why would you make sure an object is null and then set a property to it? I safely assumed they didn’t really want to do this the 20 times it happened.


• This one occured in two ways: object.setValue(txt.toString()); or object.setValue(new String(txt));  This isn’t a bug per-se. It’s not going to cause an exception, but turning a String into a String is a waste of cycles and memory.  Not a big deal if it happens once or twice.  But I changed many, many, many of these.  Inside large loops.
  
  

So if you are working on a old Java project and have some time on your hands, run FindBugs and find all the mistakes you or your predecessors have made.

Powered by ScribeFire.

This entry was posted on Tuesday, May 15th, 2007 at 10:33 am and is filed under java. You can follow any responses to this entry through the RSS 2.0 feed. You can leave a response, or trackback from your own site.