For better code and happier devs — six code review pitfalls to avoid

While most software teams are doing code reviews, not everyone stops to think about how to do them. Putting some thought into the process can help get much more out of it. Done well, code reviews can be a powerful tool for learning, improving code quality, fostering collaboration, and binding a team together. Done poorly, code reviews can tear a team apart.

These pitfalls are usually not written down as rules for code reviews. They’re just problems you run into easily when you’re not paying attention. Dialogue is key — if your team never discusses why and how to do code reviews, different members can have different mindsets about code reviews, leading only part of the team to fall victim to the pitfalls listed below. For example, a junior developer might think they shouldn’t be reviewing code because they lack experience.

So here are six pitfalls I’ve observed while working in various teams. For each pitfall, I’ve listed some ideas about what to do about them. I have certainly been guilty of at least some of these myself along the way.

#1: The only purpose is to find defects.

While finding issues in the code is an integral part of code reviews, it can be harmful to think that’s all there is to code reviews. For the code author, it can be demotivating when all you get from a code review is a list of things to fix.

This mindset can also lead to all parties going through the motions without thinking about it too much: open/view pull request, get/give a list of things to fix, fix things/wait, get/give approval, done. This approach can make reviewers look only for superficial fixes and miss more significant structural issues.

I’ve seen this mindset lead to unnecessary conflict between team members, which is understandable: If you only ever receive requests to change things or negative comments in a code review, that’s bound to be annoying. A more conversational approach to code reviews makes things more pleasant for all parties involved.

Do instead: Collaborate to make the codebase better.

Code reviews have many other benefits than just finding what’s wrong with the code.

One significant benefit is learning — for both the author and the reviewer. The change author can learn from the reviewers’ comments, while the reviewers can learn from the author’s change. This learning compounds over time, improving overall quality.

Thinking of code reviews as a collaboration rather than a control mechanism also improves how a team works and solves problems together. You’ll be more likely to properly discuss a change rather than just waiting for a list of things to fix and fixing those.

When team members review all code that gets merged, this also builds collective ownership of the code. This way, each change that goes into the codebase has at least two people who are familiar with the change. It will be less likely that some parts of the codebase are only familiar to one developer, which improves the team’s bus factor and makes it easier to make changes in the future.

Code reviews are an excellent opportunity to give timely and specific positive feedback. Say it aloud if you see an elegant solution or learn something new from a code review!

#2: Only seniors review code.

This one can go hand-in-hand with pitfall #1: if code reviews are only for finding defects, aren’t the senior developers most likely to find those, given their experience? However, if less senior team members don’t get a chance to review code themselves, they miss a significant opportunity to learn. Over time, the seniors doing the reviews get a good sense of how the codebase develops and see the big picture, while juniors don’t get this same benefit. This doesn’t help build collective knowledge or help juniors become seniors.

Different team members often have differing ideas about who should do reviews. If nobody explicitly says that everyone should review code, some people might skip it because they don’t feel they know enough to review. It might also happen the other way around so that a developer always asks for reviews from specific team members via a direct message and excludes other reviewers for one reason or another. The excluded team members might not get a chance to review before the change is already merged since they didn’t get notified.

Do instead: Everyone who writes code reviews code.

It’s good to set some explicit rules together as a team about how to do code reviews and who should be doing them. This way, it’s possible to avoid issues related to team members having different ideas about how to review code. One important rule is that all team members must have equal access to notifications about new change requests — these should not go via backchannels only.

If you’re a senior developer and you notice someone who rarely reviews code, you could encourage them to review a change you’ve made. You could also offer to look at the code together with them. This way, they’ll learn about the change and hopefully be more likely to review code later on proactively. Of course, this doesn’t have to be the only review. If it’s an important change, you can have multiple reviewers and a mix of reviews from juniors and seniors. This way, you can improve the quality of the code both in the short term by having more sets of eyes on it and in the long term by ensuring everyone in the team has a chance to learn through reviewing.

Having junior team members review code is also beneficial for making code understandable to everyone. If juniors can’t understand the code, it’s most likely too difficult to understand.

#3: All changes are rubber-stamped.

I’ve often been in situations where I feel like many of my change requests are accepted quickly and with no comments other than “Looks good to me.” Rather than taking this as a sign that my code must surely be perfect, I’ve felt the reviewer might not be taking a detailed look at the change. I’ve noticed rubber stamping most often happens when I’m working independently on some sizeable new feature that the rest of the team doesn’t understand well. If this is the case, finding ways to collaborate on the change is good so that others become more familiar with it.

For small trivial changes, rubber stamping is often perfectly fine, but it becomes a problem when it’s done regardless of the size of the change. It’s good to think about when a lighter process is acceptable and when a more thorough review is needed.

Do instead: Understand the change and think about it from multiple perspectives.

It’s hard to give good, thoughtful feedback on a change that you don’t fully understand. However, since you’re doing code reviews, you will also be merging the change into a branch with collective ownership. If only the author understands the change, then there’s no common ownership. If you don’t understand the change, then ask questions about it and discuss it until you do. Efforts to understand the change can also lead to ideas for improved documentation or code clarity, which both help all team members in the long term.

If you understand the change but still can’t think of anything to comment on, it might help to follow a checklist of perspectives for each change systematically. For example:

Has documentation been updated? Do the documentation changes cover everything in the change?

Would the new code be easy to understand for junior team members?

What are the security implications of the change? Does it introduce any new risks?

#4: Ten lines — ten comments. Five hundred lines — looks good to me!

It’s much easier to understand and grasp a small change. Finding something to improve is easier when you understand a change. Large changes can easily be overwhelming, making it tempting to blindly trust that the change is probably OK instead of taking time from a busy day for a proper review.

Do instead: Use small batch sizes and do pair coding.

Most of the time, code changes don’t have to be made in large batches, so the best way to avoid the issue is to keep changes small. When you can’t avoid making a bigger change, consider reviewing the code together on the same computer or using pair coding to make the change in the first place. In some cases, you could consider pair coding as an alternative for a code review since it covers much of the same needs.

#5: All changes are reviewed asynchronously.

Organizing a face-to-face conversation or even a video call for every code review can be challenging. Time zones, deadlines, and schedules can easily clash. For many routine changes, it would also be overkill to do so. The problem is that even if people sit in the same office, code reviews are never done together but only happen through asynchronous pull request review messages.

Do instead: Review the code together where it makes sense.

There are many minor changes where a quick asynchronous review is fine. However, in many cases, it is much easier to collaborate and discuss changes and different alternatives when you sit together for a code review or review over a video call. The author can explain any parts that are unclear right there. It’s much easier to have true collaboration this way than through asynchronous reviews.

In addition to code reviews related to a single change, it can also be a good idea to have review sessions where the whole team covers the most significant code changes, with the author explaining the change to the rest of the team.

#6: Nitpicking on style issues

I’ve sometimes reviewed changes where formatting made the code much harder to read than needed. Some common issues are the lack of empty space, lines that are way too long, and inconsistent white space usage. Then again, fixing these types of problems is annoying for both parties if done via code review comments.

Do instead: Use automatic linting on everything.

Style issues are always at least somewhat subjective. Instead of having the same conversations repeatedly, it’s much easier just to add automatic style checking to change requests and have it complain if any issues are found. First, agree together as a team how pedantic you want to be about style, and then codify a set of rules for all the languages you use. There are many different options for linting software that can automatically check and fix style issues. For example, for JavaScript, there’s eslint. You can also check your documentation with markdownlint. Or you could use a linter that covers multiple languages like super-linter for GitHub Actions. You create a config file for any linters you want to use in the root of your repository and then run the linter automatically for each change as part of your continuous integration pipeline.

Rather than wait until the review, you can write onboarding documentation for the whole team about how to set up their editors to automatically correct any style issues according to commonly agreed style rules.

Conclusion

Implementing various practices without thinking too deeply about how to implement them is common. This is especially true for things widely considered best practices like code reviews — it’s taken for granted that we should do them. Still, it can be worthwhile not to stop there but to also pay close attention to how we implement these practices to reap the many benefits of well-done code reviews.

Risto Laurikainen

—

Risto Laurikainen is a DevOps Consultant with a decade of experience in building cloud computing platforms.