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.