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.

Bad Quality Project

Recap

The Code Analysis report gives metrics on code quality, and we want to improve the overall quality of our code over time. We put great importance on the Code Analysis report even though we haven’t done anything to address the issues. It’s just that certain managers keep hyping up how important it is.

New Project

An important project is going into the Main branch which will be released next month. I was asked to review the code. I was originally told the review would be sent by the end of the day, but it was delayed a day. I proactively found their code branch and saw it was out of date by a few months, which isn’t a good sign. (They should be merging Main into their project branch to minimise code conflicts).

So after they finally updated it to the latest version and sent the review, a manager said they “really need to get the project reviewed and approved by 5pm so they can start the regression testing tomorrow”.

I jestingly asked if the expectation is that ‘I click the “Approve” button without looking at the code’ – if I criticised their code in any way, then they would miss the deadline since they would have to make the changes and retest those features. It was true though, the deadline was about 3 hours time.

Since we are more ethical than that, a few of us ripped it apart. Initially, we ended up with around 100 comments which I think is the largest I’ve seen on a review. The next day, we added even more to get it up to around 130.

If they really wanted to cut corners, then it’s up to the managers to decide whether they want to skip the review and check it in anyway.

I thought the pressure they were putting on the reviewers was unfair. It wasn’t just that initial message (“need to get the project reviewed and approved by 5pm”), but they kept on sending direct messages to each of us asking when we will be finished because we are pushing time.

Code Critique

Instead of moving some old code to a new location, and refactoring it so it is reusable with the new features (utilising C#’s object-oriented style (base and derived classes), then fixing up the existing issues to meet our coding standards)… they decided to copy and paste it instead.

So now we have some old, bad code; and some new, bad code. When the Code Analysis report runs, it is going to flag up issues in both files, and if it successfully identifies the duplication, we will have loads of Code Duplication errors too. Brilliant.

I expressed my concern to the team’s manager and he gave me a response along the lines of “we were a bit pushed for time, so decided to take the easy route”.

On the review, I questioned some of the messages we were displaying to the user, and the response I got was “this is an existing message”. Well, fair enough to a point, but why didn’t anyone question if it should be changed. You are revamping that area of the system, why don’t you improve the quality and user experience whilst you are there? The project has enough testers allocated to it, and they will be testing these changes in detail. It’s a great opportunity to rewrite it.

There were several files where their response was “actually, this file isn’t used, so I’m deleting it”. I’m not sure how that can happen with several files. Why only realise now?

Closing Thoughts

After all those discussions about how our team is supposed to have Quality as our main focus, then we proceed to make a mess of things!

It would be nice to have some consistency. If I check in some code that shows up on the report, then I get an email from someone. It doesn’t matter if I have reduced the problem count by 50, but added 1 new problem – they make out it is a problem.

Why is someone manually emailing about what a report shows, rather than having it as an automated check as part of the review? Maybe it’s a licencing issue. If they were that serious about this report then they would pay for it and enforce it properly.

There’s been a few times where projects were supposed to be merged in, and the people doing the code review didn’t like the code. If you are doing a project, maybe get an expert to review your code earlier in the project. That way, you don’t get loads of issues to fix on the day you are supposed to merge it.

Team Summary

The team is showing unwavering commitment, innovation, resilience, and drive to deliver continuously. I’ve never seen anything like it in the years I’ve been here.
Head of Development

I always say that managers often show a massive disconnect from reality. They seem to praise when the team has performed poorly, and I think this is another case of that. I went through our team’s changes in the last month. Merges are trivial and I don’t really count them as real work, but I’ve included them in the table anyway.

Here is the analysis:

Name	Merges	Fixes	My View
Me	1	4	I’ve been disappointed in my productivity. However, I haven’t done that bad when compared to everyone else. Additionally, I have got loads of changes in a branch, on hold; awaiting confirmation of actual requirements.
Senior Dev	1	11 (mainly small items)	Decent work, but really only cherry picking “quick wins”
Colin	4 (made a massive mess of one of these, made a bit of a mess with another. I think the other two were primarily to teach people how to merge [ironically].)	5 (I believe 2 of these fixes were issues he created in the other 3)	Colin wasted a lot of time by making a mess of a merge which introduced at least one bug, and he made a mess of one of his other changes. He claims he has been more productive whilst working at home.
Colin 2	0	3 (1 of which was removing an unused using statement)	With only 2 real changes, one of which was small and it didn’t work properly; this is terrible performance.
Rob	0	2	Rob has been real quiet. I think he had a bit of time off work, but still, this is poor
Beavis	0	1	Beavis was primarily just making excuses why he can’t work. To be fair, there was one week where he was doing some other work for another team.
Total	6	26

Our “drive to deliver“

So in a month, we haven’t done much work. I’d be interested to calculate what the average amount of fixes developers do each month. It’s a bit meaningless because a fix could take an hour, and another fix could take a week. However, you would think it would average out over a period of time.

I’ve never seen anything like it. I might use that as a recurring phrase from now on.
My manager actually said to me that my efforts “haven’t gone unnoticed”. I didn’t bother to correct him. I think I need to play up the delusion to try and get a promotion. Honesty never got me anywhere before.

The Code Analysis Meetings

Our Team Lead had been hyping up how important it is to improve the statistics of our codebase. So we have a report that runs and gives you metrics about duplicated code, “code smells”, possible bugs, test coverage etc.

Colin volunteered to lead on this. I know that he is just trying to impress our manager to try and worm his way into another undeserved promotion. The thing is, he isn’t even thinking about what he is doing.

There’s been 2 meetings per week, and it’s been going on for 3 months. I wanted to know what was going on because we haven’t heard any outcome from the meetings, and the statistics haven’t improved either.

Intervening

I asked one of the attendees if they had discussed a particular rule that was flagged by the report, and told him that if we suppress the rule, it will improve the metrics by 300 or so. He told me to come to the meetings.

So I join the meeting, and ask them to investigate the possibility of suppressing this rule and people seem to think it is a great idea. No one had any other new ideas, so then Colin said we would continue going through the report.

I asked him to explain the process since it was my first meeting. He said there are different severities like low, medium, high, critical, and managers only care about the two highest categories. So he was going through each rule and commenting on them saying they can be changed to Low, or that we need to fix them.

So we start looking at each rule violation, and there’s about 8 people on the call; mainly developers but a few interested testers, and 2 managers. I say “interested” because they have turned up to the call, but when Colin asks people to discuss it, no one can make a decision. Colin kept on trying to describe what the code was doing but made so many mistakes, I had to correct him several times.

I was basically taking over, but kept on asking for other people’s opinions, but people seemed pretty indifferent. The call had gone on for nearly an hour and a developer hadn’t said a word, so Colin accused him of being asleep. The developer said he was awake, but then continued to stay in silence for the rest of the call.

I was told there was another meeting in a few days time, and it would be more technical, so I agreed to join.

The Great Realisation

That meeting was basically the same people, but excluding the managers. Colin continued going through each rule violation again. The conversation that followed roughly went like this:

Me: “Hang on, this is taking forever. We are seeing the same type of rule violation and you have gone through about 15 of them now.”

Colin: “Yeah, there are loads”.

Me: “This seems like it is gonna take all year. How many violations of this particular rule are there?”

Colin: “4000”.

Me: “Hang on, let me get this right. You are going through each of the 4000 violations, analysing the code, and trying to decide if you should mark it as Low severity or not?”

Colin: “Yep”

Me: “Don’t you think it is stupid?”

Colin: “That’s what managers want us to do”

Me: “No, the managers want you to decide how important the rules are, not how important each particular violation is. If they were aware there’s 4000 of the same rule, I’m sure they wouldn’t want you to do this. Why don’t we just look at the rules? How many rules are there that are being flagged as High or Critical”

Colin: “5”

Me: “So you have had about 12 meetings to discuss 5 rules, and you haven’t even gone through 1?”

Colin: “Yeah”.

Absolute waste of time. Just think, if there’s only 6 people and you paid them £12 an hour, and had 24, 1 hour-long meetings that they attended – that’s a cost of £1,728. But then sometimes there would be more people attending, and you had the expensive managers too. What did they achieve until I came along? Nothing. No one questioned it.

The thing is, we are concentrating on the wrong thing. There’s plenty of bugs to fix which bring value to the users. Why are we wasting time tweaking things that don’t need to be changed? The crazy thing is, we are spending all this time talking about doing it, and it isn’t being done.

The Efficient Team

In the next meeting. I told the manager our new approach, and also raised some of my concerns with how much importance we are placing on this. She didn’t care about my concerns so that got ignored. What she did say was “you guys are becoming an efficient team”.

Is that even serious? Months of wasted time. The words sound sarcastic to me, but I think she was being serious; the tone sounded sincere.

So in conclusion: Colin was just trying to impress the managers by taking the lead, and so was everyone else really; they were on the call, but they really had no passion for what we were doing.

Report Ideas

Our Team Lead said that Colin had some great points and would be presenting them in our meeting.

and Colin’s points were

Can we add rules?
Can we remove rules?
Should we document our changes?

Which are all basic questions and not any kind of guidance. Obviously, the answers are

obviously,
obviously,
and the majority of the time, no; that would be overkill

He also did a demo to illustrate what we can include in our documentation. The “code smell” was that there was the same assignment twice like:

somethingEnabled = whatever!=null && somethingElse!=null

otherFeatureEnabled = whatever!=null && somethingElse!=null

So the assignment logic is the same. Then he said the solution is to create a new variable. Yes, that is obvious…. but he has some timing tests to illustrate the performance difference of 0.00025 seconds. Since he has proven that this is the correct thing to do, we now must document that as the official solution.

Embarrassing. So cringey.

Then the next day, Colin was talking about his work item and said if he has permission, he would also fix the “code smells” that were declared by the reporting software. So the Team Lead asked him what the “code smells” were and Colin said “unused using statements”.

Since when did developers ever declare/ask for permission for something so trivial? We just did it because it’s obvious. Even Visual Studio tells you to do it. “Unused usings” are shown in a lighter font, and you have an icon in the side prompting you to remove them. It’s part of the Code Cleanup feature too, so you run it and it automatically removes them.

Any developer would just do these refactorings as standard.

There’s a general rule called DRY; Don’t Repeat Yourself that means you should avoid duplicated code. We don’t need to document that any further.

Other code smells like extra blank lines, spelling errors, unused methods; are all trivial. You don’t need to explain them. The reporting software actually has explanations and examples for all the rules anyway.

Review Of The Performance Review

For context, it’s best to read my initial blog on this topic: Appraisals

Our review system basically involves us stating some salient things we have done, and we have to rate it against a few categories of values, and rate it 1-4, but we are only really allowed to score it a 3.

So my Performance Review is complete. To nobody’s surprise; I was scored a 3. My manager asked if I was surprised by the score, and I said I wasn’t because that is what everyone else would have got. She said that some people did get a 4. Not sure how; it doesn’t make sense to me.

I think if I went above my role, I’d expect push back from someone. Imagine waltzing into another department like Support with the aim of being “collaborative” – and start demanding changes. Then they are like “who the hell are you? Someone call security!”. If a development manager went in there and started demanding changes, then people would listen. In reality, they would organise a meeting, but their meeting would be accepted, and mine wouldn’t. The reason is that I’m just a developer and have no authority. Yet, if it did happen, I can then tick the boxes of Collaborative Working, Initiative, Innovation etc.

Even trying to take the lead on a project is a tough one. People’s job title’s give a natural hierarchy. When you have a designated Lead, then some Seniors, what chance does a Developer have at even running a meeting, never-mind designing some architecture? The only time it’s gonna happen if people are on annual leave for multiple days or have extended leave. Then you have to spot the opportunity, and take it.

In another meeting, I was talking to a different manager, and raised my concerns about the review process. He then told me that someone in the team got scored a 2.

How? If you read my previous blog on Appraisals, I mentioned how I tried to score myself a 2 for some areas, and I got rejected. If you talk about your score with your manager in your monthly 1-to-1 meetings, even if they did tell you that you have scored a 2 that month, then you will just step up your game, or write more justifications on the sheet to make sure it doesn’t happen again. Even if you did score 2 for 3 months or so, then are scored 3 for the final 9 months, surely your overall score would become 3 anyway? So how can you end up with a 2 unless you have consistently under-performed, done nothing to correct it, and even agreed with your manager that it is fine to be scored a 2? Surely no one in their right mind would do that.

The only reason I can think of is if the scoring hasn’t been consistently applied. Now if you read my previous blog on Appraisals, I mention that we have arrived at this current process with the aim that it is the fairest way of assessing someone, and would be consistently applied.

So let’s talk about the consistency.

Firstly, the goalposts were constantly moved during the year. We were told to fill out a basic spreadsheet, then a month later, new columns were added, then the criteria was changed, then we had to add a personal objective in addition to the default ones. Then there was an additional sheet to fill in. Then near the end of the year, someone told me the criteria had changed, but yet my line manager never told me that. So by the end of the year, some teams were using a different system.

Secondly, sometimes people with the same line manager as me told me extra tips, or about approaching deadlines, but my manager had somehow forgot to tell me about it.

Thirdly, we were initially told our final score mattered and there was no chance of promotion during the year. Then half-way in, an email comes in from HR saying how there is a Mid-Year mini review where some people will be promoted, and we had 3 weeks to submit our forms. However, that same day, we get an announcement from a manager in the department stating that 3 people had just been promoted…but yet they hadn’t completed their forms; the Mid-Year review hadn’t happened yet. For the Mid-Year review, I was struggling to finalise it, and my manager said I had till the end of the week, and she would submit what I had by Monday because it’s a hard deadline. On Monday, when the deadline had apparently passed, I overheard her say to someone else that they had until Wednesday.

Fourthly, even though I said we have monthly 1-to-1’s where we agree a monthly score for the items we submit, one member of my team said he hadn’t done that since the Mid-Year, and he had to cobble something together to submit for the End Of Year Review.

The thing is, because of the poor management of projects this year, there’s been many moments where I have been idle. When we have had requirements, they haven’t been clear and there hasn’t been great scope to actually shine. So if you compare my productivity to previous years, maybe you would give me a 2, rather than 3, but that’s not even really my fault.

I also think any metrics shouldn’t change people’s behaviour. However there was one time where I heard someone say something along the lines of “I have been investigating TechnologyX but it’s not really feasible.” Her colleague then said “so are you free to pick something else up?”, then she replied “no, I’m going to investigate it further so I can add more detail to my appraisal documents”.

In a similar fashion, there are plenty of times in the stand-ups where people say “today I am updating my documents” or “I am doing personal development”. The fact that people spend entire days updating a document instead of doing actual work – is a massive problem.

I also think people try and introduce ideas just so they can tick some boxes for innovation on their documents. I don’t see any reason why teams would switch from Code Build, to GitHub Actions, then Jenkins unless there is some extra personal incentive.

I think if the review system was consistently applied, was set-in-stone rather than constantly evolving, and we had no surprise deadlines spring up on us; then the review system would be acceptable. I feel like I can only give this system a 1 out of 4.

Hopefully it will be better next year.

Window-dressing The Codebase

We have a report analysing our code that does the usual static analysis checks like Code Coverage, “Code Smells” etc.

One team boasted how their report was 100% code coverage and 0 Code Smells; a perfect score.

I thought it was strange because over the previous 3 weeks, many people had pointed out the many bugs their project had. If they do have 100% code coverage, then they aren’t testing the right things. Zero code smells seems suspicious too.

I had a little peek at their repository to spot any suspicious activity. One of the recent bugs stated “we have run out of switch statements”. Obviously, to us software developers – that is a very funny statement to make. What he meant was, they have a huge switch with 30 conditions and the report flags up switch statements that exceed 30 conditions. This is a “Code Smell” because it implies its a bad design.

You could address this warning by:

rewrite a large part of it to implement a better design
do nothing. The warning still flags it in the report.
suppress the warning so the report doesn’t show it.

The developer decided to make a code change, but didn’t bother with a better design. His new switch had 30 statements, but the “default case” then called another method which had another several conditions in it. It gets rid of the warning, but it makes the code more harder to read; so he has made it worse. Suppressing the warning keeps the code readable, but you will still be perceived to be fiddling the report.

Looking at other recent check-ins, the team had “temporarily” suppressed warnings and then logged Bugs to address them later. This is definitely fiddling the report. It makes the report look good, but increased their backlog of work, and wasted time. They could have done meaningful work and let the report do its job with reporting accurate figures.

Then they are highlighting their teams “success” to the managers. The managers don’t look much further than a report, so they are happy.

Just follow the instructions

There’s a team who chose to set-up a generated report every time a code build was triggered. This report will show the usual static analysis checks like Code Coverage, “Code Smells”, bugs etc. It was encouraged that other teams should set this up too; so we were told to contact Rory who was leading this initiative.

I took responsibility for setting this up for my team, so I contacted Rory who sent me a link and said “just follow the instructions”.

I look at the instructions and they seemed to be for setting up a new instance of a Report server, rather than making your build process utilise the existing Report server. Common sense told me he had sent me the wrong link. I asked him again.

He reiterates: “It’s really easy, just follow the instructions”.

I felt Rory was messing me about, so I asked another colleague who was also setting it up for his team. He told me that with Rory’s link, there was only one link within that page which showed you some sample configuration. Every other step listed wasn’t relevant. Yup, Rory was messing me about.

Once set up, you then get a “permission denied” because Rory needs to give your team explicit permissions to contact the Report server. Of course, even though I told him I was setting it up at the start of the day, he hasn’t bothered adding me to the authorized users. Instead, he then wants me to send a request on his Slack channel before he will grant me access.

The thing is, several other teams also needed to set it up, and they went through a similar process of being messed about.

Why didn’t he write some custom instructions? Most of the configuration could just be copied from one team to another.

Also, he always waited until teams were stating they were getting the “permission denied” before granting them access. Why couldn’t he just add them when they initially requested the instructions?

Absolutely infuriating.