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.
Stranger Coding Tings 