Useless Git

As part of my employer’s technology refresh, we will now be using Git as our source control (alongside Azure DevOps), moving on from Team Foundation Server (TFS).

Although using Git requires a different way of thinking, it is still a type of source control, so the same steps of checking out code, making changes, and checking in still applies.

I get into work, click a few buttons to “Clone” the source (it took around an hour for that to complete though), then click a few more buttons to set up a branch. Since I had been making changes before the switch-over, I then copied my changes that I had been doing in TFS into my Git repository. After 2 hours, I had my code uploaded to the server and had a decent understanding of the process.

Today, Derek was doing the same process, and by the end of the day, he had finally sent his “Pull Request” (Code Review in TFS terms)…with the assistance of the Lead Developer. He then exclaims “that’s a fairly successful first attempt”, then goes home.

Sorry to disappoint you Derek, but this is a trivial process which took you 5.5 hours longer than me, and required assistance of the Lead Developer.

Work Hours

I always wonder what is an acceptable amount of breaks because people have their own views on what is efficient. Some people like regular breaks, whereas others prefer fewer but longer breaks.

Often, managers say:

“I’m not bothered if the work gets done”

but it would be strange if someone did 3 hours of perfect work, then left for the day.

Personally, I rarely leave my desk, but sometimes procrastinate by peeking at other team’s work to learn what else is going on.

Derek takes 20 minute smoke breaks at certain times of the day. He will also have a 20 minute break with his best work-mate, then take an hour lunch even though contractually we get 30 minutes. On a good day, he will have 2.5 hours out of his 7.5 hour day away from his desk, but on bad days he can have up to 4 hours.

Now, the amount of output we get from Derek isn’t actually the lowest in the team, although often you can question the quality of work. My view is that since Derek spends so much time trying to avoid work, he isn’t very good at it. So when he actually tries doing work, he has to try and cut corners and cobble something together.

Meanwhile, Colin comes into work late, takes way fewer breaks, but his breaks are longer. He is known to work obscure hours, remoting in at home. When available, he will also claim as much overtime as possible, so he appears dedicated. His output is much higher than Derek, but the quality is often erratic.

If I was a manager, I think I would much prefer people to be around their desks, so even if they are chilling out; they are free to answer questions from team members that are trying to get something done. Ultimately, I think the person’s overall output should be analysed to have evidence of their approach. Low quality, or low quantity is poor; so I think Derek and Colin’s approach needs to be questioned.

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.

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)