Inverted Logic

A few months back, Derek was writing some logic to decide if something was valid or not. This would then be used to display warning messages to the user.

He ended up writing a method like this:

public bool
NotValid()

Now, if you aren’t a programmer, this sounds fine. But when you come to use it, things can get confusing. When you want the Valid case, you will have to invert it using the logical negation operator “!” (i.e. “!NotValid()”) which then reads “if not not valid”. A double negative.

I flagged this up on his Code Review, but Derek left it in. This led to a bug, because he had used it in a method and didn’t use the “!” to invert it.

I was at his desk the following week discussing a problem he was having. I saw a method that was work in progress which had the same double negative problem (still hadn’t learned his lesson). I was about to comment on it, but then he turned to me and said “I know you don’t like that negative logic, but I find it easier”.

How can it be easier? Having a method called “Valid()” then using “!Valid()” if you want the opposite is perfectly fine. It is easy to understand and easy to read: “Valid” and “Not Valid”; completely fine. Versus “Not Valid” and “Not Not Valid”; confusing. I don’t see any aspect you can use to justify it is easier.

On a similar note, there was a Code Review from Colin that was to check one date wasn’t greater than the other. The common validation scenario where you have two Date controls, and want to enter a date range; From date, and To date.

public boolean IsFutureDate(DateTime startDate, DateTime endDate)
{ 
  return !(startDate <= endDate) 
}

Here, the method signature is fine (maybe their naming was better than mine), but the logic inside is using a negative approach which makes it hard to read. “Start date not less than or equal to end date”. Why not simply “start date greater than end date”?

return startDate
> endDate

Lessons Learned

Logic within programs can be naturally complex, but the skillful programmer can break this down into small, manageable functions. There is no point adding in additional complexity by writing simple statements in an illogical, hard to read way.

Time In Ints

I wanted to keep my stories anonymous, but there is a very specific developer I want to write many stories about. I want to attribute those stories to the same person to build up his character, so I’ll always blog about him under the name Derek. Any other story, I’ll blog under the name Colin, and collectively, maybe Colin’s persona will match the insanity of Derek.

So here is a Colin story.

Just a background for those not familiar with C# or programming languages. If you want to store a number, you will often use the type “int” which is short for Integer; a whole number.

Colin had to program a countdown timer which took the time from a config file. Here was the Property he chose:

public int TimeInInts

The second word is the return type, so as we begin to read this, we know the Property returns an integer. What does this integer represent? A time. Fair enough, but in what unit? Ints.

Ints!

Absolute useless! Is it hours, minutes, seconds, milliseconds, something else? We don’t know without reading more code. That’s the telling sign of a badly named Variable/Property/Method; if you have to dig deeper to work out how you can use it.

I assumed it was minutes, but I look in the same file and there is another method. 

public int GetTimeInMinutes() 
{
  return TimeInInts * 60 * 1000; 
}

So GetTimeInMinutes uses TimeInInts in order to calculate minutes, so TimeInInts isn’t minutes.

Hang on, “* 60 * 1000“. That’s the calculation to convert minutes to milliseconds. So TimeInInts IS time in minutes and GetTimeInMinutes returns the time in milliseconds!

What.

The.

Hell.

How can you even write that, use it, and possibly get the feature working when everything you are dealing with has the wrong name?

Unbelievable.

Anyway, if you wondered where my username came from; now you know.

Lessons Learned

Always try to come up with the best variable names possible. Anything ambiguous, misleading or wrong will lead to confusion and bugs.

Derek Origins

There was a project I was working on which appeared to be low priority. When the Lead Developer had left the company, he wasn’t replaced. Additionally, another developer was reassigned to another team. This left us with three developers, but one soon handed in his notice. That meant I had suddenly become the Lead Developer in the team by default, but there was only so much you can do in a team which essentially has 2 developers.

I always find it inspiring working with better developers because you can aim to be as good, or better than them. So when the news broke that we were getting a Senior Developer and Derek, a Developer that had just been promoted from his Junior position; I was excited. The business seemed to have a great track record in bringing good Juniors in that prove their worth (myself, for example), so I had high hopes indeed.

However, Derek instantly made a bad impression, and he hasn’t improved in the following years.

One of my earliest memories of Derek was when he was tasked with fixing a trivial bug, but came up with a solution I just didn’t expect anyone with more than a few weeks experience to even consider.

We had a simple feature that you select an entry in the grid, click a button, fill in some text and click save. The text, along with an ID is passed into a stored procedure to save the data to the database. It was simple bit of code to understand, the SQL took in the ID of the row, and updated one text field. However, it seemed some recent changes had caused the ID to be Null, so we were asking the database to save some text against an unspecified row. As a result, this crashed the application.

The solution to everyone is obvious. Find the line of code that should set the ID, and make sure it is populated with the correct ID. The SQL doesn’t even need to be changed.

Derek changed the SQL.

He decided that: because the Bug Report was reporting a crash; the solution is to simply stop the crash. His solution was to put an If Statement in: “If the ID is null, then return”. Crash averted. However, the feature is still broken. All that happens is that the user is no longer told something is wrong. The system just pretends it has saved (deceiving the user) and carried on as normal. This could have serious implications, because this was actually a fairly important bit of data that should be saved. There was a bug in the system, and Derek had made the problem worse.

I obviously dismissed the Code Review, but to my surprise, he had checked it in anyway. I went over to his desk and told him in person that this needs to be looked at. He got angry and argued that I was changing the requirements because it never said in the bug report that this needs to work; it only said there was a crash and this shouldn’t happen. He had met the requirements and advised I needed to log a new User Story. I think he was actually deadly serious, but I walked off, deciding to wait until the next day before pursuing the issue further.

Luckily, Derek must have reflected on the incident somewhat and did revisit it, although he gave me a flippant comment about only doing it to “make me happy”.

Lesson’s Learned

Everyone understands that an application crash is bad. The software isn’t working as intended, and the user will become frustrated when there’s a feature they cannot use. However, a crash informs the user that something is wrong, and the user has full knowledge that they have to perform this action again when the issue is resolved. Hiding the error from the user means the user has lost their work and is unaware. Depending on the feature, the user may take a while to realise the data is lost, and by that point, maybe they don’t recall what they entered to be able to redo it. The missing data could mean someone makes a different decision, potentially having serious implications.

There may be times when problems can safely be hidden from the user. Careful thought needs to applied in order to fully determine if the functionality is trivial enough, and the problem should be logged (in some kind of log file) for developers to investigate.

Data Table Madness

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)