Thursday 7 February 2013

Bad (programming) habits instilled in me

What can happen when enforced terrible coding style practices become ingrained!


A company I used to work for had a "Style Sergeant" [ref: The Code Warriors] of the worst kind.
This person was allowed to enforce coding style rules on the rest of us as long as they could justify them.
In this case a stye rule for method local variable names was introduced on the premise that it shortened build times considerably.

The rule was simple, the names of the variables had to be as short as possible to remain unique with the accepted case being an abbreviation of what the variable was.

Now I am not going to get into the obvious fallacies here.
Build times went up because the code base was growing anyway.
Code readability went down the toilet. Mr Code Sergeant spent too much time refactoring existing code and committing it rendering existing code unreadable too.
Number of bugs introduced in new or changed code went up (it was expected that it would take time to get used to the new naming convention)

I was reviewing and refactoring some code I wrote a while back and was horrified that it took me so long to unravel what was going on. There was also a bug in the code that had the potential to cause a mountain of database orphans that was really hard to spot which was caused by the local names I was in the habit of using back then.

Here is the original code after refactoring for Java 7.

public PlantProperties replicate( PlantProperties toReplicate ) {
        Replicator<PlantProperties> rp = new Replicator<>();
        PlantProperties cpp = rp.replicate(toReplicate);
        PlantTypes pt = toReplicate.getTypeID();
        Replicator<PlantTypes> rpt = new Replicator<>();
        List<ProductionLot> lpl = toReplicate.getProductionLotList();
        if( lpl != null ) {
            try {
                List<ProductionLot> clpl = lpl.getClass().newInstance();
                Replicator<ProductionLot> rpl = new Replicator<>();
                for (ProductionLot productionLot : clpl) {
                    ProductionLot cpl = rpl.replicate( productionLot);
                    cpl.setPlantID(cpp);
                    clpl.add( cpl );
                }
            } catch (InstantiationException | IllegalAccessException ex) {
                Logger.getLogger(PlantPropertiesReplicator.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
        cpp.setProductionLotList(clpl);
        return cpp;
    }

Absolutely unreadable after 2 hours never mind 2 years.

This is the code after a refactor including the bug, the bug should become apparent almost immediately, at least it did to me.

public PlantProperties replicate( PlantProperties toReplicate ) {
        Replicator<PlantProperties> toReplicateReplicator = new Replicator<>();
        PlantProperties replicatedPlantProperties = toReplicateReplicator.replicate(toReplicate);
        List<ProductionLot> plantLotList = toReplicate.getProductionLotList();

        if( plantLotList != null ) {
            try {
                List<ProductionLot> copyOfPlantLotList = plantLotList.getClass().newInstance();
                Replicator<ProductionLot> plantLotReplicator = new Replicator<>();

                for (ProductionLot productionLot : copyOfPlantLotList) {
                    ProductionLot replicatedPlantLot = plantLotReplicator.replicate( productionLot);
                    replicatedPlantLot.setPlantID(replicatedPlantProperties);
                    copyOfPlantLotList.add( replicatedPlantLot );
                }

                replicatedPlantProperties.setProductionLotList(copyOfPlantLotList);
            } catch (InstantiationException | IllegalAccessException ex) {
                Logger.getLogger(PlantPropertiesReplicator.class.getName()).log(Level.SEVERE, null, ex);
            }
        }
        return replicatedPlantProperties;
    }

In the for loop it turned out I was replicating the copy of the PlantLot list instead of the original PlantLot list.
By the way, the NetBeans <CTRL>+R refactoring of current selection makes this sort of thing trivial and was what led me to spotting the bug immediately.

The bug was that if the replicated version of the instance in question was used as a template for a potential Deletion Strategy then the PlantLot instances in the list would not be deleted as well.

Four things here


  1. If you are responsible for code style rules make sure they make sense. Good style means others like to look at what is created. Other reasons you can find here and here.
    You should have refused the responsibility anyway, this sort of thing is so totally bureaucratic and counter-productive
  2. Even though I have been free from the tyranny of the Style Sergeant, Nero and the others in my old dev team (yes, the blog I linked evoked some powerful memories) I STILL FIND MYSELF WRITING CODE USING THOSE UTTERLY AWFUL RULES!!!
  3. Protest vigorously if someone tried to oppress or repress you in this manner.
  4. This is not a rant against naming conventions which are very important but can also end up leading to a bureaucratic mess when overzealously applied. The Style Sergeant was applying the rule for coding style not as a naming convention. Not that it makes any difference, it is still utterly terrible.
The creepy thing is I only write like this when in the office. At home my code is (hopefully) self-documenting and more like the second example and I was compelled to do this refactoring while in the comfort and safety of my own living room!