How Google, New Relic, Palantir and Yelp manage their code review process.
A lot has been said about the reasons to perform code reviews as part of the software development life cycle, their benefits, and how effective this practice can be in detecting and preventing deficiencies from being introduced into production code.
Since you are reading this, you are probably already convinced that this is a must-have practice that should be part of a healthy process in our engineering organizations. However, if done incorrectly, code reviews can be a source of frustration, conflict and wasted time and effort.
Many of the barriers to successful and effective review programs are social and cultural in nature instead of technical. So, the question becomes about how to guide humans into an effective way to communicate feedback to one another about their code.
This article is a summary of my findings from surveying the best practices employed by four high-functioning engineering teams in recognized tech companies.
The specific recommendations and rules in place by these companies may vary, however, there are some common topic areas that emerge which form a good blueprint for other engineering teams looking to establish their own guidelines.
The companies that were included in this review are some of those that have publicly facing documents outlining their approach: Google, Palantir, New Relic, and Yelp.
This article is organized into two main sections. The first section presents an overview of the approach that each company has taken to code reviews, their motivations and how they present guidelines to their engineers. The second section summarizes their best practices and recommendations organized in common topics or categories that emerged from a detailed review of their guidelines.
Finally, I offer some advice on how to move forward with the creation of your own guidelines.
Google has a lengthy section dedicated to code reviews in their GitHub engineering practices repository.
They basically break it down into two guides, one focused on the Reviewer and one focused on the Author (i.e. the developer making the CR).
The overarching principle is that every CL (Changelist) for review should improve the overall code health of the system, even if the CL isn’t perfect. The key here is that they acknowledge there is a balance to be struck between code owners wanting to strive for an ideal state and developers wanting to make progress and deliver value to the business. To them, there is no “Perfect Code” only “Better Code”.
The overall tone of their document is very “Google”. Straight to the point and written in a very concise and practical way. It is long enough to be comprehensive but not too long that nobody would want to read it. It is also broken down into sections, which makes it easy to be used as a quick reference guide.
Sometime back in 2018, the team at New Relic decided to rethink the way some of their development teams provided feedback in code reviews. This initiative came from an actual emerging problem where team members were delving into unhealthy patterns of non-constructive feedback.
To them, the root cause of the issue was the inability to differentiate between objective and subjective feedback and how to provide and handle each type. They make the point that engineers are trained to see feedback purely as a functional quality assurance task (objective) instead of as part of a creative process (subjective). To illustrate this, they cite the example of academic creative writing programs where there are formal workshops to train students to provide critical constructive feedback.
New Relic defines guidelines that aim to support engineers in dealing with the review of subjective decisions that are made by CL authors, which can be the source of conflict and frustration. These guidelines are cemented in their decision to make the author of a CL the ultimate person responsible for the correct execution of a change and whose opinion would carry the most weight in a subjective debate. The most important change they implemented was the explicit tagging of comments as blocking (objective) or non-blocking (subjective).
Another key takeaway from New Relic’s process is the use of coding standard sponsorships. If a reviewer feels very passionate about a certain subjective approach to writing code, they can suggest that it becomes a standard during retrospectives. For this to work they must describe the change in an objective way, so it can be assessed by reviewers during code reviews and they must be ready to support the change with training and documentation around it.
You can find the list of guidelines and the full story for their motivation in their engineering blog post about it.
In Palantir’s approach to code reviews, they emphasize their motivation to have code reviews in the first place. These motivations provide context for the guidelines themselves.
They see code reviews as an opportunity for authors to grow and improve themselves as developers by receiving feedback. It is also a growth opportunity for reviewers as well since they can learn from others by reviewing their code.
Another key motivation is to have consistency and improve the legibility of the codebase. Which prevents bugs, facilitates collaboration, promotes reusability and favours future-proofing.
Finally, for Palantir, code reviews avoid accidental, structural and integration problems as well as ensure compliance, which is especially important in highly regulated environments.
Their guidelines focus on the topics of preparation for code review, the anatomy of commit messages and what to look for when reviewing. They also provide several examples of “good” vs “bad” reviews that include code samples.
The engineering team at yelp has spent years refining their process. Jason Fennel, their (then) SVP of Engineering shared their code review guidelines in their engineering blog.
Similar to Google, they separate the advice for authors and reviewers, outlining best practices for the submission phase and others for the review and discussion phase of a code review.
Their guidelines are very short and to the point and they provide concise checklists that can be useful at the time of submitting and reviewing code. Then they provide some final advice on handling communication and conflict.
Summary of Best Practices
The following sections outline the specific topics that were discovered as part of the survey of best practices. For each topic, there are tags for the companies that had recommendations in their guidelines around that topic. The list of best practices is an amalgamation of the advice found in the guideline documentation from all companies.
These are general guidelines that apply as an overarching principle for code reviews.
Google, New Relic and Palantir
- Technical facts and data overrule opinions and personal preferences
- The style guide is the absolute authority. If it is not in the style guide, then defer to what is already there or the author’s preference.
- Aspects of software design require the author to prove that their approach is one of several possibilities that are based on standard practice, in which case their approach should be accepted.
- Provide subjective feedback as such. That is a matter of personal opinion.
- Code does not have to be uniform. It is ok to have different coding styles that still conform to standards and style guides
- Leave expensive human-based review time to business logic. Style, syntax, and formatting should be checked by automated tools.
- Code reviews should happen after automated checks have passed.
Change request submission
Advice about prep work and how to submit a code change request for review.
Google, Yelp and Palantir
- Choose the right reviewers. People who own each area of the code or can provide meaningful feedback to improve.
- Pick a primary reviewer. Someone who will be responsible to ensure the review is executed thoroughly and provides ultimate approval.
- First-line of the commit message. The first line escription should be short, focused, and to the point. It should be followed by an empty line. It is a complete sentence of what is being done, if possible include why and how, written as though it were an order. (i.e. an imperative sentence).
- The full description. Should include information about the problem being solved, the approach and any shortcoming as well as information about how to test. It is also good to include a reference to a ticket #.
- Take time and effort to write good CL descriptions, they will be used to search previous work done by other developers.
- Take the time to review your diff and make your code easier to review (i.e. clean up, reorganize, comment, simplify and test)
Scope and size of change requests
This is advice on what to include in code change requests before submitting for review.
Google, Yelp, and Palantir
- Keep them as small as possible. There is a lot of advantages to doing so. Small means the minimal self-contained change. Your change addresses just one thing (avoid piling up multiple things in one). The key is “self-contained’. Don’t make it smaller than required because it loses context and clarity. Metrics about size can include the number of lines or the number of files affected(each time can define reasonable limits).
- Reject large requests. Reviewers should reserve the right to reject overly large CLs and request to be broken down. So the developer might as well do it in smaller chunks, to begin with.
- Some large CLs may be ok. Certain large changes can be the exception to this rule if they are the result of large dependency additions, deletion of files or refactoring changes from an automated tool that can be trusted.
- Keep refactors separate. Separate refactors from features business logic into separate CLs. (i.e. do your required refactors first and then implement your feature)
- Bundle test code along. Include test code in the same CL that has the feature they test.
What to look for as a reviewer
This is advice for reviewers so they know what to focus their attention on while performing a review.
Google, New Relic, Yelp and Palantir
- Architectural design. Make sure you understand all components and how they fit together in a sensical way. Assess usage of abstractions and dependencies. Did the author follow established patterns?
- Correctness. Will it break the build?
- Completeness. Are there any ‘todo:’ comments left? Any commented-out code?
- Functionality. Does it do what was intended? lookout for edge cases, integration issues etc.
- Complexity. Is it more complex that needs to be, reducing clarity?
- Maintainability. Will it be difficult to change down the road? Are they reinventing the wheel because of a lack of awareness of existing reusable code? Does it break existing tests? Is it backwards compatible? Are comments and commit messages clear enough?
- Security and performance. Are there any glaring issues? Look for robust authentication and authorization implementations, weak configuration, secrets management, input sanitization, n+1 queries, table indexing etc.
- Test coverage. Are they including the appropriate level of testing?
- Naming. Did they pick good representative names for variables, functions, classes etc.?
- Compliance. To the style guide, coding standards and other company security policies.
- Documentation. Inline via comments, readme.md or formal docs
- Every Line. Look at every line, don’t just scan over full files or class implementations.
- Context. Make sure you review with a good understanding of the context of implementation.
- Good things. Praise the developer for good practices that are worth highlighting
Steps to review
Advice on the specific steps to follow to perform a review.
Google and Yelp
- Overview. Do one pass as a broad view of the change, description, and context. Validate that it makes sense and can be understood at a high level.
- Review the main parts. These are the broad design strokes of the change at a high level. If there is a problem here, communicate it immediately and avoid wasting time with the smaller parts.
- Review smaller parts. Once you verify there are no major problems, take the time to review all the individual small changes and files in a logical order.
- Provide comments. Indicate clearly blocking (objective) vs non-blocking (subjective) comments and provide links to style guides or coding standards when possible.
Best practices around reacting to requests for reviews in an expedited way.
Google, Yelp, Palantir
- As fast as possible. (i.e. 1 business day, or think about hours not days) or be explicit about your expectation for turn around when submitting the review
- Don’t interrupt yourself to do a code review. Wait to complete your focused tasks, but don’t start another one before doing the code review.
- Acknowledge of receipt. Send a quick note that you have seen the request and will get to it asap. Or acknowledge that you won’t have time and the author should seek another reviewer.
- Break down large commits into smaller ones to unblock or split code review tasks to make the review process faster.
- Account for “emergency” situations that require a different and more expedient code review process.
Best practices to provide feedback to the authors that requested review.
Google, New Relic and Palantir
- Be kind and courteous using neutral language. Provide feedback in a respectful manner and make it about the code and not the developer (i.e. avoid ‘Why did you…”, and use something like “The code here makes it…”). Avoid absolute judgments (i.e. “this would never work”). Ask for clarification before assuming ignorance.
- Explain your reasoning as much as possible, unless it is obvious or non-compliant with the style guide.
- Try to be helpful and offer suggestions for alternate approaches. It is the responsibility of the developer to fix it, but you can help.
- Provide actionable feedback. Avoid vague comments based on personal opinion, or be clear that it is your opinion but not a change request.
- Clearly identify blocking vs non-blocking comments and be specific about how you expect the author to handle your comments (i.e. change something vs. consider your opinion vs. answer your question)
- “Ask” for changes, do not “demand” them.
How to address feedback
Advice on how to process and respond to feedback provided by reviewers.
Google, New Relic, Palantir
- Don’t take it personally. Remember the goal is to improve the codebase.
- Don’t reply in anger. Even if you don’t agree or you think they were personal in their feedback, wait until you are calmed before responding with a levelled head.
- Reply to every single comment. Even if to acknowledge non-blocking comments and communicate that you won’t act for the time being.
- Be ready to change your code. If the reviewer didn’t understand it, then it probably is worth improving (unless there was a misunderstanding or lack of contextual knowledge).
- Be open and consider the feedback carefully before responding or pushing back, even if you don’t agree.
- Take non-blocking feedback seriously and don’t just ignore it
- If consensus is not reached, use the ruling principles and practices for conflict resolution.
- Teams should define clear rules for updates to code under review (i.e. rebase or merge? Squash commits or add new ones? etc.)
Ways to handle pushback from authors regarding feedback provided during a review.
Google and New Relic
- Take close consideration of any pushback. The author developer might be right, they are closer to the code and they might actually have a better insight than you after all.
- If after consideration you still insist on your request, continue to be respectful and explain things differently and always make your points with technical facts and data.
- Don’t agree to “Clean up later”. Either agree that the change is not required or get the developer to clean it up now. Cleaning up issues “later” rarely happens and becomes more expensive.
- If consensus is not reached, use the ruling principles and practices for conflict resolution.
- Mitigate complaints about the strictness of code reviews by increasing speed and giving developers time to adapt to the new practices
Advice on ways to resolve conflict when there are disagreements between reviewers and authors.
Google, New Relic, Yelp and Palantir
- Author and Reviewer should try to reach a consensus through polite and professional conversations using all other guidelines and best practices.
- Switch to real-time communication. It is preferred to have face to face discussions or phone conversations than lengthy review comment threads.
- When consensus is not reached, escalate to more team members, senior engineers or product owners.
- Don’t let PR sit there for a long time because consensus can’t be reached
- Identify who is responsible for the change going into production (i.e. reviewer or author). In a purely subjective debate, the opinion of this person should carry the most weight.
- If the situation is unprecedented, compromise in order to move forward, and propose a change to style guides or coding standards for future reference.
Through this survey, it was evident that there was a lot of common ground among the different companies. I hope this concise summary of best practices and the identification of topics can inform your own code review guidelines for your organization.
One important observation is that although companies offered different points of view and advice, they didn’t really contradict one another in their guidelines. This tells us that, putting companies and projects aside, writing and reviewing code is a pretty standard activity done by humans. And code and human nature are pretty much universal concepts.
You may take a lot of the points outlined by these organizations as is. But my advice would be to look internally first and figure out the dynamics of internal company culture and communication patterns and then adapt these guidelines to what works for your own company.