Terrible Developers

Last week, I was reminiscing to myself about the developer that changed a random line of code which clearly wouldn’t have fixed the bug he was working on. It was also the same developer that repeatedly checked in “fixes” in an attempt to fix the build: Using the Build Server to test your code.

I said to my Apprentice, “how do you get into that mindset: where you write code, don’t even check it works then send it to your team to review?” but also “what happens if they did approve it and you are then embarassed when the Software Testers tell you it doesn’t work?“. Or even worse, “what happens if your bad code somehow went live?“

I wasn’t sure if I’d see something like that again, but it seems like we have hired some terrible developers in recent times.

Today, I was having a nosey at Pull Requests (aka Code Reviews) and saw some changes that seemed like the developer hadn’t even built the changes, therefore didn’t run their own unit tests they wrote, or done any kind of manual testing.

When you create a Pull Request, it triggers off an integration build. Then you can only complete your request if you have a successful build, and your team members have approved the change.

First of all, the build was failing, and it was a simple syntax error which Visual Studio would have highlighted, and definitely would have been brought to their attention if they clicked the Build button.

Looking at the code though, the first thing that I thought was funny is that they had a Code Analysis error. When you get these errors, most of the time you have to fix the problem it is highlighting. In the case that Code Analysis is wrong and you are smarter than it, you can add a Suppression. To make it clear why it is suppressed, you can add a Justification parameter and add your own explanation to it. To force people to do this, we have added a Custom Rule “SuppressionsMustBeJustifiedRule“. But the sneaky developer went and suppressed that!

[System.Diagnostics.CodeAnalysis.SuppressMessage("Custom.Maintainability", "CM3000:SuppressionsMustBeJustifiedRule")]
		[System.Diagnostics.CodeAnalysis.SuppressMessage("Microsoft.Reliability", "CA2000:Dispose objects before losing scope")]

I then looked at their unit tests.

bool isValid = validator.IsValid(input);
Assert.AreEqual("True", isValid);

I was convinced that would give an error and it certainly does.

Assert.AreEqual failed. Expected:<True (System.String)>. Actual:<True (System.Boolean)>. 

Comparing the wrong datatypes. A Boolean is a true or false value but it cannot be compared to a String (text) representation of it (“True” or “False”). What they actually need is simply:

Assert.IsTrue(isValid);

If isValid is true, then the Assert passes. If isValid is set to false, then the test fails.

Then the next set of tests were like this:

string successMessage = string.Empty;
List<string> errors = validator.Validate();
if (errors.Count() != 0)
    successMessage = string.Empty;

Assert.AreEqual(string.Empty, successMessage);

So Validate returns 0 or more errors. If there are 0 errors then the test passes because successMessage was initialised to string.Empty and we are comparing it to string.Empty. If there are 1 or more errors, then we explicitly set successMessage to string.Empty (which hasn’t actually done anything because it was already initialised to string.Empty), then we compare it to string.Empty and the test passes. So no matter what Validate does, the test will always pass.

What an absolute waste of time.

I also said to my Apprentice that it is perfectly fine to have a go and writing some code but failing. Then you can say “here is what I have wrote, it doesn’t build but I don’t know why, can you help me?”. Then I can go through it, give him some advice, and together we come up with a good solution.

The worst thing you can do is give up, but then try and get the crap code checked into the codebase.