I've published an updated and re-edited version of this article on the Stack Overflow blog - check it out!
I have been doing day to day code reviews for over a decade now. The benefits of code reviews are plenty: from someone else reading through the change, through knowledge sharing all the way to tooling and automation improvements. If you are not doing code reviews, take the advice from Jeff Atwood shared in 2006 and just do it.
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 the SmartBear team, the Palantir engineering team and from engineer Philipp Hauer are all great reads. Below is my personal take on what good code reviews look like and what great ones - better than good - are. This is in the context of the tech environment I have been working at - currently at Uber, before at Skype/Microsoft and Skyscanner.
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 following 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 in one review, suggesting keeping code changes single purposed and breaking the change into more focused parts.
Better code reviews also 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, test structure, duplication and other possible improvements. The way engineer Joel Kemp puts it is these code reviews do 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 micro-aggressions. Opinionated language can turn people defensive, sparking heated discussions. At the same time, professional and positive tone can contribute to a more inclusive environment. People in these environments are open to constructive feedback and code reviews trigger healthy and lively discussions.
Good code reviews ask open-ended questions over making strong or opinionated statements. They offer alternatives and possible workarounds that might work better for the situation. These reviews assume the reviewer might be missing something and ask for clarification instead of correction, at first.
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
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, often called "nitpicks" or marked with "nit". They are explicit when approving a change - e.g. giving it a LGTM. They are equally explicit when they are requesting a follow-up, using the code review tool or team convention to communicate this.
Better code reviews still don't approve changes until there are important questions that need to be answered or addressed. These 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
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 over 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, over going back-and-forth on the new revisions. 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
As mentioned before, good code reviews make it clear when changes are unimportant nitpicks. These could be things like variable declarations being in alphabetical order, test structures following a certain structure or brackets being on the same or the next line.
Good code reviews usually don't have too many nitpicks. Too many nitpicks can become frustrating and take the attention away from the more important parts of the review.
Better code reviews realize that too many nitpicks are a smell 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, most 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, following them - perhaps even automating them, eventually.
Code reviews for new joiners
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 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, 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-timezone Reviews
Code reviews get more challenging when reviewers are not in the same location. They are especially challenging when reviewers are sitting in very different timezones. 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 timezone difference when they can. Reviewers aim to review the code in the overlapping time between offices, assuming there are a few hours. 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. Solving dependencies like this will make the life of both teams easier and their progress more efficient.
Organizational Support
How companies and their engineering organizations approach code reviews is a big part 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, in some cases. Engineers advocating for doing better code reviews might feel isolated, not getting support from above and eventually give up. Companies that treat code reviews as a key part of good engineering make for a more empowering place for engineers to work at.
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. Teams facilitate healthy discussions at the 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 at the team and org level. These organizations invest in developer productivity, including more efficient code reviews and tooling improvements. Executives in these companies usually were software engineers themselves who 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 behaviour. Both engineers and engineering managers feel empowered to improve how code reviews are done.
Featured Pragmatic Engineer Jobs
- Engineering Leader - Card Platform at X1 Card. $250K+. Remote (US).
- Engineering Leader - Card Product at X1 Card. $250K+. Remote (US).
- Senior Android Engineer at Polarsteps. Amsterdam.
- Frontend Software Engineer at Enveritas. $130-150K. Remote (Global).
- Senior Software Engineer, Distributed Systems at Mixpanel. $200-270K + equity. New York, San Franciso, Seattle or Remote (US).
- Senior Software Engineer, Fullstack at Mixpanel. $200-270K + equity. New York, San Franciso, Seattle or Remote (US).
- Software Development Manager - eCommerce at Card Kingdom. $175-195K. Seattle, WA or Remote (US).
- Senior Frontend Engineer at Pento. £80-92K. Remote (Global).
- Senior Fullstack Engineer at Synthesia. £70-110K . Remote (Europe or US Eastern time) or onsite (London, Amsterdam, New York).
- Senior Backend Engineer at Synthesia. £70-110K . Remote (Europe or US Eastern time) or onsite (London, Amsterdam, New York).
- Senior Frontend Engineer at (catch) Health. $90-120K + equity. Remote (North America).
- Staff Backend Engineer at Pento. £95-115K. Remote (Global).
- Senior Full Stack Engineer (Laravel / Vue) at RXMG. Remote (Global).
- Sr. Cloud Infrastructure Support Engineer at RXMG. Remote (Global).
- Founding Engineer, Front End and API at Causal. $175-225K - equity. Boston.
- Senior Full Stack/Frontend Engineer at Vitally.io. $180-270K. New York or Remote (US).
- Senior Software Engineer - Core Platform at CAST.AI. €72-€96K. Remote (EU).
- Senior Software Engineer - Cost Optimization at CAST.AI. €72-€96K. Remote (EU).
- Solution Architect at CAST.AI. Remote (EU).
- Senior Software Engineer - Security at CAST.AI. Remote (EU).
- Solutions Engineer at Pigment. $70-120K. New York or Toronto.
The above jobs score at least 10/12 on The Pragmatic Engineer Test. Browse more senior engineer and engineering leadership roles with great engineering cultures, or add your own on The Pragmatic Engineer Job board and apply to join The Pragmatic Engineer Talent Collective.
Want to get interesting opportunities from vetted tech companies? Sign up to The Pragmatic Engineer Talent Collective and get sent great opportunities - similar to the ones below without any obligation. You can be public or anonymous, and I’ll be curating the list of companies and people.
Are you hiring senior+ engineers or engineering managers? Apply to join The Pragmatic Engineer Talent Collective to contact world-class senior and above engineers and engineering managers/directors. Get vetted drops twice a month, from software engineers - full-stack, backend, mobile, frontend, data, ML - and managers currently working at Big Tech, high-growth startups, and places with strong engineering cultures. Apply here.