This is no joke.  A bloke on our team came up to me today and asked me about a simple change to some code that was evaluating the BigDecimal.compareTo() operation. (See my earlier blog on why you should do this.) The current code was written defensively to avoid NPE’s:

if (blee != null){

if (blah != null && blee.compareTo(blah) != 0){ return true; } else { return false;}

}

Turns out that the blah != null term was causing an issue as the line would always return false if blah was null instead of the correct answer, which is true. 

So matey wants to avoid the blah != null test failure.  You and I would simply break up the if..statement into two if..statements comme ca:

…

if (blah == null ) return true;  // assuming that blee above was not null

if (blee.compareTo(blah) != 0) …

Much to my dismay, this colleague wanted to NOT do this, but instead add an additional line setting the blah to some random BigDecimal value because “I will chose a number that will never be used”.  

…

if (blah == null) blah = new BigDecimal(-7878); // WTF?!!?!

if (blah != null && blee.compareTo(blah) != 0){ return true; } else { return false;}

What? Eh?  Can he really guarantee that claim? I see our future, and it involves many hours digging out this hard-of-thinking code.

Don’t worry, I straightened him out.

Banging your head all day against a codebase that has been so badly polluted by incompetent developers can really take its toll.  So here’s a list of things you can do to relieve the stress.  In each case note who the culprit is so you can “name and shame” later:

1. count the number of times the plonkers have put double negatives in the code. You know, things like
   1. if (! (something != somethingElse) ) { …
2. hunt around for a) nested loops and b) nested loops that don’t sensibly exit when they should. You get extra brownie points if you can spot one in a critcal execution path.
   1. for(Idiot iDot : collectionOne){
   2. …
   3. for (Stoopid duh : collectionTwo){
   4. …
   5. }
   6. }
   7. // tada! look mum, I just wasted a whole load of time for no reason at all
   8. return somthingThatCouldHaveBeenFoundAgesAgo;
3. Count the number of times you see the same 5 lines of code repeated in the same class.  Extra points here for spotting this across sibling classes. Even more points if you spot a case where an implementation in the base class already exists so the subclass is overwriting the base class implementation with the exact same code. Gah!
4. Count the number of times a method begins with the same two or more words. Like createSomethingUsingA() and createSomethingUsingAWellAlmostAllOfA().  Accessors don’t count.
5. Look for classes that implement THE SAME METHODS as a generalised utility class, but make no reference at all to the utility class and the clever clogs who thought to put his code there.
6. Count the number of //TODO comments scattered throughout the code.  If you are really bored, use your SC system to work out how old the comment is.  Make a note of its age.
7. See how many recursive loops you can find in a class. Play a game with yourself and predict how many recursions it will take before the VM explodes with a StackOverflowError. Make a bet with a colleague and wait for the production defect ticket to roll in.
8. Count the number of static classes and static methods in instantiated classes. Order a small trophy with the words “luddite of the century” engraved on the side, include the statics count and ceremoniously give it to the responsible colleague.  If he/she is no longer there, have a posthumous ceremony in the Pub instead to drink away the pain they have caused you.
9. See how many methods modify the content of an argument without making any comment or effort of any kind to indicate to the sucker following along behind that this method is going to mysteriously change the data on you without warning.  Even better, see if you can find a method that returns a COMPLETELY DIFFERENT OBJECT TYPE while also modifying your argument on the sly.
10. And to my favourite:  Count the number of offences above commited by each member of your team.  There will be certainly one, if not two, who are consistently rubbish.  Go get them a large trophy with “Consistently Rubbish” on the side. Adorn it with all of his/her wrap sheet, for extra zing add the number of hours you have spent looking at and unravelling their cock ups.  Present this trophy to the “winner” in front of his/her line manager.

OK, so you may be without a job after 10), but you will have had a certain satisfaction knowing that you have exposed a serious weakness in the team and that you had public display of your own saintly suffering.

One of the more annoying aspects of working with a predecessor’s codebase is seeing cut and pasted code all over concrete implementations that can so easily be refactored into the base class implementation.  Many books tell us that refactoring upwards is a good thing for a number of obvious reasons.  Code reuse, behavioural predictability and simpler maintenance are the most often quoted.

On a recent project I have seen no less than 12 places IN THE SAME CLASS where the SAME CODE has been cut and pasted.  This suggests that the author of these unnecessary method implementations did not have a firm grasp on what the original code was doing.  Rather than make what would have been a fairly lightweight modification to the existing implementation the author chickened out and simply duplicated the code in a another method (or 10). Oh, and unit tests were nonexistent, proving that the author did not attempt to understand the existing code, properly check the existing behaviour nor modify what was there carefully. This deficiency makes matters much, much worse.

When code like this is released into production, all hell breaks loose.  As the code does not have any unit testing at all, proof of implementation was left to integration testing which is notoriously unreliable because it is very time consuming.  The end result, as has been proven by a recent release with such code was that it needed 9, yes that’s right, NINE, patches in the first month after release.  The main issue was that Defects seen were caused by subtly different implementations of each cut&paste method implementation.

The message here? DON’T DO THIS UNLESS YOU WANT TO LOOSE YOUR JOB.