Code reviews crippling your process? How comment hierarchy..

Are your pull requests stuck in the code review stage, going through multiple iterations while your initial estimates date are long in the rearview window?

Ever miss a release cutoff because of a nitpicky comment about comment grammar or missing punctuation marks?

Or maybe you get the sense that a senior developer is 'flexing his knowledge' at the expense of your code?

I love code reviews, I loved them when I was a junior developer who thought YAGNI is some sort of Persian flatbread sandwich and I love them now, even when they occasionally devolve into arguments about the merits of tabs vs spaces.

I've also been on teams where code reviews are merely a rubber-stamping ritual and where major bug fixes were held back because of 'trivial' (at the time, to me) comments. My current team has some of the best MR review processes I've ever been involved with, where the comments span the range of nitpicks to in-depth design overviews, but they do not necessarily hold up delivery. I've been thinking about the trade-offs and best practices that naturally emerged, and I want to document the comment hierarchy I use to speed up the delivery process with my team.

In general, code review comments fall into a hierarchy of the following categories, from least to most severe:

Level 1: Clarifications. I also think of these as sanity checks, I may even start the comment with something like I may be completely off the base here... or Just to sanity check... (but only if this cannot be taken as derision). These comments come from a spirit of "I do not have the full context of the problem that you do" and more often than not means the reviewer will get a fuller context of the solution, but occasionally it may catch something fairly obvious that the merge request initiator missed.
Level 2: Nitpicks. Usually, comments about grammar errors and minor stylistic issues/typos go there. The solution to the nitpicks is usually very obvious and if the solution is opinionated, the opinion is not strongly held. Naming a method foobarGenerator vs foobarFactory goes in here, and nitpick comments often start with Nitpick:.
Level 3: Suggestions. These can also be thought of as recommendations and alternatives. This is where a reviewer brings in a fresh perspective on how they would've implemented a functionality differently, usually with reasons, and invites a tradeoff discussion. This is where the 'fun' should be for more senior developers as they think through alternative solutions and share knowledge.
Level 4: Infringement. This is where things get more serious, note that infringement means rules were broken. In this context, rules can mean a number of things, from the more obvious feature spec and framework rules to things like style guides and coding principles. This is also a great place to share tribal knowledge, like I did this once and got burned because... and we actually have an internal document on why we don't do this anymore. What's important here is comments like 'I would do it this way', and 'you are wrong, do this instead' no longer apply at this stage, if it's an infringement the rationale behind the change needs to be stated, and ideally recorded in the style guide as an agreed-upon team principle.
Level 5: Stop the line. The highest level of code review comments. Borrowing the term from Toyota's manufacturing process this is when the code reviewer noticed something in the PR that signals a major defect. Similar to how a worker in the Toyota car assembly line can stop all work until the root of a defect is found, the stop the line comment indicates additional guide rail needs to be installed for the team's process. This can mean adding, removing, or amending rules in style guides or adopting a new programming concept as best practice. Stop the line comments should result in a team-wide agreement, and usually is best resolved via additional discussion threads and meetings instead of inside an MR.

So far I've presented a taxonomy of code review comments, but how do these categories make your team's code review process faster and deliver more customer value? Because levels 1, 2, and 3 are all fairly unopinionated or otherwise easy to fix; if a code review only results in comments in those categories, the reviewer should approve the MR and trust the submitter will make the necessary adjustments. All this means there are fewer rounds of additional review chasing, and fewer context switches for everyone on the team.

This taxonomy and its associated workflow are not set in stone, nor are they strictly enforced by my or any other team, it's based on my observation of what a healthy code review workflow looks like, and your team may add/remove/rename the categories. But I think we can all agree that having the right taxonomy separating the minor nitpick comments away from the emergency team meet comments is the road towards code review blessings.

If you find this post helpful, you may also be interested in my post on how to write code review comments that don't offend or result in pushback.

Happy Coding!