Code Reviews: How Far Down the Rabbit Hole Do We Go?

“I only modified one line of the file, why should I be responsible for fixing the coding style of the whole thing?” That was my first reaction when participating in a full-scale code review at one of the software companies I worked for. Then, if I do have to fix the coding style, my name is going to be on the blame list when somebody checks version control for who wrote that section of code. How far should we take fixing things during reviews?

Code Reviews: Why?

Coding architecture and style are huge factors in how maintainable a project is. Everybody has their own opinions regarding these, especially coding style. When several developers work on a project, we all add our own dialects into the code we write, even if there is an official style guide we are supposed to be following.

Code reviews help each of our code to mesh together nicely. It’s an opportunity to discover flaws in the architecture early on, implement small (but important) code maintenance tasks, and gives somebody else a chance to both learn from and improve the code base.

The Heavy-Handed Approach

On the one extreme, you can conduct code reviews on all files that the developer modifies before it gets marked as approved (or whatever your workflow has for this). In order to improve the existing codebase, you would come up with a comprehensive list of things that need reviewed or fixed. The developer would then have to complete these and, once the code reviewer is satisfied, the code can enter into the codebase.

On the plus side, the quality of your code base will theoretically improve since the developer would slowly, file-by-file be fixing the identified issues. The developer and the code reviewer would be able to keep track of what they fixed and what’s left.

The downside to this approach is that it does not work well in organizations that operate on lighter processes that do not have this approval-before-checkin type of workflow. This method slows down coding iterations (some of which can be offset by using automated code review and linter tools).

The Lightweight Approach

On the other extreme is the informal type of coding review where periodically somebody looks through the code base for general issues. The nice thing about this approach is the speed factor. This can be adjusted to occur as frequently or infrequently and in-depth or terse as needed.

The downside to this approach is that, since the code is already in the codebase and basically “approved”, the motivation to fix it is reduced. This can lead to the code review results becoming stagnant or forgotten as it is difficult to gauge if any progress  . The other problem is that this doesn’t catch all violations and leaves it up to the programmer to search for other spots in the code where they may have used the same style.

My Favorite

I actually like to lean towards the heavy-handed approach more than the lightweight approach. As annoying as it is, the codebase will definitely improve. It’s very easy under the lightweight approach to let code improvement tasks fall off the bottom of your todo list.

Matt Bajoras

Matt Bajoras

Matt Bajoras

Latest posts by Matt Bajoras (see all)

Tags:

Creative Commons License

This work is licensed under a Creative Commons Attribution-NonCommercial-NoDerivatives 4.0 International License.

1 Comment

  1. Charlie Burrows

    The heavy handed approach can introduce an awful lot of risk. If you have working code and you refactor the hell out of it you run a significant risk of breaking it in some subtle way. Unless your autotest suite is perfect (unlikely) you will also introduce significant testing overhead simply by touching more of the code. I’m not suggesting that you shouldn’t try to improve your codebase without some defined bug to fix but I think it’s important to weigh the risk and the time involved to mitigate that risk to your satisfaction before refactoring.