The following story is true, but is anonymised. The domain of Animals wasn’t used in the exact code, and the requirements about Identifiers for England and Scotland is purely fictional. However, if you find and replace the fictional terms for the actual ones, then the story is 100% true.
So we have a dialog which displays various animals which basically come under three types. Dogs, Cat, and Fishes. The Dogs and Cats both have Mammal Identification Numbers, although we only display them for the Dogs as per requirements.
Now Scotland has come along with the requirements that Dogs now need to display alternate identifiers (ScottishPrimaryIdentifier and BreedID), so there’s two different identifiers compared to the one in England.
This new way of displaying the information is in a dialog, and one of the buttons launches a different dialog which shows a slightly different view, but still needs this information. You would think it was simple, but when Derek is on the case, you know things can get messy.
So the first dialog, Derek adds two columns and programs it so they show for both mammals which was incorrect, and due to the way it was trying to populate the information; it was crashing (obviously no regression testing was done on Derek’s part). Not only that, but he didn’t actually add two columns for the new identifiers like you would expect. He added a duplicate column for the “Animal Name” (hiding the existing “Animal Name” column in England, showing the new one for Scotland), then changed the name of the EnglishPrimaryIdentifier column and shared it with BreedID, with a new column being added for the ScottishPrimaryIdentifier. This is absolute madness of course, because if you felt the need to share a column, the ScottishPrimaryIdentifier would be the only logical column to share with EnglishPrimaryIdentifier since they are primary identifiers and somewhat logically equivalent. It looks even more stupid in the code, since he ended up writing “breedIDCellValue = englishPrimaryIdentifier” for the English case. Further to this, he had all kinds of local variables which were wrongly named so you even saw lines of code like “englishPrimaryIdentifierCellValue = animalName” even though it was actually the cell for Animal Name. So I end up rewriting it, correcting all the names, removing the duplicate column, separating the shared column, correct the logic to only apply to Dogs, and then notifying Derek that we must ensure we only change these dialogs for Dogs.
The next day, a Code Review comes through for the second dialog which looks like it wouldn’t work for Cats (yet again), so I leave a comment on the Code Review asking him to check the logic. This gets checked in, but reopened by the tester because, to Derek’s surprise; it didn’t work for Cats.
So Derek picks up the work item for his second attempt. Instead of using the same logic I had written, he decides he will use his own approach. He creates a property called IsCat and uses that to set the visibility of the columns. This obviously fixes the Cat problem, but breaks it for Fishes because obviously Fishes aren’t Cats. Fishes now show extra columns with no data in them. It gets reopened again.
So Derek picks it up for the third time and creates a new property called IsDog which sounds correct, but he still insists on using the redundant IsCat property; ie IsDog = (!IsCat && animalID == dogTypeID). Obviously stupid, but it works.
When analysing the rest of the final code, I notice that he has also pursued a very stupid design using more redundancy, plus misleading method signatures. There’s a method AddIdentifierRow(string animalName, string primaryId, string breedID, bool isCat). Remember how I said we don’t show identifiers for Cats? Well when you look at the code inside the method: if isCat is true, then the two data cells for identifiers are set to string.Empty, rather than using the two identification parameters that you passed in. So basically the boolean isCat means “ignore the previous parameters”. It’s also misleading that it is named AddIdentifierRow when it isn’t gonna display Identification Numbers in some cases. Even more misleading is the fact there was an overload of AddIdentifierRow which didn’t even take the isCat parameter, and the only usage was inside an If Statement where it was always guaranteed to be a Cat. But how does AddIdentifierRow method know it is dealing with a Cat rather than a Dogs? Well, this time it checks if BreedID is null. Technically Cats do have a BreedID, it’s just the business logic to not show it; so that’s not a great way of determining it wasn’t a Dog. So we have the same method name which has different ways of determining the intent of the caller! So confusing! To be fair though, this was existing code before Derek got his mitts on it, but a good developer would have refactored the code to make the intent clearer. Simply putting up with pre-existing poor code and making it worse is unacceptable.
Lessons Learnt
I’m never a fan of passing booleans into a method. In most cases booleans will result in an If statement with two outcomes. If we already know what the outcome should be, then we should be able to call a method directly which describes exactly what we want to do. So AddIdentifierRow will become AddCatRow, AddDogRow, AddFishRow, and the parameters you pass in will always be used. So AddDogRow will have a signature like (string animalName, string primaryId, string breedID), yet AddCatRow can just have (string animalName)
Stranger Coding Tings 