The Inquisitive Coder - Davy Brion’s Blog

At my previous job i was often asked to do code reviews of various projects. The idea was to make sure that each project was developed according to the guidelines and the default architecture we had defined (bad idea, i realize that now). Those code reviews were usually targeted at the developers of the project being reviewed so they could clean up the issues that were outlined in the review. The results were somewhat mixed. Some developers didn’t have a problem with it and they fixed the issues. Some couldn’t care less. Some didn’t have time to fix the issues because they were already behind schedule. And of course, some thought we were simply out to ‘get’ them with our reviews. I’d say the majority of those reviews we did were completely useless and a waste of time and money. Not only that, they probably set off a lot of bad vibes between the people doing the reviews and the people being reviewed.

This week, i was asked to do a 2-day code review of a project. Considering my previous experiences with code reviews, the thought of wasting 2 days on a review that nobody was gonna care about didn’t really thrill me. But this time, the situation was slightly different though… management wanted an assessment of how the implementation of the project was going. This project has been in development for a long time, with most of the code having been written by people who aren’t even there anymore now. The developer who’s working on it now inherited it a while ago, and he’s doing his best to clean up the code while adding new features requested by the business. My usual routine of going through the code and listing the parts that needed to be cleaned up and commenting on them wasn’t gonna be really helpful to the developer (since he already knows how bad the code is) and management doesn’t really care about that either. They don’t want to see a shitload of bad code with my comments on them. They want an idea of how bad it is in general, and what should be done about it.

So i tried a different approach… I browsed through the code, and in my document i listed general issues that were prevalent in the code base. I didn’t really focus on good or bad code, more on good or bad practices that had an impact on the performance, stability and consistency of the application in general. Stuff that affected how the application ran and behaved and also, how easy or hard it would be to maintain or add functionality to the application. In other words, the stuff that end-users and stakeholders of the project care about. They don’t care about how the code looks, nor should they. So i focused on innefficient data structures and data fetching strategies, chatty remote communication, and stuff that would be painful (and thus, expensive) to maintain or to extend with new functionality. After that, i wrote down my conclusions on the general technical state of the project. Instead of providing a summary of what i had mentioned already in the document, i listed a couple of questions that i think are pretty useful to judge a project on:

• Does it work and if so, how well?
• How much did it cost to develop?
• How much will it keep costing us in maintenance?
• How much will it cost if we want to improve it?
• How much will it cost to add new functionality?

This is for general in-house IT projects btw… if you’re developing applications or sites that generate revenue, one of the most important questions is obviously: “How much revenue does it generate?”

Anyways, i then tried to provide answers to each of those questions and finished the review by mentioning the steps that i would recommend to improve the current situation. This was the first code review where i thought it might actually be useful. And even if my conclusions will be ignored by management, i’ll still feel it wasn’t a waste of time like the previous code reviews i had to do.

In case you’re wondering what my thoughts are about reviewing code with the goal of immediately improving the quality of the code: i’d skip the ‘formal’ review documents, and just go over ‘bad’ code while sitting together with the developer who wrote it. Don’t make it seem like you’re criticizing his/her work, make it seem like you’re trying to teach them how to do better. Because that’s what you should be doing anyway.