Code Review Anti-Patterns

#productivity #career #developers

There are few developers who haven't felt a touch of nerves after finishing a feature, raising a pull request, and then waiting for the approval of a reviewer - sometimes, multiple reviewers! When done well, code reviews can help prevent bugs and be a welcome opportunity for engineers to collaborate. When done poorly, they can be a source of dread and dysfunction that can badly fracture a team.

Here are three very common issues, all of which I've experienced during my career, with the first one in particular being the inspiration for this piece. Note that these are all issues on the reviewer and organisational side - a second article could easily be written on practices to avoid when on the receiving end of a review!

Rude, Bullying, or Cruel Tone

Software isn't carpentry, painting, or some other craft where there's both a mental component and a physical output that relies on dexterity. As an all-virtual affair, the software that someone writes can very much be regarded as a direct mental projection - a pure manifestation of their knowledge and intelligence. As such, it is important to be extremely careful when critiquing someone's code, because the line between "that's stupid" and "you're stupid" is so thin as to essentially be absent. Even without using those specific words, it's all too common for code reviews to be aggressive, demeaning, or undermining. Firstly and most importantly, this is disrespectful and hurtful to individuals, but it also demolishes trust and psychological safety within the team as a whole.

I'm not the first person ever to observe that there is, sadly, far too much intellectual bullying in software development. While code reviews are an absolute playground for such types, all of us could stand to be more mindful of our tone and approach. Bear in mind that everyone is doing the best they can, may be working with rules or constraints you don't know about, and that anyone can have a sub-par day!

Holding Pull Requests to Ransom

Code authors should be open to feedback and willing to take on board suggestions from the reviewer that will improve the code, so what we're not talking about here is a reviewer properly withholding approval until some reasonable changes have been made. This anti-pattern has two distinct sub-types;

1. Withholding approval pending changes based on personal taste.

This often takes the shape of nit-picking over small stylistic issues, specifically for cases in which the organisation hasn't mandated an approach. This can be reduced with the use of linters/pre-commit hooks in combination with agreed style guides for whichever languages are in use. However, some issues are more subjective and don't lend themselves to automated detection and correction. It's worth making the distinction for anything that falls into this category between something that you wouldn't do that way and something that shouldn't be done that way. Only the latter should hold up the approval of a pull request.

2. Withholding approval pending changes outside of the reasonable scope of the work being carried out.

This second type is a particularly odious practice. In this case, an engineer has submitted a completely reasonable solution to their task, fitted it into the existing codebase with respect to the existing style and structure, and overall there are no problems. Despite this, the reviewer will refuse to approve the pull request until a large amount of further work is done. Commonly, this is a substantial refactor or entire redesign of an area that happened to be touched by one of the commits, even if the proportional size of the submitter's change is tiny. If the reviewer is much more senior than the submitter, this can carry the implicit weight of an order, even if the reviewer is in no official position to assign work. This is an anti-pattern because:

It strong-arms the submitter into resolving historic or systemic issues that preceded them by using their need to complete their currently assigned work as leverage, which is bullying.
It reduces the speed of feature delivery and disrupts scheduling by adding in additional "hidden" work not accounted for in planning/estimation.
Over time, it can ultimately discourage developers from making changes, fearful of being burdened with a disproportionate amount of additional work on anything they touch.

If further work in an area would be of benefit, it should be noted and then added to the product backlog to be prioritised and assigned appropriately. This allows for better planning, visibility, team input, and collective ownership.

No Reviews At All

While it can certainly be argued that dependent on the context and organisation, some changes are so trivial (e.g. a text content update) and externally observable before official release that mandating a review might be seen as wasteful, I would say that the vast majority of changes should be reviewed in some fashion. This isn't a universally held view though, and one counter I often hear is that new functionality that can be disabled/enabled easily via a feature switch removes the risk of deploying new code without the delay of a review. However, using this approach in isolation rests on a very specific assumption;

That the externally observable behaviour of the code is the only thing that matters.

While the code may initially appear to work, there may yet be problems lurking below the surface, just a few of which could be:

Performance issues due to a naïve implementation that won't be obvious in real usage until it has been running for some time
Poor maintainability/flexibility due to the chosen approach
In the worst possible case, unauthorised changes could be slipped in alongside assigned work

Bearing that in mind, why not make use of both approaches together? Reviews can help avoid some of the pitfalls above, while feature-switching can be used both to align feature releases with business needs and to have a safety control in case of unforeseen problems.

Conclusion

The code review process is not only a method for improving software quality but also provides the opportunity for team members to teach, learn, and build trust. Conducting reviews in the right spirit, acting in and assuming good faith, can help make them pleasant and productive for everyone involved.