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.

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.

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.