Pointless Project Update Meeting

A couple of years ago, I was working on some new software but the management of it was an absolute disaster. Even though it was an absolutely massive project, I was often sat there not knowing what to do. Additionally, there were teams that were doing exactly the same work as each other. It was a mess, so eventually I moved back to work on our existing software.

This week, there was a project update demo by a team still working on this new software. I thought all the speakers were working on this feature, but it seems Jennifer might have been on annual leave or something because she didn’t understand what work was done.

After a few slides of their presentations, Jennifer interjects.

Jennifer : “How is this different from what Igor is doing?”

(Igor is part of another team)

Luke: “I’ve had the conversation and I still don’t understand why this was brought in twice.”

Jennifer: “I don’t know what the use case is for the two versions.”

Mark: “We never got to the bottom of it, so we just cracked on and built what we got asked to do.”

Brilliant. So there’s multiple members of the team that were aware that another team was also making this functionality. Despite having conversations with managers, they still were none-the-wiser, so then they just made it anyway, despite knowing that one team’s work is going to be binned off.

I can’t believe a couple of years have gone by and they are still doing stuff like this.

Cannot Count

There was a bug in our software where a crash occurred when an address with over 50 characters was specified. For the fix, the developer changed the code to truncate the text down to 49 characters.

“why are you using 49 and not 50?”
Me

He thought the “length” property was actually 1 less than there are; so 50 characters would report the Length property as 49. He was thinking of arrays where they are zero-indexed, so the first position in an array is Index 0, second position is 1 etc. Obviously he didn’t test it, and it was the wrong assumption. Length means length.

Also, there were 5 other address fields with no validation (Line 2, Town, County, Postcode, Notes). Why didn’t he add validation for them too? Just because the bug report was for the first line of the address, why wouldn’t you consider the rest of it?

After I prompted him, he added validation for all fields, truncating them to 50 except one where he truncated to 51. Why 51?

“I manually checked in the registration screen. That field holds 51 chars.”

Weird, because I manually checked it and looked at the code – it is 50. Let’s check the database column too… That’s right. It’s 50.

“Thanks, I’ve changed it to 50. Sorry, some kind of counting mistake.”

We seem to have so many developers that don’t put much thought into their work. This is one of the easiest bugs you can fix, and yet I’m basically hand-holding them through it. When there’s more complicated bugs for me to review, I end up scrutinising the code in great detail because I end up assuming they put zero thought into the solution, and didn’t test it.

String comparisons

We recently had a controversial bug where our software was showing female-related information for males. Another team was responsible for the code that generates the warnings, but first I had to check we were passing in the correct information.

For males, we were passing in “Male”, “Female” for female, and “Unknown” if the information wasn’t stored. This looked correct, although using text in this way easily leads to bugs as I will soon discuss.

Another point of contention here is that if this is representing Gender, we aren’t providing enough options for today’s culture. More controversially – throughout the codebase, we are sometimes labelling this Sex and other places it is Gender. The user interface labells it Gender, but then the database column says Sex. 😬

This is obviously going to lead to problems if the developer uses this data in a way that wasn’t intended.

I was surprised users hadn’t flagged this as an issue because surely someone would be offended by the limited options and the way we display it. Also, some features of our software could easy display misleading information (just like the bug I will discuss).

So back to the main point of this blog. We were passing in what looked like the correct data, but then I looked at the other team’s code. It was actually worse than I anticipated.

if (sex == "M")
	criteria.Add("Sex = Male");
else
	criteria.Add("Sex = Female");

Whoopsies. This is wrong on so many levels.

First of all, the method call asked for a C# String which is: text of one or more characters. There is a special type called “char” to represent a single character. So if they only wanted 1 character, then they should use the “char” type. However, using char still has similar flaws like I explain in the next paragraphs.

Performing String comparisons is bad because I could pass in “Male”, “male”, “MALE”, “m”, “m” etc, with similar permutations for “Female” and “Unknown”, and I could have also passed in some complete nonsense. What happens if I pass in “Thing”? surely it should throw an error because that is invalid? Instead they treat “Thing” as a “Female”.

This has caused the behaviour we were seeing. We pass in “Male” which seems like the correct thing to pass in, but because “Male” doesn’t equal “M”, the method treats “Male” as “Female”. It also treats “Unknown” as “Female”. Even if they had used char types, we would still see problems when comparing “M” to “m”, and “U” would still be treated as “Female”.

A more robust solution would be to define an Enum type of all the states, then check for certain types explicitly, but throw an exception for anything that doesn’t match the conditions you have defined. If someone adds more cases to the Enum in the future but doesn’t modify your code, your code will cause a crash. At least that highlights a problem rather than causing unexpected behaviour, or causing offence to users or their customers.

It can look something like this. (You can use a “switch” statement if you prefer).

if (sex == Sex.Male)
{
   criteria.Add("Sex = Male");
}
else if (sex == Sex.Female)
{
   criteria.Add("Sex = Female");
}
else if (sex == Sex.Unknown)
{
   criteria.Add("Sex = Unknown");
}
else
{
   throw new ArgumentInvalidException(sex);
}

I was going to suggest that if they had written unit tests, then some of these problems would have been spotted earlier. However, if they were barely thinking about the code they were writing, then no doubt they would come up with unit tests to check that “M” returns “Male” and “F” returns “Female”, and wouldn’t have bothered with other scenarios. You would have thought someone that code reviewed it would have spotted it, but they must have lazily done that too.

Database Patching – Everything Is Fine

When it comes to changes to the Database, we have a tool (which I will call DBPatcher) which runs your changes, runs the Unit Tests and runs Code Analysis (finds bad formatting, violations of coding standards, common mistakes etc). So it is vital that this passes successfully before you send your code to review.

I was doing a Code Review aka Pull Request (PR) and I saw evidence that they hadn’t run it through this DBPatcher tool.

Ronald was eager to get his code checked in, so wanted me to approve. However, I wanted them to run the tool and fix any issues first. The conversation went like this:

[Thursday 8:21 AM] Ronald
     Can we complete the PR? do you have any doubts on it 
[Thursday 8:23 AM] Me
    I'm convinced DBPatcher will flag those select statements because there is a mix of tabs and spaces
<yes it is trivial to flag, but DBPatcher will flag this, so this is evidence they haven’t run it. There could be other errors too, but I will let DBPatcher find them>
[Thursday 8:23 AM] Ronald
    OK, thank you. I will complete the PR 
[Thursday 8:25 AM] Me
    what? I am saying the DB patcher will give you errors
[Thursday 8:26 AM] Ronald
    sorry for misunderstanding 
    I ran it in the morning. We didn't get any error for our DB changes and unit testing also didn't throw any error for our code
<he attempts to send me a screenshot of the final result but it didn’t seem to transfer>
[Thursday 8:44 AM] Me
   The image isn't showing for me. But since I started running DBPatcher when you messaged me, and mine has just finished, I can only assume you disabled the "Run Code Analysis" to speed it up
[Thursday 8:45 AM] Me
    In fact, there's some failing unit tests too
<this is contrary to what Ronald claimed. He said there were no Code Analysis errors and no Unit Test failures, and I see both.
[Thursday 8:45 AM] Ronald
   I have enabled those and haven't unchecked it before running the patch 
[Thursday 8:45 AM] Me
    What is in the output window?
[Thursday 8:46 AM] Ronald
    yes there are some errors, but not related to our code and our schema 
[Thursday 8:48 AM] Me    
DataWarehouse
Error on line: 12
ColumnListFormatting: Select column list incorrectly formatted
<clearly his code>
[Thursday 8:50 AM] Ronald
    oh ok 
[Thursday 1:19 PM] Ronald
    we resolved formatting in our SQL commands 
    we couldn't find which unit testing is failing and we are not sure if this unit test is part of our project. Can you help us with this one ?
[Thursday 1:21 PM] Me
    
|20|[DataWarehouseTest].[Test1] |Error |
|21|[DataWarehouseTest].[Test2] |Error |
|22|[DataWarehouseTest].[Test3] |Error |
|23|[DataWarehouseTest].[Test4] |Error |
|24|[DataWarehouseTest].[Test5] |Error |
[Thursday 1:26 PM] Ronald
    
I ran the DB patcher 20mins ago with the code analysis checked and we checked the output results also, we couldn't find anything related to DataWarehouseTest 
Attached the DB patcher output result we got 
[DBPatcher OutputResult.txt] 
<I look at the file. It has hundreds of errors, so it is hard to make sense of. His database is clearly screwed. No wonder it was running quick and he couldn’t see any Unit Test errors; they simply weren’t running>
[Thursday 1:31 PM] Me
    your database looks absolutely messed up. You shouldn't have those errors. The unit tests are failing to run

C:\DatabasePatcher\tSQLt\RunAllUnitTests.sql
Could not find stored procedure 'tSQLt.RunAll'.

    you need a new database.
[Thursday 5:50 PM] Ronald
    Thanks for notifying us of these issues.
    Now we have fixed these issues and ran the patch, and there were no issues with our project.
[Thursday 5:51 PM] Ronald
    please review it from your side

I then look through their changes which fixed the unit test. With Unit Tests, you usually create a variable called “Expected” then set that manually. Then you create an “Actual” variable and this is set based on the actual code. They had those statements as normal, but then they had added this:

update #ActualResult set SessionGuid = '38090f0d-3496-48c3-a991-a0220fe3b58f', SlotGuid = '0b817794-7ffb-4ae3-8013-a7847a1b2139';

So this means their code isn’t returning the correct result, but they are then manipulating the result (#ActualResult) to force it to match – so the test passes. They could have just changed the Expected result, but that would be sabotage anyway. Why would they knowingly break a feature like this?

Anyone who is serious about software development shouldn’t be doing this. They have “Senior” in their job title, and this change was approved by two of their team members. It was up to me to be the gatekeeper and reject this change.

[3:51 PM] Ronald
Sorry for the unit test update statement, I have removed those and all the unit tests are passing correctly.
Sorry, that was some typo.

A typo!? How can you possibly claim that was a typo? “Sorry, I accidentally bashed on the keyboard and somehow produced a sequence of characters that was valid: not only to be executed without error, but for the unit tests to pass too.”

I also don’t understand how you can have hundreds of errors and just continue working like everything is fine. Then when someone is telling you something is wrong, you still pretend everything is fine. When I tell him he hasn’t run DBPatcher, why didn’t he respond with “I did, but there were loads of errors. Can you help me fix this?” Proceeding like he did just wasted my time, created unnecessary friction and made himself look like a complete idiot.

Code Comments are Code Smells

There was some code where there were 2 statements that look similar in meaning:

Details is null
No Details

  if (Details == null)
     return (bool?)null;
 
  if (!Details.Any())
     continue;

Yet, both those statements returned a different value. The first returns null, the second continues iterating the loop that the code was in and could return a completely different value depending on what was next in the list.

So a reviewing developer writes a comment on the review:

“It is not clear why one should return and the other continues here. Maybe a quick inline comment to explain?”

(So the reviewing developer wanted them to write a code comment alongside the code, so anyone reading the code would be able to read the code comment explaining what the code means – therefore quickly understand the intent.)

The authoring developer responds:

“Explaining here as adding comments will open up code smells,
The Message can contain more than one ‘Detail’ and it will validate the following cases….”

(It’s not important for this blog to post his full explanation. The key thing is his refusal to add the code comment because code comments are apparently a “code smell”. The reviewing developer counters this statement accurately…)

Reviewing Developer:

Code smells are code that cause people headaches to read and understand, not what Static Analysis Tools say. Static Analysis Tools often spot things that meet the above criteria, but that doesn’t mean it is right all the time. Adding comments to explain why something has been done the way it has when it isn’t obviously clear IS best practice. It is best to write clean code so you don’t need comments, but when there is always a clear benefit to adding them, you should add them.

I’m not even sure what the author meant by “open up code smells”. I can’t think of a Static Analysis rule that could violate. Recently, I have encountered many examples of developers writing bad code, or changing code they didn’t need to touch. Then when I challenge them on it, they say the Static Analysis tool was flagging it up, so they modified the code until it stopped complaining. The point of Static Analysis tools is to write good code/to enforce best practice. However, like the reviewing developer said, it isn’t always correct. Also, the rules aren’t smart enough to detect when you are trying to trick them, so you can write bad code that doesn’t violate the rules. If you are resorting to tricking the analysis, then you definitely don’t care about writing good code.

In the book Clean Code, a point that the author makes is that you can eradicate code comments by writing readable code. Sometimes that’s not possible which is why you then need to write a code comment to provide the clarity.

Mentoring #5

I am mentoring an Apprentice who has never done C# before, and this is his first programming job. So this is a diary of some-sort of his progression and my ability to mentor. I haven’t written about my Apprentice in detail for a while – it’s been 4 months!

The bad thing is that there’s no good news to talk about, but I can sure rant about him instead. I’ve spent a lot of time with him over the past year, giving him general advice based on my journey as a developer and telling some great stories. I’ve explained what I think makes a good developer, emphasising how it is fundamental to know how to debug effectively, and how you need to find the root cause of an issue rather than papering over the cracks.

When a batch of Apprentices joined the company, we had no plan for them, so managers passed them between teams aimlessly. When he joined my team, I promised him I would put in the effort and be the mentor he expected to have when he joined. So I can help him reach whatever potential he has, but he needs to put in the effort to get there; I can just get him there faster. I said he can ask as many dumb questions in the first year, but after that, then I will judge him.

I’ve given him exam questions, a pdf book, a list of project ideas I spent a few hours writing, a list of easy bugs to fix, and I frequently send him code samples to discuss. I’ve even made some basic programs and set the challenge to enhance them. I’ve done live coding demos, talking him through how to make a noughts-and-crosses game. I said I would code review anything he sends me.

I told him that it’s not mandatory to do everything I send him, because people learn in different ways, but he needs to put the effort in to do something. Maybe he doesn’t like reading and prefers watching videos? The key thing is just to get writing code.

However, with my Apprentice, he has been with me a year, and yet has basically nothing to show for it. He just seems to make excuses and somehow time has flown by.

I did get another Apprentice assigned to me. He did the exam questions within a couple of days. He made his own Battleships program in the following week, he completed a few easy bugs in the weeks following that. Then he moved teams, but I thought he was probably ready for proper work. He did seem far more naturally gifted, but he also had the high determination and took all my advice and instructions on board. I couldn’t have been happier with him.

A few months ago, my Apprentice also switched teams and I said to him he was nowhere near ready to be assigned proper work. The work his team had planned would feature more advanced, Senior level topics like threads, and required good knowledge of the domain.

I advised him to partake in meetings and do a bit of pair-programming so he has a feel for what is going on, but he must spend half the day practicing code, working through the assignments I had given him.

After a few weeks he calls me and says he has felt he has wasted his time and learned nothing. I asked him what code he has written and he says he has only gone to meetings and been watching a colleague work. He then asks me for advice. I remind him of my original plan and he says “oh yeah, you did say that, I should always listen to your advice because you are always right”.

A few weeks go by and I ask him why I still haven’t seen any code from him. He said he felt bad doing his own personal tasks, on work time. In his team stand-ups meetings, he felt like he needed to talk about his team’s work otherwise his colleagues would think he isn’t contributing. I ask him what he has spoken about if he hasn’t done any work for the team. He has not contributed anything. At least if he actually practiced coding over the last couple of months, he would have had the skills to try and contribute from now on.

He said I was right again, although he was still adamant he wanted to do some proper work. He promised to start his project at the weekend and continue to do it outside work. So I picked out a few bugs from the backlog that looked easy. His Product Owner and Scrum Master weren’t happy about bringing in work to their team’s backlog when it wasn’t planned. I explained that currently their work is far too hard for him and he isn’t contributing so should imagine him not in the team. So let’s give him some easy work, then he can do something. Even if the work is shelved and the Testers don’t have time to test it, at least he has learnt something and he will feel like he has done something.

So after 1 year of my mentorship, I can now judge him and he has been a complete disappointment. He has only done 2 easy bug fixes with my supervision. In some ways, I feel like a failure but I know I’m not a bad mentor. I have put so much effort in and gave him so much advice. I feel like he disregards it, then says I am right, but then doesn’t change and carries on with the wrong approach. I definitely held my side of the bargain of being the mentor that they promised him.

Has he started his project yet? No. He just comes out with excuses. He does have some good reasons, but it’s hard to justify not being able to find a few hours here and there over a year, or even just the last couple of months:

Moving into a new flat
Went to visit a friend down in London
Forgot his son was staying the weekend
His son enjoyed his stay so wanted to stay again
Decided to start boxing as a new hobby
Watching the Euro 2020 England game.

I’ve stopped asking him about it, because I feel like I am just repeating myself, and have given him so many ideas of what to code. He isn’t in my team so I feel like I’m not responsible for him anymore. I’ll still be around if he wants help or advice because I enjoy helping people, but I’m not going to be proactive anymore.

Truncate

A developer sent a review for a method that was displaying a tooltip. He was fixing a simple “null reference” exception, but the logic for displaying the tooltip seemed weird:

If the text is greater than 100 characters, we don’t show a tooltip. If it is less than 100 then we do.

So I suggest that we always show the tooltip, but if it is greater than 100 characters, then we truncate to 97 characters plus some ellipses “…” to show the user that some text is missing.

At first, the developer responds that he was already truncating it. So I explain again that it doesn’t show at all in the case the text is greater than 100. He agrees to fix it. Later, he asked me to review the code again.

Now he is showing the tooltip if it is greater than 100 characters, but he is showing 100 plus the 3 ellipses to make 103 characters in total; which is over his defined limit. It’s a bit weird but not a huge problem. Additionally, he now no longer shows a tooltip when it is less than 100 characters, so he has just reversed the problem!

So I point out both the problems and he agrees to fix them. He asks me to review the code once more. Despite fixing these issues, he has now reintroduced the original problem that he was fixing! Now it will crash again with a “null reference exception”!

So I say

“Doesn’t this reintroduce the bug you were originally fixing?”.

It’s a rhetorical question of course, I know full-well that it does; but it’s more polite than an expletive-ridden comment questioning his intelligence.

“No, it doesn’t bring back the original bug back in – I have verified that.”

I ask him if “he is sure about that?”. I don’t even need to test it, I can see that it is 100% a problem just by reading the code. I’d expect a Junior to easily spot that without testing it. Not sure how he could miss it if he had done some proper testing.

“As per my basic testing it didn’t crash but I can do some more testing and let you know.”

1 hour goes by. He then replied:

“Just now I saw the mistake from my end”

(－‸ლ) 🤦

Terrible Colin Logic

The developer job titles are essentially: Junior, Developer, Senior, Principal, then I guess you have Architect which could be seen as a rank above that. Colin is a Principal. In this blog, I document his struggles with simple Junior-level concepts. Using simple AND and OR logic is a prerequisite to being a developer in the first place.

Let’s say you have 4 possibilities which we will label with the letters A, B, C, D and we have some code that checks a variable is one of these types (or not).

He wanted to write some logic that says “Not A or B”. In the programming language C#, “||” is the logical OR, and “!” is NOT which inverts it (so “true” goes to “false”, “false” goes to “true”). So we can denote “Not A or B” as !(A||B). Here it is as a “Truth Table” of how it should work.

Item is	(A\|\|B) “A Or B”	!(A\|\|B) “Not A or B”
A	true	false
B	true	false
C	false	true
D	false	true

“Not A or B”: !(A||B)

but instead, Colin wrote !A || !B (“Not A Or Not B”).

Item is	!A “Not A”	!B “Not B”	!A \|\| !B “Not A Or Not B”
A	false	true	true
B	true	false	true
C	true	true	true
D	true	true	true

“Not A Or Not B”: !A || !B

So A returns true because it is not B. B returns true because it’s not A. C returns true because it is not A. D returns true because it is not A. So the statement is always true.

A developer points out that it’s nonsense so he has another go. His new method was

A || !B || C || !D

“A Or Not B Or C or Not D”. It’s getting really confusing now, but also returns true for everything.

Item is	A	!B	C	!D	A \|\| !B \|\| C \|\| !D
A	true	true	false	true	true
B	false	false	false	true	true
C	false	true	true	true	true
D	false	true	false	false	true

A || !B || C || !D

I couldn’t believe he has come up with more nonsense and still not tested it. I didn’t want to tell him exactly because he should be able to tell that he has done it wrong if he glances at that logic, so I left a simple comment along the lines of “this won’t work”.

Later, after I got back from lunch, I saw that I had a missed call. So I send him a message “If you are calling about my comment, just test your method”.

He calls me back and starts explaining the requirement. I said that I don’t really need to know, I just know the method wasn’t what he wanted. I ask him to look at the method again. It took him 30 seconds or so before he spotted it.

Colin: “Why did I type the exclamation mark? I must have copied and pasted it. I’ll correct this line and you can review it”.

Me: “Aren’t you going to test it?”

Colin: “No, this will work this time”

Me: “Well, you thought the last two times would work. I recommend you look at the logic a bit longer if you’re not going to test it”.

He looks at it for 10 seconds.

Colin: “Oh there’s another exclamation mark. I’ll change that too”.

He got it right with a bit of supervision, but I don’t think he had much intention of testing it.

Increasing Customer Satisfaction

There’s been a big push to increase customer satisfaction, and the Head Of Development is actively monitoring the biggest issues seen by our users.

Colin has been looking at the most frequent errors and trying to proactively fix them. One “frequently occurring” error is from a group of users that need to log into multiple organisations.

For this to work, you have to have both client versions installed and use a launcher to log into the correct version. However, instead of using this tool, they are just using the same client but logging into an organisation assigned to the other version. The version difference between client and server causes crashes when using certain features.

Our software wasn’t initially designed for this since 95% of users just log in under 1 “organisation”, so support for this requirement has been retroactively added (the launcher), but it isn’t perfect at the moment.

There’s a team working on better support. Until then, we should instruct users how to use our software correctly, then release an update to stop this from happening.

So the “frequently occurring” error is that these users are not using the launcher, then encountering crashes when they try to use certain features. However Colin aims to “fix it” by adding some defensive code. All that will happen is one particular server call will now “succeed”, but there will be loads of other server calls that will have the problem.

If the user now sees a particular feature is now supported, but another feature isn’t, then surely they may ring Support and ask why it isn’t supported. Support will tell them they aren’t using the software correctly, but the user will say that we should support it because we have addressed similar issues before.

I discussed my concerns with Colin and he eventually agreed to cancel his changes.

Really, the endgame for Colin isn’t to please customers, it is to please the Head of Development who is aware of these errors. If he sees that the errors aren’t happening anymore, then he will be pleased. We need to make sure we are actually addressing the root causes of the issues, rather than hiding the problems.

Communication Breakdown – Reports

All our server calls are logged in a monitoring database along with their execution time. We generate reports in order to spot any negative trends and identify where we could make efficiencies. Additionally, we need the server calls to be under 2 seconds to meet our contractual obligations.

Ryan got told to update the performance monitoring because some of the calls had generic names like “Add” and no one knew which server call it actually was because there were more than one feature with the same name. After his changes, all the logged server calls have more verbose names to easily identify where the code is e.g. “Configuration.Users.Add”. However, no one told the people who generate the reports that they need to modify their report-generating program to take into account the new format.

So when the new update went live, the reports were broken because they are searching for the wrong method names.

There was a “Major Incident” call with managers and support staff who all had no idea what was going on. I invited Ryan to the call to explain the change. Ryan pointed out “this should be in the release notes”. However, when they checked, all they stated was “Performance Monitoring should be more traceable” which is not only vague but doesn’t highlight an action for a team to take.

There should have been some explicit communication between the teams, and the improvement agreed upon. It sounds like the feature was requested, but the team hadn’t known a developer was working on it and would have it ready for the upcoming release. Additionally, if there was some communication when the development started, maybe there were other problems that could have been fixed alongside this change for increased benefit.