Category: Software Development

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 truefalse
Btrue false
Cfalsetrue
Dfalsetrue
“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”
Afalsetruetrue
Btruefalsetrue
Ctrue true true
Dtrue 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!BC!D A || !B || C || !D
Atruetrue false true true
Bfalsefalse false true true
Cfalse true truetrue true
Dfalse true falsefalsetrue
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.

Shots Fired On Inexperience: Leaked Transcript

Intro

Even though all software developers should take part in code reviewing their peer’s code, I find most developers I work with aren’t that interested, so ultimately it comes down to a few high-ranking developers, plus me.

Recently, we have been rejecting a lot of bad code from inexperienced developers. It’s fine being inexperienced as long as you have a team to support you and help you eventually produce an effective solution. I think what happens is – there’s not enough seniors (or seniors that are actually good), so the code gets into their Project Branch. Then, when it is time to merge in for the Main release, experienced developers review the code and reject it.

I often specifically get asked by the Software Delivery managers to review code to ensure it is in a good state for the release. We had a group conversation where we were discussing the current bad state we are in. Software Delivery managers had asked Darren, Dave and me to review a project.

Participants

Darren, Dave – Principal Developers

Mark, Michelle – Software Delivery Manager

Me – the silent observer

I’ve given the developers names that start with D, and the managers names that start with M, so it might be easier to follow.

The Conversation

Darren  08:36

Whole lot of Nope in this Code Review.

Mark  10:52

Judging by your review comments, this doesn’t look to be something that can be resolved in the next couple of days. Does their implementation need to step back and restructure?

This might need another call

Have any of you guys been involved from a high level view for this Project?

Darren  10:55

The changes to the Database are totally inappropriate, and possibly way off the mark of what is needed.

Changes to the Database filing are non-trivial, if people don’t understand it, they need to reach out for help, not just smash code in.

Our product is going to turn into a legacy codebase very quickly if this behaviour carries on.

Dave  11:11

We do keep getting Code Reviews to Main with unworkable solutions. It’s highly frustrating, and a big waste of our time. Most of the time, the code or even the work item has not been reviewed by an expert-level person before a review to Main is requested. On occasion, someone gave correct advice in the first instance which was then ignored.

We need to fix this, and have help from you guys raising the problem.

Darren  11:14

The solutions feel like a very reactive approach to programming… Very little thought for design.

Mark  13:02

So have any of you guys been approached about this Project before this review  was raised?

Because I think this ties in with the whole idea of needing an Subject Matter Expert during the construction of the project rather than at the very end – something that we’re trying to drive forward

Dave  13:07

only about 2 days ago 😖

All our Subject Matter Experts are on the New product 😛

Darren  13:08

I reviewed the previous incarnation of it, without the concerning the Database changes.

“All our Subject Matter Experts are on the New product” Even so, I might have made time to discuss this with someone if asked.

Dave  13:09

yeah same

My point is that the day-to-day design and building of the projects is led by inexperienced devs. The developers are not being mentored while developing the code – but instead, at the end of their code writing; which should be something a lead developer is doing

It’s good to have the view throughout the dev process, or at least the start, as well as quality checks at the end – which should be a straight-forward “yeah, you’ve not done anything crazy, it meets the coding standards and is what we agreed”.

Mark  14:43

Yeah. This is all the good stuff which me and Michelle are trying to address as well. Trying to have a Subject Matter Expert assigned to each project during construction just for that overarching view, and to give a steer when needed.

I.e. a month ago this could have been caught at the start by saying “don’t make these changes within the Database itself” haha.

Darren  14:51

To be fair, you don’t need a Subject Matter Expert to tell someone not to dump a load of copy and pasted code from one module into another. Just someone with some experience in developing software to a higher standard than “Hello World”.

Michelle  14:52

“Hello world” just triggered some terrible school flashbacks.

Dave  14:52

Agreed, but as a business, we need Seniors to point out when people are constantly doing silly things to their line manager.

Michelle  14:57

It’s the experience aspect that is the biggest challenge, if there was a better ratio of experienced people to inexperienced people areas where skills are not good enough, they would start to improve (or at least that’s the theory).

Everything just seems stretched way too thinly at every turn. Hell, it isn’t just development that has the problem.

Darren  15:00

I’d also add: knowing anything about the teams working on the code would help.

I can’t know everyone, but I don’t even know what the teams are, what they are working on, who the seniors / leads are to discuss with.

E.g. if someone has a problem with code my team is doing, then I’d expect to be brought into the loop (either to explain our position, or to understand the other sides)

Dave  15:06

Who do we talk to, to at least pass the buck on the issue and raise it?

Michelle  15:11

I think it is more for the Development Director. It’s something me and Mark have brought in fashion everytime we come across an example. Difficulty has always been what to suggest to fix it within the limitations of what we got

Dave  15:21

Certain business types don’t understand the value of good engineers and will simply get the cheapest money can buy, in the hope the decent engineers will act as quality gates and stay with the company.

It must be soul-destroying, working away in an office across the globe, with ‘experts’ in the UK saying if your work can or can’t join the rest of the product. It’s not the right thing to do.

They need infrastructure mirroring ours, and separate projects would be a great way to achieve it

Darren  15:25

Last I knew, the other office still have Development and Testing as separate departments

Dave  15:25

This just reinforces the stereotype that Ben bangs on about: the company is ruled from the UK. Normal companies have different product sectors at different locations, because it mitigates these problems

Michelle  15:27

All valid points. What Dave is saying are points raised verbatim by more than a few people who have been in this world for a while. As for soul destroying: 100% agree it has to happen to catch this stuff; but each time it’s caught it will knock you back that little bit more

Just needs some kind of sensible structure that is adhered to. But then again – if no one is made to follow something, who will?

Dave  15:30

Yeah, for me, it’s responsibility, if you don’t have to pay for your actions, then it drives bad behaviour. But since we maintain our Product, having to pick up the bugs, see the customers complain on Social Media, see the error reports on live and backlogs grow, we’re a lot more invested

it is feeling more and more like, knock this feature out as quick as possible, throw it over the fence at UK law-enforcement, see if it goes through

👮

With responsibility you’re driven to get the right structure in place, you want to place experts/leads with the teams and have them monitor each day, because it soon becomes a very long task to get fixed last-minute. Top-down approach, drives the 9-5 job behaviour, lack of interest and minimum viable work ethic.

Darren  15:32

+1

Which goes back to my point about who the teams actually are. And we really need to do proper ownership. I still own large bits of our Product, because I care about them!

Dave  15:34

Same. Developing and code reviewing isn’t my job, really. However, I don’t want to see the product damaged and go to waste, because it helps people, genuinely.

Darren  15:34

Would be so much nicer to say “I don’t look after this any more, just do what you want and hope nobody gets sued” 😣

And it isn’t any one change that is unsafe, it is the poor quality, lack of adherence to existing designs, just piling code in, coupling stuff together, changes in the wrong place, etc that lead someone else to create a unsafe bug later – usually without anyone batting an eyelid.

OK, some changes are totally unsafe too – a few weeks ago, I have to point out that you cannot calculate an age in months by doing ageInYears * 12

😖

Dave  15:48

So, if I were to write some general comments without mentioning names, and send them to the Development Director, expressing that we’re at our wits-end, would anyone be prepared to back it up or be a part of the revolution?

Darren  15:50

Feel free to add my name to give clout to this

Dave  15:50

sure, I will of course send to you first to check

Summary

  1. Not enough people willing, or have the ability to review code. – I always find this a weird one; given that I reckon 90% of a developer’s job is reading existing code. If you cannot review code, then can you be a good developer?
  2. People aren’t putting enough thought into the architecture, or adhering to well-known standards like SOLID Principles – This is the kind of behaviour I saw in developers I’ve written about like Colin and Derek.
  3. The ratio of Juniors to Seniors is far too high, which not only is a problem now, it is a problem in the future when the Juniors haven’t progressed enough.
  4. The rapid hiring of staff has meant you don’t know who people are, what they are working on, and what their level of experience.
  5. Hiring cheap staff has caused some resentment/uncertainty with UK staff. Pay seems frozen and more jobs are moving abroad. It wouldn’t be that bad if the staff we hire are cheaper and are good, but they often just appear to be cheap.
  6. It would be better to split the offices into separate products so people feel like they have full ownership. There’s other problems too like time-zone differences and general communication problems.

More Bad/Weird Code

This is a follow on from my previous blog. I picked out a few examples that were bad changes from their Code Review, but maybe you didn’t think it was that bad in the grand scheme of things. So here is a collection of other weird stuff from the review.

Example 1: TrimEnd

TrimEnd method can be used to remove trailing whitespace, so if you have some text “Example text “, you can trim it down to “Example text”. You can also pass in an array of characters you want to remove from the end as well. So what happens if you create a completely empty array of characters?

return value.TrimEnd(new char[0]);

I had to test it out to see what it actually did. I was thinking there was a chance it could prevent the method from trimming anything at all. It actually does the same as TrimEnd() so removes whitespace only. So the char array is just redundant and looks confusing.

Example 2: The Weird “Any” check

Give a C# programmer a list called “textToProcess” and ask them to check if the list is Empty, and they will write this:

List<String> textToProcess = GetData();
return !textToProcess.Any();

or alternatively:

List<String> textToProcess = GetData();
return textToProcess.Count == 0;

But when you see this, then questions need to be asked.

List<String> textToProcess = GetData();
var empty = new List<string>();
return empty.SequenceEqual(textToProcess);

If you are a Junior and you are reading a book, working through tutorials, or even Google how to check a list is empty; what is the chance you will come across SequenceEqual? I’m sure I only came across that after a year of C# development. Weird choice.

Example 3: Unnecessary Converting

If we have a variable that stores a number. And I say that I want it to be a number. What are you gonna do to it? If you thought “nothing, it is already a number”, then you are correct.

If you thought something else, then maybe you did what this developer did. You could convert the number into text, then convert it back to a number!

This “number” variable is already a “long” aka Int64. They really are converting it to text, then instantly converting it back to the type that it already was. I really can’t emphasise how pointless this is.

Convert.ToInt64(number.ToString())

What’s weird is that I have seen this a few times over the last couple of months. Why are people doing this?

Example 4: Reinventing a private setter

There was a simple, standard property that you see all the time in C#.

public Validator Validator { get; set; }

And he changed it to

public Validator Validator => _validator ;
private Validator _validator  { get; set; }

So now the property is using this newer property syntax that looks weird to me. But really it is a property with a “get”, but no set. Then the second line is named as if it is a field (preceded with an underscore and first letter is lowercase), but has a property definition (get/set). Properties should be public but this is defined as private. This is incredibly weird.

Here’s how it should actually look (just has the word “private” added):

public Validator Validator { get; private set; }

So I comment on the second line:

this is named like a backing field, but you have defined it as a property. Do you need a backing field? couldn’t you just have changed the set to be a “private set”?

Me

Yes we need this backing field and it won’t allowed to set as private set

Developer

I had no idea what he was trying to say here. It definitely can be a private set. Plus there is no backing field, (just that he named it as such), and I had wondered if that’s what he was trying to achieve.

It isn’t a backing field, it is a Property that you have named as if it was a backing field, then the Validator just calls your new property. I think this change means Validator essentially only has a “get”. So surely you can achieve the same thing with public Validator Validator { get; private set; } then remove “private Validator _validator { get; set; }”

Me telling him exactly what to do.

Yes you are correct, but here we never wanted to do any changes in existing flow since it will give huge impact, so that used the different one without touch any of the current flow.

Developer

Just like the example in the previous blog, I tell him he has changed far too much and tell him to revert it/fix it properly, then he starts telling me how he doesn’t want to make more changes than he needs to. Most confusing.

I don’t understand what “existing flow” means here. Your changes have already removed the public set from Validator so no other class can set it. You’ve just done it in a way that people don’t expect by also changing it to call another property (which is named like a field). This is just confusing.

Me

He finally changed it, but it took some effort.

Example 5: Unit Testing

There was a method that returned an object that you could get multiple properties from. However they had created 2 separate methods that you call instead which was inefficient when you want all data. 

public string GetForename()
{
   return GetUser().Forename;
}

public string GetSurname()
{
   return GetUser().Surname;
}

When GetUser is a server call that reads from a database, why can’t we just grab the user once, then take the info from it as desired. We shouldn’t duplicate the server call.

User user = GetUser();
string forename = user.Forename;
string surname = user.Surname;

Why not just call the server once and use both properties?

Me

We made 2 method calls to get unit test coverage

developer

What is he on about? I could only see 1 test, it was for a completely different method and it didn’t really test anything.

        [TestMethod]
        public void CreateOutputFromDataTableTest()
        {
            DataTable dataTable = new DataTable();
            dataTable.Columns.Add("ID", typeof(string));
            dataTable.Columns.Add("Code",typeof(string));
            dataTable.Columns.Add("Term", typeof(string));
            
            dataTable.Rows.Add("9908", typeof(string));
            dataTable.Rows.Add("44D6", typeof(string));
            dataTable.Rows.Add("test", typeof(string));            

            var output = Transformer.GenerateOutput(dataTable);
 
            Assert.IsNotNull(output);  
        }

They are creating a table with 3 columns. They are then creating 3 rows but only populating two columns. You can tell by the data that they seem to want to create 1 row but with 3 columns set.

Then after they retrieve the output from their method, instead of actually checking if it has specific data they expect, they are checking if it isn’t null. So it could be any type of object: could have no data in any of the properties, it could have the wrong data in the properties. As long as there is an object it returns, then the test passes. If you are going to write such nonsense, why not make it brief like this:

        [TestMethod]
        public void CreateOutputFromDataTableTest()
        {
            var output = Transformer.GenerateOutput(new DataTable()); 
            Assert.IsNotNull(output);  
        }

Example 5: Incorrect Renaming

public bool IsQueryRunnable(Search search, out string message)

to

public bool IsQueryRunnable(Search searchGuid, out string message)

why was search renamed to searchGuid?

Me

The parameter name in the interface and in the implemented class should be same.

Developer

I think Code Analysis does flag that as a rule. But if the “interface” had the declaration with “Search searchGuid”, then surely the logical change would be to change it to “Search search”, because as I pointed out

but it isn’t a Guid

Me

Conclusion

This person is probably a junior. When I was a junior, I would ask for help when I was unsure – I would never claim incomplete code was production ready. I would also make sure my team members reviewed my code and gave me good feedback before I would want other teams to see my work. This work just seems careless to me.