Two people review code

How to Make Good Code Reviews Better

Two people review code
I have been doing day-to-day code reviews for over a decade now. The benefits of code reviews are plenty: someone spot checks your work for errors, they get to learn from your solution, and the collaboration helps to improve the organization’s overall approach to tooling and automation. If you’re not currently doing code reviews in your organization, start now. It’ll make everyone a better engineer.

Plenty of people and organizations have shared their code review best practices and what the definition of good code reviews mean to them. Guides  from Google,  the SmartBear team, and engineer Philipp Hauer are all excellent reads. Below is my personal take on what good code reviews look like and how to make them even better at the team and organizational level. This is in the context of the tech environment I have been working at – currently at Uber, and before that at Skype/Microsoft and Skyscanner.

Good code reviews are the bar that all of us should strive for. They cover common and easy to follow best practices that any team can get started with, while ensuring high-quality and helpful reviews for the long term.

Better code reviews are where engineers keep improving how they do code reviews. These code reviews look at the code change in the context of the codebase, of who is requesting it and in what situation. These reviews adjust their approach based on the context and situation. The goal not only being a high-quality review, but also to help the developers and teams requesting the review to be more productive.

Areas Covered by the Code Review

Good code reviews look at the change itself and how it fits into the codebase. They will look through the clarity of the title and description and “why” of the change. They cover the correctness of the code, test coverage, functionality changes, and confirm that they follow the coding guides and best practices. They will point out obvious improvements, such as hard to understand code, unclear names, commented out code, untested code, or unhandled edge cases. They will also note when too many changes are crammed into one review, and suggest keeping code changes single-purposed or breaking the change into more focused parts.

Better code reviews look at the change in the context of the larger system, as well as check that changes are easy to maintain. They might ask questions about the necessity of the change or how it impacts other parts of the system. They look at abstractions introduced and how these fit into the existing software architecture. They note maintainability observations, such as complex logic that could be simplified, improving test structure, removing duplications, and other possible improvements. Engineer Joel Kemp describes great code reviews as  a contextual pass following an initial, light pass.

Tone of the Review

The tone of code reviews can greatly influence morale within teams. Reviews with a harsh tone contribute to a feeling of a hostile environment with their microaggressions. Opinionated language can turn people defensive, sparking heated discussions. At the same time, a professional and positive tone can contribute to a more inclusive environment. People in these environments are open to constructive feedback and code reviews can instead trigger healthy and lively discussions.

Good code reviews ask open-ended questions instead of making strong or opinionated statements. They offer alternatives and possible workarounds that might work better for the situation without insisting those solutions are the best or only way to proceed. These reviews assume the reviewer might be missing something and ask for clarification instead of correction.

Better code reviews are also empathetic. They know that the person writing the code spent a lot of time and effort on this change. These code reviews are kind and unassuming. They applaud nice solutions and are all-round positive.

Approving vs Requesting Changes

Once a reviewer completes their review, they can either mark it approved, block the review with change requests, or not set a specific status, leaving it in a “not yet approved” state. How reviewers use the approve and request changes statuses is telling of the code reviews.

Good code reviews don’t approve changes while there are open-ended questions. However, they make it clear which questions or comments are non-blocking or unimportant, marking them distinctively. They are explicit when approving a change – e.g. adding a thumbs up comment like “looks good!”. Some places use acronyms like LGTM—these also work, but be aware that newcomers could misinterpret these insider acronyms for something else. Good code reviews are equally explicit when they are requesting a follow-up, using the code review tool or team convention to communicate this.

Better code reviews are firm on the principle but flexible on the practice: sometimes, certain comments are addressed by the author with a separate, follow-up code change. For changes that are more urgent than others, reviewers try to make themselves available for quicker reviews.

From Code Reviews to Talking to Each Other

Code reviews are usually done asynchronously and in writing through a code review tool. This is usually out of convenience, to enable remote code reviews, and to allow multiple people to review the same code change. But when is it time to stop using the tool—however good it might be—and start talking face to face about the code?

Good code reviews leave as many comments and questions as are needed. If the revision does not address all of them, they will note those as well. When the conversation gets into a long back-and-forth, reviewers will try to switch to talking to the author in-person instead of  burning more time using the code review tool.

Better code reviews will proactively reach out to the person making the change after they do a first pass on the code and have lots of comments and questions. These people have learned that they save a lot of time, misunderstandings, and hard feelings this way. The fact that there are many comments on the code indicates that there is likely some misunderstanding on either side. These kinds of misunderstandings are easier identified and resolved by talking things through.

Nitpicks

Nitpicks are are unimportant comments, where the code could be merged without even addressing these. These could be things like variable declarations being in alphabetical order, unit tests following a certain structure, or brackets being on the same line.

Good code reviews make it clear when changes are unimportant nitpicks. They usually mark comments like these distinctively, adding the “nit:” prefix to them. Too many of these can become frustrating and take the attention away from the more important parts of the review, so reviewers aim to not go overboard with these.

Better code reviews realize that too many nitpicks are a sign of lack of tooling or a lack of standards. Reviewers who come across these frequently will look at solving this problem outside the code review process. For example, many of the common nitpick comments can be solved via automated linting. Those that cannot can usually be resolved by the team agreeing to certain standards and following them—perhaps even automating them, eventually.

Code Reviews for New Joiners

Starting at a new company is overwhelming for most people. The codebase is new, the style of programming is  different than before, and people review your code very differently. So should code reviews be gentler for new starters, to get them used to the new environment, or should they keep the bar just as high, as it is for everyone else?

Good code reviews use the same quality bar and approach for everyone, regardless of their job title, level or when they joined the company. Following the above, code reviews have a kind tone, request changes where needed, and will reach out to talk to reviewers when they have many comments.

Better code reviews pay additional attention to making the first few reviews for new joiners a great experience. Reviewers are empathetic to the fact that the recent joiner might not be aware of all the coding guidelines and might be unfamiliar with parts of the code. These reviews put additional effort into explaining alternative approaches and pointing to guides. They are also very positive in tone, celebrating the first few changes to the codebase that the author is suggesting.

Cross-Office, Cross-Time Zone Reviews

Code reviews get more difficult when reviewers are not in the same location. They are especially challenging when reviewers are sitting in very different time zones. I have had my fair share of these reviews over the years, modifying code owned by teams in the US and Asia, while being based in Europe.

Good code reviews account for the time zone difference when they can. Reviewers aim to review the code in the overlapping working hours between offices. For reviews with many comments, reviewers will offer to chat directly or do a video call to talk through changes.

Better code reviews notice when code reviews repeatedly run into timezone issues and look for a systemic solution, outside the code review framework. Let’s say a team from Europe is frequently changing a service that triggers code reviews from the US-based owner of this service. The system-level question is why these changes are happening so frequently. Are the changes done in the right codebase or should another system be changed? Will the frequency of changes be the same or go down over time? Assuming the changes are done in the right codebase and the frequency will not go down, can the cross-office dependency be broken in some way? Solutions to these kinds of problems are often not simple and could involve refactoring, creating of new services/interfaces or tooling improvements. But solving dependencies like this will make the life of both teams easier and their progress more efficient for the long term, meaning the return on investment is often quite impressive.

Organizational Support

The way companies and their engineering organizations approach code reviews is a big element of how efficient they can be. Organizations that view them as unimportant and trivial end up investing little in making reviews easier. In cultures like this, it might be tempting to just do away with code reviews entirely. Engineers advocating for doing better code reviews might feel isolated, without support from above and eventually give up. The result is an organization where problems continue to repeat and compound upon themselves. 

Organizations with good code reviews ensure that all engineers take part in the code review process—even those that might be working on solo projects. They encourage raising the quality bar, and teams facilitate healthy discussions on code review approaches both at the team and org level. These companies often have code review guides for larger codebases that engineers initiated and wrote. Organizations like this recognise that code reviews take up a good chunk of engineers’ time. Many of these companies will add code reviews as expectations to the developer job competencies, expecting senior engineers to spend a larger chunk of their time reviewing the code of others.

Organizations with better code reviews have hard rules around no code making it to production without a code review—just as business logic changes don’t make it to production without automated tests. These organizations have learned that the cost of cutting corners is not worth it; instead, they have processes for expedited reviews for urgent cases. These organizations invest in developer productivity, including working continually to develop more efficient code reviews and tooling improvements. Helpful engineering executives don’t need convincing on the benefits of code reviews and other engineering best practices. Instead, they support initiatives on better tooling or more efficient code review processes that come from teams.  

When people come across reviews that feel hostile, they feel they can speak up and have support all-round to resolve the issue. Senior engineers and managers consider code reviews that are not up to the bar just as much of an issue as sloppy code or poor behavior. Both engineers and engineering managers feel empowered to improve how code reviews are done.

Start With Good, Make it Better

Good code reviews already have lots of good effort going into them. They do a thorough review of the change itself, avoid being opinionated with the tone of comments, and make nitpicks clear. They maintain a consistent bar, regardless of who is requesting the review and try to make cross-time zone reviews less painful by paying additional attention to these. Organizations that have good reviews ensure that every developer regularly receives and does code reviews. This is already a high bar—but if you get here, don’t stop. Code reviews are one of the best ways to improve your skills, mentor others, and learn how to be a more efficient communicator. 

Get to better code reviews by continuously improving on the details, but also start looking at changes at a high level as well. Be empathetic in the tone of comments and think of ways outside the code review process to eliminate frequent nitpicks. Make code reviews especially welcoming for new starters and look for systemic solutions for painful cross-time zone reviews. Organizations that are forward-looking encourage investing in tooling and process improvements to make code reviews better, reaping more of the benefits.

——-

Gergely is currently an engineering lead in Amsterdam. This blog post originally appeared on Gergely’s blog, The Pragmatic Engineer. If you’d like to read more from Gergely, you can subscribe to his monthly newsletter for his articles on engineering, tech leadership and distributed systems. If you would like to contribute articles to the Stack Overflow blog, send an email to pitches@stackoverflow.com.

Author

Gergely Orosz
Engineering lead at Uber, in Amsterdam. Microsoft, Skype & Skyscanner alumni. Writes on The Pragmatic Engineer blog.

Related Articles

Comments

  1. Asteroids With Wings says:

    I like how your solution to “nitpicking” over style standards is…. introducing style standards. How do you propose enforcing them? Perhaps by doing code review? 🤗

    1. The codebases I’ve worked on, we usually enforce these via linting. Once agreed on the standard and the linter configured, if you add it as a build step or CI step, the build won’t pass until those issues are fixed.

      If you can’t automate it, having them agreed by the team will probably still help having fewer discussions about these.

  2. Many people have never seen a good code review, a better code review, or any code review.

    This wouldn’t be easy for obvious reasons, but I would love to see videos of code reviews, including when there are some issues to point out.

    Descriptions of code reviews are helpful, but still leave most of the picture blank. Imagine making a movie if you’ve only read reviews but you’ve never actually seen one. You get Birdemic – Shock and Terror.

    1. The hard part in reviews is to compose with individual caracters which can be either harsh on co-workers or does not accept criticism for their own code. But if everyone is nice then it’s piece of cake to achieve all the other goals.
      I don’t think you have to watch one to understand how to, it’s like when you ask a fellow worker his opinion/advice about something you are doing, except it’s on a supposed finished part, and it can be more than two people.

      1. I’m sure that someone can do one without having seen one. But there could be many subtle details that could be observed from watching one that teams would have to re-discover on their own.

        I’m not suggesting that anyone should watch a video and blindly imitate it. But starting from a good model and adapting it may be a better starting point than just reading and guessing. (Why does this blog post exist? Teams need help with this.)

        Birdemic is a great example. We’ve all seen bad movies, but this one stood out as unusual. You can’t understand until you see it. It’s as if people from the future or another world had read about movies but had never seen one. They understood that there’s a script and that you film people saying things and pretending it’s real. The result illustrates the difference between doing something you’ve read about and doing something you’ve seen.

  3. Here’s a nitpick:, 🙂 – code reviews don’t need to be limited to review of just changes. They can be usefully done for a first implementation, for a detailed code design, or even a high level design.

    1. Alex Churchill says:

      Designs can and should be reviewed, absolutely. I’d call that a design review rather than a code review, though. You can even have a multi-stage process: identify the requirements and user goals (and have a requirements review); design the externally-visible functionality (and have a functional design review); design the internal architecture to achieve that functionality (and have an architectural design review); finally implement that architecture (and have a code review). Of course, this is overkill for small features.

  4. (nit) This sentence doesn’t really make sense: Good code reviews already have lots of good effort that going into them.
    Great post though, really enjoyed it. 👏

    1. Thanks for the catch! Fixed it.

  5. Faraz Ahmed Abbasi says:

    This is good blog for reviewing code and I think when I review code, I do recommend what to do and what not to do but after reading this blog I will improve my coding reviews so everyone can improve its coding.

    Thanks

  6. Nice post 😀

    To nitpick your nitpick-section: the word “are” appears twice in a row.

  7. Thank you for the article, very nice written! I would be curious how you deal with difference of opinions regarding on how the code should look like. For example a colleague is requesting changes which you consider are not necessary better and you both stick to your grounds?
    Also on the same PR every time he looks at the code he requests more and more changes prolonging indefinitely how long the ticket is open?

    1. If there are too many questions/disagreements on the code style, sounds like it’s time to decide about that, as a team. And perhaps implement automated linting to end all disagreements. I’ve mentioned this in the “Nitpicks” section.

      When there are too many back-and-forths, talking in person is key. See the “From Code Reviews to Talking to Each Other” section thatt talks about this.

  8. > Good code reviews ask open-ended questions

    Nope.
    “Open-ended questions” are harder to understand and, generally, are not very actionable.
    More specific questions or claims — are much easier to understand and are much more actionable.
    If code reviewer made a very specific claim, it is much faster to process such claim.
    Even if code reviewer was incorrect – clear and specific claim allows to reply with a quick rebuttal (instead of playing prolonged “do I understand you correctly?” game).

    1. I’ve shared what I saw work, based on my experience. Open ended questitons are unassuming and make for a nice review, from the point of the person whose code is being reviewed.

      The better you know the person you’re reviewing the code for, the easier it is to be more direct, without offending the person. I work in an environment, where we often don’t know the reviwers – and they are in different offices – so keeping it open-ended keeps the tone of code of reviews pleasant for everyone.

  9. Otavio Macedo says:

    I would only add something about the mindset: authors should keep in mind that submitting a code review is an opportunity to get feedback, more than just get your change approved. Try to learn something, consider other points of view and so on.

    More importantly, though, reviewers should *also* keep in mind that the goal is to provide feedback to help the author and, by extension, the team. It’s *not* an opportunity to show off your knowledge and skills! I’ve seen so many instances in which reviewers make tons of irrelevant comments. Some of these comments are nitpicks, as you mention, but there is also the type of rant that goes off on a tangent, just to show how much the reviewer knows about software development.

    1. I love this! Fanastic oversavations, Otavio. Yes, the mindset going into the code review is really key. Some of the best reviews I’ve seen from people following this approach, may that be consciously or subconsciously.

Leave a Reply

Your email address will not be published. Required fields are marked *

This site uses Akismet to reduce spam. Learn how your comment data is processed.