Today, I saw some code that looked like the classic “I can’t recreate this bug, so I’ll write some code to log extra information, and I’ll hope this somehow helps me diagnose the issue”.

They had a list of data, and the ID in this list should be unique, but it was in there twice due to a bug. So it would look like something like this (ID =1 is in there twice):

private List<Code> _codes = new List<Code>()
			{
				new Code
				{
					IsSelectable = true,
					ID = 1
				},
				new Code
				{
					IsSelectable = true,
					ID = 1
				},
				new Code
				{
					IsSelectable = false,
					ID = 2
				},
				new Code
				{
					IsSelectable = true,
					ID = 3
				}
			}; 

Then their method contained this code to validate the data:

		private void EnsureValid(int idToFind)
		{
			try
			{
				_codes.Single(s => s.ID == idToFind);
			}
			catch (InvalidOperationException)
			{
				Dictionary<string, object> customData = new Dictionary<string, object>
					{
						{ "ID", _codes.ToDictionary(x=>x.ID) }
					};

                LogToErrorDB(customData);
			}
		}

The Single will throw an exception due to there being more than one matching element (Single will succeed when there is a Single match). In the catch block, they then convert the code list to a dictionary and will log this in the error database. However, a dictionary requires the keys in the dictionary to be unique. Since there’s duplicate IDs, this will throw an exception again, this time with:

“System.ArgumentException: ‘An item with the same key has already been added.'”

Error thrown in the catch block

So you get an error whilst trying to log your error. The extra information they intended to log is never logged.

I’ve seen this mistake happen a few times before, and usually you can test it by artificially creating the data condition. Then they would realise their error logging doesn’t work.

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.

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.