“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.
