how to make good code reviews better
src: How to Make Good Code Reviews Better - Stack Overflow - 2022-03-12
Abstract
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.