Category: Software Development

Blindly Following Code Analysis

Recap

We have a Code Analysis report which gives metrics on code quality, and we want to improve the overall quality of our code over time. Managers hype up this report and put great importance on the Code Analysis report. I’ve found that in an attempt to please managers, Developers are going out of their way to try and address these issues, but are making a mess. Often they just tweak the code which fools the Code Analysis rules, but it’s actually far worse to read and understand – and that’s the opposite of what Code Analysis aims to achieve.

The Problem

I was asked to review a project but it was an absolute mess since the developer had gone out of their way to change files they had no reason to change. So it’s just scope creep, increasing the need for more manual testing, and increase risk of introducing a bug.

I am all for refactoring code in order to improve it. It needs to be done properly, and best done when the testers were already going to fully test that functionality anyway – then you aren’t increasing scope.

Example 1: ToDo

Sometimes developers may put in reminders in the codebase to go back and fix/investigate a particular issue. They do this by writing a code comment, and can write the phrase ‘TO DO’. They should address these areas, and remove the comments before they check in. Sometimes this can be missed.

So the rule is “Complete the task associated to this ‘TODO’ comment.”

And the explanation of such rule is: “TODO tags are commonly used to mark places where some more code is required, but which the developer wants to implement later. Sometimes the developer will not have the time or will simply forget to get back to that tag. This rule is meant to track those tags and to ensure that they do not go unnoticed.”

So what you need to do is investigate why the developer added the comment:

  • Do we need to fix a bug?
  • enhance the feature?
  • or is it unecessary and we can delete the comment?

What this developer did was none of those options. They simply deleted the word “TODO”

//TODO: Probably the wrong event?

becomes:

//Probably the wrong event?

and

// TODO - may need to set other properties here

becomes:

//may need to set other properties here

So it doesn’t help the developer at all, but it stops the Code Analysis report flagging it because now it is just a normal code comment.

The report identify only TODO text to remove, so that did like.

Explanation from the developer

Example 2 – As

We had code with loads of if statements that checked the object type. The code had maybe 10 branches. This isn’t a great design as any developer that understands the SOLID principles knows.

This code sample is a simplification, and way more brief. Just imagine it having more branches and doing some exciting things within each branch.

if (animal is cat)
   Cat cat = (Cat)animal;
else if (animal is dog)
   Dog dog = (Dog)animal;
else if (animal is horse)
   Horse horse = (Horse)animal:
else if (animal is baboon)
   Baboon baboon = (Baboon)animal;

The Code Analysis report doesn’t like it because there is one check in the if statement, then a cast afterwards which is basically duplicating effort – it is inefficient.

The developer had changed maybe 5 statements, but had left the other 5 statements as there were – so it was just a mess of a design.

var cat = animal as cat;
var dog = animal as dog;

if (cat != null)
//do cat stuff;
else if (dog != null)
//do dog stuff;
else if (animal is horse)
Horse horse = (Horse)animal:
else if (animal is baboon)
Baboon baboon = (Baboon)animal;

So I challenged him and asked why change cat and dog, but not horse and baboon? I also felt there wasn’t even an advantage doing it this way anyway. We now have several variables where only 1 will be set, and the rest are null. I felt we should either keep it as it was, or spend more time refactoring it out to get rid of the branching, and adhere to the SOLID principles.

I just simply resolved some existing Code Analysis issues which not raised because of our changes. It is a kind of help to resolving existing issues, It is very difficult to resolving all the issues as of now

Explanation from the developer

But he has gone out of this way to change things that weren’t part of what he was working on at all! If it is difficult, then why only partially “fix” it? Obviously I had to respond to that to point out his stupidity.

if you don’t want to change code that isn’t relevant to your work, then simply don’t change it. These changes aren’t useful to a developer. The aim of Code Analysis is to write good code, and not reduce all the numbers down to 0 on a report.

Me

I was quite proud of that one.

It does frustrate me that I want the codebase to be high quality; but others are more concerned about looking good in front of the managers – even if the codebase suffers as a result.

Recently, I’ve felt like the “Gatekeeper” to the codebase. Luckily the Software Delivery managers trust my judgement to whether code needs to be reworked or not, so I can successfully block changes like this. It’s all the managers in the Quality Assurance that care about the report.

Poor standard and My Inspirational Quote

I saw a Pull Request (Code Review) from a Senior developer. They had created a new property along the lines of HasSetOption but there was already a property called HasOptionSet in the same file which was for the same purpose. I looked at how they were using their new property and they never even set the value; they only read from it; so it wouldn’t have worked anyway. He even had 4 failing unit tests for bonus points.

I told my Apprentice about it to show him that not everyone is actually smart:

“Senior developers, Junior developers. They are all the same really”

Me

“From a learner’s perspective, that is both scary and reassuring lol”

Apprentice

“Yeah. Doesn’t matter how bad you are, you can probably find a Senior that you are as good as :laughing:”

Me

I thought that was quite an inspirational quote. It’s sad that it’s actually true.

Mentoring #4

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. After spending a bit of time with him, he then got involved in more form-filling and exams for his Bootcamp company. It seems weird that we probably paid a premium for the Bootcamp company to train him, but yet he has been working with us for about a year now (I think) and we have barely managed to do anything with him. I really hope we don’t hire from those Bootcamp companies again. You may as well just hire someone and train them up yourself.

The good news is that my Apprentice has now “passed his apprenticeship” which means he has no more dealings with the Bootcamp company and now can work with me full-time.

At the moment, I’ve given him some Junior developer tests that employers normally give before an interview, so he is working his way through those. Ideally, he should have done them in an hour, so I guess he is a long way off from a Junior standard since he has had them a few days. But hopefully, I can train him well, so I’m quite excited for that.

Another complication is that my team has been assigned another Apprentice and my manager asked me if I could also train him. I made a great point that we have loads of Seniors in the team, so why aren’t they being asked? He said it was a good point and would look at promoting me. We will see how that goes.

Here’s a small summary of aspects I have gone through with my Apprentice. These are C# or Visual Studio topics:

  • Use some shortcuts like F12 to navigate methods
  • Ctrl+t “Go to type” shortcut that seems way better than Ctrl+f to find code
  • Use the “Rename” feature to easily rename variables/methods/classes setc
  • Break large methods into smaller ones – use Visual Studios “Extract method”.
  • Using breakpoints, the watch window, immediate window, “tracepoints” to debug. This is very important, you can never be a good developer without being good at debugging.
  • What the “Call Stack” is, and how to navigate through the method calls using Call Stack window.
  • How to create a branch in Git, push it to the server and create a Pull Request.
  • Class inheritance and polymorphism. 
  • When to use Abstract classes and methods.
  • The difference between “hard” casts and “as” casts.
  • Using some basic Linq (Select, Where, OfType).
  • Creating basic Winforms, and how to use Dock and Anchor to automatically resize controls as the form resizes.

The Dialog

I logged into our application and was greeted by a new dialog. When I see something new, I normally scrutinise it, and check out its functionality. I guess I’m quite inquisitive.

The thing is, the dialog also caught my attention because it looked really odd. 

My eyes were instantly drawn to the link which was in a strange position, and the font looked very large. I loaded up the code to see what was going on. Usually, we use Tahoma 9, but this dialog was using a combination of “Arial” and “Microsoft Sans Serif”, then the link text is 10.5 rather matching the other 9.5 (which is wrong anyway because it should be 9).

The buttons look a bit dark: they are using “Light grey” rather than the standard “Control Light”.

I also thought the Email label was a bit far away from the text box. The buttons are quite far away too.

So I quickly made a few minor tweaks to illustrate how I thought it should look like.

I probably should have left the text in bold, but I had already deleted my changes after taking the screenshot

It’s not perfect, but closer to what I expect to see. It’s easier for me to judge because I’m used to our application. Moving from the original dialog to another looks jarring since the fonts were clearly different sizes, whereas you readers have no comparison.

How does an entire team of developers and testers not notice these differences? I quickly checked the other dialogs they added and saw similar problems there. Sometimes spelling mistakes, grammar mistakes, randomly double spaces between words, and some uncentered text:

why not centre the text vertically?

It’s a complete lack of attention to detail by the entire team really.

I contacted the team, which was led by Colin who is infamous for his lack of attention to detail. His response was that he “was just following what the User Experience team gave him”. I checked the documentation he shared, and the mock-ups looked like how I imagined them to be – and not how his team had implemented them.

One of the Testers said something along the lines of “well, we debated these issues within the team, but ultimately – the Product Owner said it was fine”. 

When they did “fix” it, the developer didn’t send the review to me, but I noticed it and jumped in. It had already been approved by a member of their team, but was it actually good? Would it meet my standards? 

No. He had barely changed anything. 


The fonts were still the wrong style and size, the buttons were still the wrong colour, they were positioned incorrectly. He had even changed a few things no one actually told him to change – so made it worse.

Winforms Dispose Controls Quirk

Here is an interesting quirk with Winforms in C#. Calling Dispose on a Control has a side-effect of removing it from the container it has been added to.

So you can have code like this:

for (int c = 0; c < MainPanel.Controls.Count; c++)
   MainPanel.Controls[0].Dispose();

which correctly removes and disposes every control, even though you have a fixed index of 0. Since the control at index 0 is removed (as well as being marked as Disposed), the next control then becomes index 0. Therefore, it does call Dispose on every control.

So when you look at this code:

foreach (Control item in MainPanel.Controls)
{
   item.Dispose();
}

It looks correct, and would be the code that I would write had I not known about this quirk. However, it doesn’t work as intended. It will only remove and dispose some of the controls since you are modifying the list as you are iterating through it. 

This is the danger of adding “side effects” to your code. It can cause weird behaviour that you wouldn’t expect, and can be overlooked if you are just reading the code.

Age

There’s been a few occasions now where I have seen developers write code to calculate someone’s age and it has been incorrect.

I was talking to a developer, Alan who was ranting about some incorrect code, so I looked at the review.

string ageInMonths = person.AgeInYears < 6 ? (person.AgeInYears * 12).ToString(): String.Empty,

So there’s an ageInMonths variable. If they are less than 6, then we populate it, otherwise we leave it blank. When we calculate the persons age in months, we simply take their age and multiply it by 12 since there’s 12 months in the year.

Alan begins by pointing out the problem, but Pierre doesn’t understand what he is referring to, so Alan backs it up with more examples…

Alan: Is this correct? If I am 2 months old, I am 2 months old, not 0 month old?

Pierre: yes, it is optional for people less than age of 6

Alan: I don’t think saying someone is 0 months old when they are 11 months is correct. Nor is saying 12 months old when they are 23 months old

Pierre: thanks, will correct it

This isn’t something that is a language barrier. It’s not even a lack of programming experience. It is simply just a lack of common sense. You just have to be human to realise this isn’t how ages work.

He clearly didn’t test it because it should be obvious the correct values weren’t showing. We could instantly see it was a problem just by looking at the code. We didn’t need to see it in action to realise that.

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.

Wingdings

One of my all-time favourite bugs I encountered as a Software Tester was when a printout was written in Wingdings font.

The developer had wanted to utilise the check/tick character ✔ from the font, so they had programmatically changed the font, then inserted the character. However, they didn’t add the code to reset the font back to normal, so the rest of the printout was also in Wingdings.

I do wonder how people manage to introduce such bugs, because surely you will notice it if you have tested it. The only exception is: if all your tests have scenarios where the Windings character is the final character on the printout.

I really thought it was a once-in-a-lifetime bug and I would never see the likes of Wingdings grace our system again. However, I was wrong, it happened recently with a project involving some document functionality enhancements. Luckily in both situations, the Wingdings bug never made it to the live environment since it was easily spotted by our Testers. 

I bet our users would have loved it more than I did (sarcastic, obviously).

Configuration Switches

Recently, we were running into Out Of Memory issues with our software. A developer had identified that a feature which launched an embedded browser was using up quite a bit of memory, and we could move it into a separate process which would have its own memory allocation.

We followed his recommendation, so we moved the browser into a separate process and released this software (which I will denote “V1”). This caused an issue because the dll wasn’t built with the correct settings. We needed a quick fix due to the number of complaints from our users. It would be easy to fix, but due to the Testers stating it would take a few days to test it again, the managers told us to simply revert it and give the users the memory issues again (so this is “V2”).

We then fixed it, but since we had more time, we decided to implement a configuration switch so that if it went wrong, we could just disable it without having to release a new version. I made a small refactoring to get the configuration switch working, but I made a small mistake in the constructor which caused a crash in one of the places where the browser is launched. A Tester had found it late in his testing. I came up with a fix, but then we got called into a meeting to discuss what we needed to do. Unfortunately, my new fix would require a lot of the testing to be redone. The “refactoring” I did was brought up for discussion, and an experienced developer said that my way of doing it wasn’t the way he imagined. All the managers on the call trusted his judgement and felt I had gone a bit overboard, so they insisted I redo it.

So I had another look and couldn’t see how I could possibly do it without refactoring at least slightly. I did the best I could which I’d say was actually ~90% identical, apart from it was clearly worse from a code-design perspective. I showed it to the experienced developer and he said that was how he imagined it. I was baffled, but this is what the managers asked for. The release got delayed a week because the tester had to retest everything. So in the end, we had just wasted days of time to make the code worse. Brilliant. If we had gone with my original fix, it would have taken the same amount of time, but the code would be better.

Additionally, we did highlight we couldn’t test all the third-party suppliers we integrate with, so we had “low confidence”… or there’s some uncertainty at least. However, the Directors got involved and they wanted it out. So out goes “V3”.

There was a problem with just one of the third-party suppliers, but for some reason, Support left the feature enabled for several hours, when they could have reverted it with my Config Switch. This led to many complaints (yet again), and therefore panic from the management. When it came to disabling it, despite being able to disable it for particular users, they turned it off for large groups.

We rollout our changes in phases, so the Software Delivery managers wanted me to rollback the changes once more before releasing to the next group of users. I asked why we couldn’t just use the Configuration Switch, and the response I got was “we don’t trust Deployment to turn the feature off. What if they miss some users?”

Well, when those users complain, you just switch the Configuration Switch to “off”. If we rollback again, then we are back to square one; where users can encounter the memory issue.

What is the point in coding a Configuration Switch if they won’t use it? I did point out the way the Configuration Tool works is that you could set the configuration ahead of time (the previous software version will just ignore the extra config), then when the users get the new version, then the updated version would use the configuration file as intended. So with a bit of persuasion, that’s the option they went for. However, they turned it off for every user, even if it wouldn’t have caused problems for them. 

When will they turn it back on?

Conclusion

It was actually a good idea to use a Configuration Switch. If the feature doesn’t work properly, you can quickly revert to the old way (well assuming your new changes don’t unintentionally break the existing feature). Obviously, you need to trust the Deployment team to actually use it correctly.

Colin went with a Configuration Switch for his recent project. How did he get on?

“The bug can’t be introduced by our project because it is switched off on live”.
5 mins later…
“This is definitely code that we changed”

Colin