I am a strong advocate for code reviews. They have many advantages including improving code quality and team communication. On the other hand, they take a lot of time and add yet another delay to the development pipeline. With the wrong team culture, they can create hard feelings and discourage honest collaboration. This is a heavy price to pay, and yet, I’ve seen many teams that do them without ever talking about how or why.

With a bit of intentionality and the right support systems, code reviews can become a catalyst for positive change that are absolutely worth the time we put into them.

Why we do code reviews

There are a lot of reasons for code reviews, but the one I find the most valuable is that they promote shared ownership of the code. Sharing code forces teams to get aligned on architecture and coding practices. This gives you more flexibility to distribute work, reducing bottlenecks and guarding business continuity. The developers who work on your critical systems will appreciate the ability to take vacations as well.

Another advantage is that reviews improve code quality simply by being part of the development process. Developers can be lazy people (ask me how I know), but we get a little extra incentive to tidy things up when we know someone else will be looking. Since we spend much more time reading our code than writing it, this will improve efficiency in the long run.

Reviews can also be a more efficient way to onboard developers, especially for the kinds of knowledge that are hard to articulate. For a small team with little planned growth or churn, writing a huge body of documentation may not be a great use of time. Of course, learning through review feedback doesn’t always feel very nice when you’re on the receiving end, but this is another reason why it’s valuable. Making it normal to receive constructive feedback without shame is essential if you want a team where members push themselves and grow.

First, automate everything you (effectively) can

One simple way to reduce the time you spend on code reviews is to automate as much as you can. Some things require a human touch, but some other things are much easier for computers.

Compilers are typically very easy to automate, and it can take humans lots of time to detect compilation errors. If you automate only the compilation of any proposed changes, you can prevent a lot of build breaks and wasted time. This is helpful even with a team of one.

You should be running your unit tests automatically as well. Not only will this make sure any new tests pass, but it’ll prevent failing tests from getting merged back to the main codebase, which can in turn disrupt other developers.

Longer-running automation tests or e2e tests may be too slow or expensive to run on every proposed change, but you should consider the tradeoffs. Even the most well-intentioned team can find it irresistible to ignore a failing test when a tight deadline is looming. Once you have a couple of tests that fail regularly, it doesn’t take long before confidence disappears and decay sets in.

There are lots of tools for checking basic code formatting and whitespace. These are also good to automate and should be set up early in any project. I have seen so much time wasted in reviews talking about white space and proper nesting. Let the machines give feedback in minutes instead of wasting developer time on it. Make sure you have an .editorconfig that matches the rules you’re checking so that your IDEs get most code correct before the review starts.

What to look for when reviewing code

I try to focus mostly on readability and maintainability. Do I understand the intention of the change? If I needed to change this code in 6 months would I be able to find it and figure out what it’s doing? Do the names being used describe what they are?

I don’t check if the change meets its requirements unless I happen to notice it. Code is not a great level to test functionality at, and I expect the author to test this themselves, and a further manual test to be done once the change is in a build. I do check that any commit messages adequately describe what the change is.

I also like to check that a good number of tests have been written, and any complicated or risky code has sufficient coverage. If I’m honest though, I don’t typically check the contents of test methods. I find most tests difficult to read, and I expect the developer to have checked that they work already.

Self-editing and maintaining a positive culture

Before I post a code review comment I like to ask myself a few questions:

  • Is the change important? Have I communicated that?
  • Is it clear what needs to be changed?
  • Is it reasonable to expect the author to make the change?

At this point in my career, I find that a lot of my code review comments are more instinctual than formed logically. I don’t think it’s a bad thing to operate this way, but instincts can drift or misfire sometimes. Even if something feels wrong it might not be a problem, or it might not be important. I use my checklist to validate my instincts. It’s not uncommon for me to add a bunch of comments while doing a review, then go back and delete some of them before hitting the send button.

Sometimes I have concerns about the underlying approach or the broader scope of work the change is a part of. In these cases, I check myself carefully to make sure my feedback is important. If I don’t want to be a part of every decision made by the team, I have to trust the team to make them without me. Big changes are also more expensive, and the code review step is pretty late in the process. Sometimes the stakes are high enough though.

As with all business communication, it is important to be mindful of tone in code reviews. I always try to assume positive intent and try to reflect that in my comments. If I have a lot of feedback or serious feedback, I might speak with the author directly. A quick face-to-face or video chat makes it easier to diffuse hard feelings.

Another thing I avoid is the “I would have done it like…” pattern. At best it’s a poor justification for a change, at worst it implies superiority. It is much better to use “I suggest doing X because Y”.

Suggesting alternatives

The easiest way to ensure a comment is reasonable is to provide an alternative myself. For example, I won’t leave a comment about something’s name unless I can offer a better name, and give a good reason why the old name doesn’t make sense.

I also like to use the diff feature in GitHub comments to show the actual change if it’s reasonable to do so. This often doesn’t work for naming (because there will be references that also have to be changed), but it’s great for little stuff like typos in user-facing strings. It is also sometimes the easiest way to communicate an improved algorithm or propose new function signatures.

Number of reviewers

Sometimes it’s important to get lots of eyes on a critical change. Sometimes it’s not important but people do it anyway. Regardless of the reason, it can be especially painful to get code approved when multiple reviewers have to approve. In these circumstances, and especially when I’m late to the review, I temper my feedback further than usual so that the code review won’t go around in circles forever.

The worst is when two reviewers disagree on the best approach and argue about it in the code review. Whenever this happens I ask the two reviewers to talk to each other directly to resolve the issue. It can be beneficial for the author to join in as well.

I generally recommend just one reviewer for most code reviews as this gets the most benefit with the least cost. I also suggest making sure the whole team is doing reviews. You could assign them all to the most senior member of the team, but doing reviews is a valuable teaching experience and not all code requires an expert opinion. If a code review does require an expert opinion, I prefer to ask those experts to focus on specific areas but let someone else handle the broader review. Having new team members join code reviews is also an easy way to expose them to team practices and any work in progress.

Don’t say anything if you don’t need to

Sometimes the code is good enough, especially if the change is small and clear. If it’s good enough, resist the urge to nitpick and mark it as reviewed.

Accepting feedback on my feedback

When someone puts comments on the code I’ve put up for review, I take those comments seriously. Sometimes I don’t agree, but I put in extra effort to make sure I maintain an appreciative and curious tone. Even bad feedback is often grounded in good reasons. It might be a hint that another other part of the change isn’t clear. If I believe the comment is an error, I respectfully point it out and provide references if I can.

When I’m reviewing, I often make suggestions without testing them. I also go from memory, and sometimes I make mistakes. When someone challenges a comment of mine, I take it just as seriously. This isn’t a sign of disrespect, it’s just the nature of working on hard problems in a team.