When AT&T introduced code reviews, they saw a 14 percent increase in productivity and a 90 percent decrease in defects. As well, Jet Propulsion Laboratories estimates saving about $25,000 per code review by finding and fixing code defects at an early stage.
These are just a couple case examples reported in Steve McConnell’s book “Code Complete” regarding the impact code review has on businesses. In addition to saving companies time and money, engineers who engage in code reviews gain professional development.
With pull requests, engineers can flag errors, request changes and streamline a cohesive language between the whole team. Just as writers get better by reading other writers, engineers get better by reading other engineers’ code — at every level.
We talked to 18 engineering professionals about how their teams manage code reviews. They told us how they developed their best practices while fostering a positive code review culture, as well as what to do when differences arise.
Code Review Best Practices
- Ensure pipeline is standardized and clear
- Test code prior to review
- Ask questions and don't make assumptions
- Have a review checklist
- Review all code, regardless of who wrote it
Pinpoint
What best practices does your team follow when doing code reviews?
Because of SOC 2 compliance, we must do things like lock down merging into master and ensure that all pull requests (PRs) have approval. If there’s an approval but a new commit comes in, we revoke the approval and start over.
Apart from those strict, compliance-related processes, we try to have three reviewers with at least one reviewer running the code. We also try to be cognitive of the PR review load and make sure not one person is burdened with a ton of PRs a day. GitHub has a new round-robin feature for this but we haven’t implemented it yet. By doing this, we get the added benefit of knowledge-sharing and awareness around code changes in the product.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Visibility, collaboration and efficiency are core values. If there’s a new approach to some code, architecture or pattern, someone will usually prototype it out and share it with the team for feedback. This allows us to agree upon the right approach quickly. As the saying goes, “a prototype is worth 1,000 meetings.”
For example, we recently switched to React’s Router component. Some were skeptical that React Router met every single one of the requirements we had. Rather than make assumptions, someone on the team prototyped it out and demonstrated how it would tackle some of our advanced use cases. We reviewed it as a team, got quick consensus and made the pivot.
For smaller, code-level reviews, the reviewers can usually come to an agreement in the PR or after talking it out in person. Ultimately, the tech lead on the team has the final say.
"Analyze and scrutinize the code, not the person.’’
What advice do you have for fostering a positive code review culture?
Analyze and scrutinize the code, not the person. Make sure PRs are spread out among the team. Encourage team members to label nitpicky things as such, so it’s clear what’s minor, versus what is an important code critique. Enable the team through tooling for quick review.
For example, on the front end, Zeit or Netlify do PR previews of a build that improves velocity. If you have to spin up an entire stack for every PR, it can discourage a good PR review.
Piaggio Fast Forward
For Tom Donahue, the job title “human-robot interaction engineer” is just as cool as it sounds. He explained how his team at Piaggio Fast Forward makes thoughtful adjustments based on actionable feedback during code reviews. His main takeaway? Review comments are not commandments. Take an investigative approach and talk through any uncertainties.
What best practices does your team follow when doing code reviews?
One of the most important aspects of code reviews is making sure the pipeline of a review is standardized and clear. To accomplish that, you need to define when your branch should be reviewed, who it will be reviewed by and what the gating steps to getting your branch merged into the mainline are.
When to review comes down to making sure the review will yield the best possible addition (or subtraction!) to the codebase. We try to avoid a lot of premature reviews by relying on as much automation as possible. If your branch isn’t passing builds and tests or isn’t linted, it’s not ready. Keeping on top of code reviews takes a lot of effort and time for everyone involved, so it’s vital that time is respected. Relying on as many automated checks as possible eliminates a number of questions and concerns for a reviewer right off the bat.
We make sure every repo has a designated set of code owners who are well-versed in that area. We also make it clear that everyone is welcome and encouraged to review any incoming code that they’re interested in.
Finally, once a feature or bug fix enters code review, it’s important to be transparent about where it is in the process at any point. This can be a particular pain point for robotics companies like us due to the complex nature of reviewing, testing and verifying code on the robot. We do our best to make sure to track the progress of a PR through the various review stages so that authors are kept as in the loop as possible.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
It’s important to recognize that some differences are more important than others and require different approaches. Code reviews are there to produce safer, more reliable, more maintainable and better-performing code (in that order). With that in mind, if a difference of opinion is related to whether or not a proposed solution will or will not lead to those outcomes, it requires a more in-depth, face-to-face discussion to hammer out the concerns.
If someone appears to be taking a more simplistic approach to save time, it’s important to dig into why they might be doing that. Is it because they’re up against a tight deadline? Maybe that needs to be re-evaluated. If the difference of opinion is about something that goes against our adopted style guide, then the style guide wins.
Always provide useful, actionable feedback. It’s perfectly acceptable to suggest an alternative that you think the other person might not have thought of (which they are perfectly free to ignore). But don’t expect an author to change anything unless there’s a substantive concern behind it.
"Almost all code can be improved in some way.’’
What advice do you have for fostering a positive code review culture?
Make it absolutely clear that code reviews are not personal. Though it’s natural for an author to wrap their pride up in their creation, just as any other writer or artist would, the end goal is to ensure that the stability of the product is maintained during any code modifications.
Almost all code can be improved in some way. Code reviews are not an indictment of the author’s intelligence or experience. A rising tide lifts all boats. The more you can learn from everyone’s experience, the more you can share and take to the next feature you write. On the flip side, a reviewer must be mindful that they lack full context. Genuinely inquire why an approach was taken and potentially suggest an alternative.
Finally, it’s important to remember that not all review comments are created equal. The author may have good reasons for the choices they made. If there is confusion, start a dialog and dig in. A reviewer should never assume they know best.
Perceptive Automata
Perceptive Automata Senior Software Engineer Kevin Sylvestre makes professional decisions based on hard data. It’s a crucial detail, given the company’s contribution to the autonomous vehicle space. Sylvestre said that when any code or tool is in doubt, his team prototypes the scenarios in question to understand which route is quantifiably best to pursue.
What best practices does your team follow when doing code reviews?
Code reviews are performed during and/or after the implementation of new software features. We generally complete reviews before the new feature is merged into the primary software branch. Also, we test the code prior to review to confirm that it performs as expected. Usually, reviews are performed informally using the GitHub interface, though we are now instituting more formal sit-down reviews where the developer walks through the design with a reviewer or two.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Generally, the code “owner” has the final say on how a design is implemented. However, they must ensure that all the relevant stakeholders are satisfied with the design and implementation, which requires carefully considering all the feedback they receive and justifying their choices based on relevant data.
For large or complicated designs, we sometimes implement prototypes to compare their relative merits. For example, when we considered how to redesign our build system, there was some dispute that the new design would be an improvement over the last one. We implemented a prototype of the new design and compared the two across various criteria, such as the amount of effort involved in adding a new module or dependency. The prototype provided some hard data that we could use to compare with the old build system, and soon everyone agreed that the new system was better.
"Make sure any criticism can be justified with some hard data.’’
What advice do you have for fostering a positive code review culture?
Code reviews occur when multiple developers come together to consider a design solution and its implementation. As such, all the general rules of good teamwork apply: stick to constructive criticism, have respect for your fellow developers, keep an open mind, etc. Try to keep the review focused and on track. Sometimes sit-down reviews will prompt new ideas and side conversations.
Also, make sure any criticism can be justified with some hard data. Style preferences are irrelevant since they’re handled by a good coding standard. Any disputes about the design or implementation should be rooted in real, practical considerations about performance or requirements.
Panorama Education
Panorama Education Software Engineer Jemma Issroff focuses on her team’s shared end goal as it relates to the code review process, whether she’s the reviewer or committer. She explained that engineers at Panorama use “I” language so as not to project issues with the code itself onto the person who wrote it.
What best practices does your team follow when doing code reviews?
We use “I” and “we” language instead of “you” language. For example, we say: “Can we add test coverage here?” instead of, “Can you add test coverage here?” This underlines the point that the author and reviewer share the same goals instead of implying an adversarial relationship.
We prefer to ask questions on code reviews rather than making assumptions. If someone made a decision, they likely had a reason for it. Instead of assuming one thing, ask a question to allow them to articulate their point of view. An example of this might be: “Why do we need this method?” as opposed to something like, “This method is unnecessary.”
We also have a review checklist of items to look for during code review. This allows us to standardize reviews and ensures that the reviewer does not miss important parts of the process. The checklist has items such as, “Is this zero downtime safe?” and “Are there performance risks here?”
"If someone made a decision, they likely had a reason for it.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
It depends on the nature of the disagreement. If it’s a stylistic disagreement that our linters don’t cover, the reviewer will often indicate that the author can feel free to make a choice. I’ve seen reviews where the reviewer will approve a PR and leave optional stylistic comments.
If the disagreement is a bigger-picture architectural difference of opinion, we will often discuss it in person. If the author and reviewer disagree at this level, it is likely that they have different understandings of the interests that the PR is attempting to solve and why one approach might be beneficial.
We have a philosophy of valuing an influence over authority. Explicitly, this means that there is never a question of seniority when a difference of opinion arises. Instead, both engineers will discuss why they would approach it a certain way and will come to an agreement of which approach ultimately makes more sense. This has happened to me numerous times. And as both the author and reviewer, I find it to be the most productive way to approach a disagreement.
What advice do you have for fostering a positive code review culture?
Code review is most effective when it’s seen as an opportunity for both the author and the reviewer to learn. It’s significantly less effective when it’s viewed as critical of a developer or something to be rushed. I have grown most as a developer from thoughtful code reviews in which I had long back-and-forths with the reviewer.
Try to frame every part of a code review as an opportunity to learn and to understand that code review shouldn’t be a small step right before code merges. Instead, it’s a critical part of the process. Although it requires more patience, this approach will ultimately lead to higher quality code and better engineers.
SmartSense
At SmartSense, seniority takes a back seat when it comes time to review code. Software Manager Sean Leary said at SmartSense, two engineers review each developer’s work, impartially suggesting any improvements they deem necessary. For Leary, the most important advice to keep in mind is that, at the end of the day, everyone’s on the same team.
What best practices does your team follow when doing code reviews?
If taken out of context, code reviews can be seen as stressful and demoralizing. However, they are invaluable as a QA measure to catch issues that engineers may miss. With that being said, follow some simple guidelines.
First, two engineers should perform a code review and approve changes on a Git pull request. It doesn’t matter if you’re a junior engineer or a director. Our code review process is unbiased. If you create code, it’s being looked at by two other engineers.
When engineers make comments and suggestions for changes, they do so impartially, constructively and humanely. The comments we do and should see are along the lines of, “This variable name is a little ambiguous, consider changing it to X.” These types of comments are constructive, actionable and can drive a conversation if needed.
"If you create code, it’s being looked at by two other engineers.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
My teams are pretty close-knit and follow industry best practices. That being said, opinions do vary. I like to foster a culture of transparency, trust and communication. When a difference of opinion arises, it triggers healthy conversation. In the end, someone may have a better idea, which keeps us moving forward.
What advice do you have for fostering a positive code review culture?
Be kind, be actionable and do a jersey check. We’re all on the same team. When having your code reviewed, remember it’s not a personal attack. This process helps us find things we can and do miss. Code reviews can be a great learning tool for honing your craft.
Optiver
What best practices does your team follow when doing code reviews?
One of the most important protocols we have in regards to code review is approved reviewers. All merges are blocked unless approved by someone from the approved list, usually a domain expert. This ensures there is only one entry point into the master branch and at least two people are familiar with every commit, which prevents surprises and fosters a culture of shared ownership of the code.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Never be afraid to take things offline. If a reply chain is longer than two replies, the problem is probably large enough that it’s more expedient to discuss in person. This often ends in bringing in multiple people with different viewpoints and almost always generates a better and quicker solution than having a 20-reply thread of comments. Don’t forget to put the resolution in the review though. The code review is a diary of what changes were made and why, and can be an indispensable resource when looking back at a previous change several months or years later.
"Never be afraid to take things offline.”
What advice do you have for fostering a positive code review culture?
Embrace the criticism. Use it as a chance to improve your code and help others improve theirs. I am infinitely more scared when my review gets zero comments compared to when it gets 100. I think a lot of people wrongly see a lot of comments on their code review as a sign that they failed. They should see it as a sign that someone really took the time to thoroughly analyze their changes and that everyone’s code will be better for it.
Documoto
Documoto’s technology is designed to help equipment manufacturers improve technical publishing accuracy and lead time for equipment identification. In order to operate their software optimally, code reviews are essential to decrease errors that would show up in production. Software Architect Paul Skokan said code reviews are also an opportunity for engineers to transfer knowledge.
What best practices does your team follow when doing code reviews?
Code reviews consist of a few different elements. First and foremost, the code must address the problem described in the ticket. If the code does not, then it must be revisited. Next, we can look into how the problem was handled. Being a company that encompasses the startup philosophy, we must be agile and flexible.
Readability is always valued, meaning code can be shared and understood by your peers. Elegant solutions, while generally requiring fewer lines of code, usually don't hold up to this standard. Code can be handed off at a moment's notice, which means that fluency is critical.
Finally, code reviews can and should be used as teaching opportunities. They give us easy access points between fellow developers, who are able to spread both business knowledge as well as code paradigms without breaking cadence. Code reviews allow Documoto developers to grow as engineers and understand customers' needs.
"Code reviews can and should be used as teaching opportunities.’’
When there’s a difference in opinions about how to write specific code, how does your team handle it?
The final decision will always land on the person responsible for the particular ticket. We are a small enough company that everyone knows each other well and code reviews can be very personable. Recently, we had a situation where some of our older code used a non-standard pattern. We had to decide whether to move away from the current paradigm, be consistent or use a standard archetype. In this case, we decided to pivot from the newest code and add comments to address the other code later.
This allowed us to move into QA faster and keep the change scope to a minimum. The key here is that none of our practices are set in stone. Sometimes significant changes are required, and best practices must come first. Other times it is imperative to meet deadlines. We always strive to do the right thing, which means balancing all changes against timetables.
What advice do you have for fostering a positive code review culture?
Code reviews should be an opportunity for transferring knowledge and allowing individuals to grow while meeting customer needs. A big part of code review is allowing everyone to own the code, and ensuring that no one person owns a ticket or a project.
One team member can start a project, and another person can finish it. This creates some objectivity when it comes to writing solutions, which comes into play with code reviews. With a growth mindset, it's important to be flexible in decisions and not dwell on past mistakes but move forward in ways that work for your organization. Know your team and help them foster a sense of growth and accomplishment.
Smartwyre
Smartwyre develops software for the commerce of agricultural inputs and services. In order for that software to function, Lead Software Engineer Kevin Walter said it’s important to be kind, not critical when conducting code reviews.
What best practices does your team follow when doing code reviews?
Code reviews should be a collaborative show-and-tell process. The code author steps through their code and provides a brief explanation of each step along the way. The reviewer has a responsibility to listen attentively and ask questions or point out sections of interest. We generally try to avoid jumping to other sections of code that take the process out of the natural flow. We like our code reviews to be conducted in such a way as to feel almost like pair programming. Most importantly, the reviewer is careful not to turn a code review into a performance review. We prefer an environment where developers can expect their mistakes to be met with empathy, positivity and with the goal of improvement for everyone involved.
"We like our code reviews to be conducted in such a way as to feel almost like pair programming.”
When there’s a difference in opinions about how to write specific code, how does your team handle it?
Our team strives for consensus as much as possible. It's important that each team member feels their opinion is valued, and is considered in a thoughtful way whenever we're not aligned. In cases where a consensus is difficult, we look for subject matter experts to provide additional guidance. If the likely outcomes of different approaches are more or less the same, it may come down to which team member chooses to take ownership of that particular body of code. Our team values ownership over seniority.
What advice do you have for fostering a positive code review culture?
Be kind, not critical. As detail-oriented professionals, it's often very easy for developers to become hyper-critical during code reviews. Strive to foster positivity, two-way knowledge sharing and camaraderie. When developers aren't dreading the code review process, they often come up with some pretty amazing think-outside-the-box solutions. Coincidentally, when reviewers are free to set aside some of the more dogmatic approaches to code review, they have the opportunity to look at solutions with a more open mind, and perhaps learn a thing or two along the way.
Payfone
Payfone validates millions of mobile and digital identities, helping companies provide an enhanced customer experience. Jennifer Risdon, senior software engineer, said the main goal of their code reviews is to improve code quality. When a developer knows the code they wrote will be reviewed, it’s typically a driving force to produce well-written code, she explained.
What best practices does your team follow when doing code reviews?
When a developer finishes a development task, he or she submits a pull request, which is available to the entire development team for review. Not everyone has to review at that time, but the PR is always there for developers to go back and refer to when they may have a task similar to what the PR covered. We currently require one approval to merge the code to the branch.
When doing code reviews, we look for consistency and strive to keep our codebase consistent and subscribe to the DRY (don’t repeat yourself) principle of coding. The code should be clean and readable. Also, a PR should only be submitted once it has had the proper unit tests written around it, and the developer has run those unit tests during a self-review of their code.
If any refactoring is done, it should be proven that the code still functions in the same manner and produces the same results.
"Mutual respect is key.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Typically, the reviewer and developer will get on a meeting or Slack thread to discuss the code in question. Sometimes, the product owner will get pulled in to clarify requirements, and a decision might then be made about which way the code should be written. Other times, the reviewer may be a senior developer who is more familiar with the codebase and may have knowledge of libraries or abstractions that the developer didn’t know about at the time the code was written. This continuous collaboration between team members effectively helps produce code and serves as a learning opportunity to become more efficient at writing future code.
What advice do you have for fostering a positive code review culture?
Mutual respect is key. Always keep in mind there are several different ways to solve a problem, and code reviews offer an opportunity to share knowledge. Being able to have a say in how things are coded empowers developers and contributes to team culture.
HouseCanary
HouseCanary uses data and analytics to predict the future of the U.S. real estate market. To make accurate changes to the company’s software, Senior Mobile Engineer Worth Baker said trust between teammates is necessary when differences arise in code reviews.
What best practices does your team follow when doing code reviews?
On HouseCanary’s mobile team, the high-level goal of our code review process is to ensure that code is bug- and crash-free, readable, maintainable and idiomatic to the platform and language in question. Perhaps more importantly, the core value of our code review process is trust: Trust that the author of the code strived to make reasonable, intelligent choices and trust that the reviewer wants to contribute to the health of the project, and not just to prove that they’re “better” at something.
We attempt to strike a balance between in-depth reviews and deference to the experience of the author. Each line of code is considered and questioned carefully, but with the general assumption that the author’s decisions were made with a more complete perspective than may be available to the reviewer. If you were to observe an ongoing review, you’d see reviewers asking for clarification and perspective before offering advice on logic or code structure, pointing out code that conflicts with established syntactic style, and an attempt to avoid wasting time on “bike-shed” style discussions that wouldn’t improve the project as a whole.
By relying on trust in our team as a whole to produce quality code, we end up with a code review process that is perhaps a bit more fluid than other teams, but that has also produced several stable and maintainable projects.
"Like any positive culture, trust and respect need to be at the center of critical processes.’’
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Continuing the theme of trust, when disagreements arise, we start with the assumption that each opinion is at least reasonable from some perspective. And if a difference of opinion can’t be resolved by talking it out, we’re all fine with letting the manager of our team have the final decision-making authority.
Most significant differences in opinion arise before the code review stage, when we’re discussing the architecture of a new feature, or the response to some bug or issue. For example, we recently implemented a feature where, when users searched for a city or ZIP code, we would display the geographic boundary of that place on our app’s map, and restrict the displayed property results to within that boundary. We ran into some performance issues with particularly complex boundaries, and different members of our team favored different solutions.
Both solutions were reasonable, but ultimately we decided to implement the simplification algorithm since it would provide a better and more accurate user experience, and meshed more neatly with some future enhancements on our roadmap.
What advice do you have for fostering a positive code review culture?
Like any positive culture, trust and respect need to be at the center of critical processes. Trust that team members will make reasonable choices and respect those choices when differences arise. With those two values in place, authors as well as reviewers can be encouraged to be opinionated, and to stand up for their decisions, without fear of awkwardness or confrontation. The end goal should always be creating the best possible product for users and customers, not winning or “being right.”
IHS Markit
IHS Markit transforms financial data into elegant user experiences. To deliver quality code, Senior Principal and Director of Software Engineer Ryan Smith said respect for the team, their goals and their processes is critical.
What best practices does your team follow when doing code reviews?
An important factor for success is to ensure that there is adequate and relevant information available for code reviewers to effectively review the change. That typically starts with a well-crafted change ticket that will ensure the work is well-justified, described and scoped.
The rules and guidelines for approaching the code review process should be agreed upon and followed by all team members and reviewed periodically.
"Always start with an assumption of good intent and pose questions accordingly.’’
When there’s a difference in opinions about how to write specific code, how does your team handle it?
It is important to understand that writing code is a subjective experience and healthy debate is an important part of the process.
We ask our team to justify comments with relevant information or experience but understand that others may have contrasting examples. As well, we seek compromise and if that cannot be achieved then the technical owner or architect is responsible for resolution.
What advice do you have for fostering a positive code review culture?
Like most things in life, a good code review practice starts with respect — a respect for each other, a respect for the process and a respect for the goals of the team.
Always start with an assumption of good intent and pose questions accordingly. The process should be highly valued and form an integral part of a team’s workflow.
Verifi
Taylor Caswell
SOFTWARE ENGINEER
What best practices does your team follow when doing code reviews?
At Verifi, we use Bitbucket’s pull request feature to facilitate code review. This allows us to assign reviewers, track comment threads on specific areas of code and easily see the code changes in question.
By reviewing code frequently, we prevent the dreaded situation of a code review that feels like it contains the entire world. Frequent review means small, easy reviews. It allows reviewers to give the proper attention to the changes being made.
We don’t want code review to become a bottleneck in our development process. This means that we have to be well-disciplined when it comes to addressing outstanding code review in a timely manner. Providing feedback early in the development process helps us avoid wasted work and wasted time. Additionally, we certainly don’t want anyone sitting around for days waiting for someone to review their code.
Every developer has their own strengths and weaknesses. When reviewing code, take this into account. If you are the domain expert, pay careful attention to the domain. If you have a knack for clean code practice, keep your eyes out for that in particular. I wouldn’t recommend that every developer try and look for everything in a code review. We work in teams for a reason. Do what you do best.
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Differences of opinion are common, normal and even beneficial. Code review brings multiple voices into the conversation around an implementation. When there is a disagreement in a code review between the implementer and another team member, we call an informal meeting and gather all of the developers on the team together. Then, as a team, we discuss the problem and weigh the solutions. Sometimes we’ll pick one of the two options in contention. Other times, we’ll come up with an entirely new solution that exceeds the initial disagreement entirely. By calling the team together, we eliminate the “me against you” mentality that is all too easy to fall into. It is no longer one developer’s opinion against another’s.
And, speaking from experience, people (including myself) are more likely to be comfortable having their initial code changed or proposal overruled if it comes from an inclusive team rather than a single opposing developer or manager.
"Avoid comments that feel like personal attacks with a simple trick: leave them in the form of inclusive questions.’’
What advice do you have for fostering a positive code review culture?
Maintaining a positive code review culture is essential to having any form of functioning code review. When things get negative, code review becomes a major source of team dysfunction.
Avoid comments that feel like personal attacks with a simple trick: leave them in the form of inclusive questions. Instead of writing, “This method is too long,” try, “Can we shorten this method?” Additionally, make questions inclusive versus exclusive. For example, ask, “Why would you query the database like that?” versus, “How did we come up with this query?”
In both examples, the first comment could have been completely innocent. But conveying intent through text is difficult. The implementer could easily feel attacked or insulted. By changing the language to a question, we turn commands into conversations and we give the implementer the ability to say “no.” By changing the language to be inclusive, we show that we are on the same team.
As a personal rule, after just one or two exchanges, I take the conversation out of the code review and right to the developer’s desk. Long comment threads can easily blow up emotionally, slow down the review and end up accomplishing nothing. Avoid all of that by simply talking to the other person. It seems obvious but it definitely bears repeating often. Don’t risk being misinterpreted or having unnecessary communication failure.
Dailypay
When it comes to issuing employees’ paychecks early, no one wants to deal with a technical glitch. Vice President of Engineering Paul Rodgers said his team at Dailypay keeps pull requests small and avoids nitpicking in order to keep the code review process seamless.
What best practices does your team follow when doing code reviews?
We try to keep pull requests small, since reviewing large PRs can be overwhelming. Twelve files or fewer is a good rule of thumb. The first thing we look for is testing. Code review happens only when passing tests have been written. Reviews should aim for objective measurements of performance and complexity, where possible. Benchmarking is better than “it looks better this way.”
We also try to think about code review in a broader context than you might get just from looking at a difference. We ask, “How critical is this file?” and, “What are the risks of changing that function in the overall system?”
Code review is a great way for developers to learn about a system, so often review comments will provide helpful insights about how some piece of the app works, even if it isn't directly relevant to getting the code merged.
Finally, we never do code reviews in a hurry. It's better to wait than to merge fishy code because a feature is “urgent.”
When there's a difference in opinions about how to write certain code, how does your team handle it?
We try to avoid nitpicking about subjective code style. A linter helps; it doesn't matter so much what the style rules are, just that everybody follows them rather than debating them. As an example, there are many different ways to write a Ruby on Rails service class, and the differences are largely stylistic. Our team picked one way of doing it, and we all do it that way.
"We try to avoid nitpicking about subjective code style.”
What advice do you have for fostering a positive code review culture?
Pair programming is helpful for getting developers comfortable with constructive feedback. It's also important that everyone on the team does code review, not just senior developers or code maintainers. It can feel lousy if you're always on the receiving end of feedback. Everyone on our team does code review starting their first week.
Teachers Pay Teachers
Between budget cuts and disruptive students, many teachers already find their jobs stressful. Teachers Pay Teachers offers a marketplace where teachers can find original educational sources like lesson plans at a discounted cost, saving them time and money. To make sure the site runs smoothly, Senior Software Engineer Courteney Ervin said that being self-referential when conducting a code review can create constructive rather than critical criticism.
What best practices does your team follow when doing code reviews?
When you do a code review, you are engaging with and evaluating someone’s work, and my team approaches code reviews as an extension of all of our collaboration. We have trust in each other’s efforts and abilities. We know that the coder is closer to the problem than the reviewer is and we acknowledge that no one knows everything.
The goal of a code review is to teach and learn from each other as a team, so we ask more questions. When changes are needed, we spend time clearly communicating our reasoning. Some examples of recent review comments of mine include, ”Can you explain this test to me?” and, “I haven’t seen this approach before. What were you thinking here?”
"The goal of a code review is to teach and learn from each other as a team.”
When there's a difference in opinions about how to write certain code, how does your team handle it?
When a reviewer doesn’t agree on an approach, they explain their perspective in comments. If the issue is minor or the reviewer lacked full context, the coder can decide for themselves whether to make the change now, later or never. If the back-and-forth is lengthy and the issue more significant, we often take the conversation offline and involve more of the team in a thorough discussion. At all times, we consider patterns in our codebase and weigh the cost of changing the decision later. All of that is to say, code reviews and any conflicts therein are an extension of the team’s overall communication practices.
For example, we recently had some rushed in-person discussions about a naming database table that didn’t land. The person who wrote the migration ticket made a decision and commented “Roast me” on their pull request, knowing it was a source of conversation. Our tech lead Jacqueline replied with some specific thoughts about the name and other team members weighed in. When we landed on our table name, the team was in agreement.
What advice do you have for fostering a positive code review culture?
Do your research. Reviewers should also be doing light research as they review a PR, when necessary. If there’s a relevant example you’d like the coder to see, provide a link. If you think a pattern or decision is different somewhere else in the codebase, take the time to find the relevant message, document or source code filename. This kind of support helps everyone feel like they are in it together.
If you are nitpicking, say so. My team has gotten down to prefacing a comment about variable names or capitalization with “Nit.” so everyone knows that we’re being dramatic and can be ignored.
Don’t sweat the small stuff. Say everything you need to, but don’t block a PR for minor, non-breaking issues or things that can be handled in a follow-up. It’s important to remember that code is always an iterative process. There are tradeoffs, of course, but if the first choice isn’t the right one, you can always go back and change it.
Keep it positive where you can. I’m a huge proponent of leaving praise for code well done. I thank people for teaching me something new with their code, leave an emoji blast when I see something I like, react to funny test data and even pass around whole memes.
Justworks
When entrepreneurs are running a company from the ground up, busy work can add up fast. Justworks gives companies access to benefits, automated payroll, HR tools and compliance support all in one place. Manager of Software Engineering Michael Matyus uses automation tools like Rubocop and Prettier to streamline consistency in his team’s coding styles.
What best practices does your team follow when doing code reviews?
When it comes to best practices for reviewing code, there are some obvious rules that we try to abide by: ensure pull requests are under a reasonable line number, separate refactors from actual business logic changes and ensure new or updated code has test coverage.
We also distinguish between “suggestions” and “blocking changes.” This paradigm helps put emphasis on the code changes that actually improve or fix specific business logic while giving the engineer freedom to use their best judgement on whether they have time to rename a set of variables.
Additionally, we have become more deliberate about how we use our time. For instance, we’ve been putting more attention on automation, like deferring to Rubocop or Prettier to guide our coding style. We have various working groups devoted to fostering consistency across all the different domains and teams.
"Don’t let “perfect” be the enemy of good.”
When there's a difference in opinions about how to write certain code, how does your team handle it?
A lot of the engineering is worked out before the code is even written. For instance, we have templates for tech specs and RFCs that help ensure we are being consistent with the way we communicate. This also helps with avoiding deep discussion in the actual pull requests because we’ve all got a pretty shared understanding of what’s going to happen before it happens.
We also have documentation around what to expect in a code review so that reviews can be as objective as possible. This helps eliminate disputes that are based on someone’s preference. For instance, by definition, syntax or coding style changes aren’t going to block a pull request from being merged, while committing untested code is considered a blocking issue.
Never forget, it’s important to aim for high-quality solutions, but don’t let “perfect” be the enemy of good.
What advice do you have for fostering a positive code review culture?
Writing clean code is as important as the context or justification for making the change in the first place. We’ve implemented a department-wide pull request template that helps us serve the code reviewer by providing the context for the code change. If a pull request is still hard to review, then we’ll do in-person walk-throughs of the code change. That way, questions can be asked and resolved in real time.
Aircall
Aircall designed an AI cloud-based phone system that allows companies to track call metrics and set up meetings all in one place. According to Xavier Durand, co-founder and director of U.S. tech operations, offering to give a review in return for receiving one is the best way to get a response from the team.
What best practices does your team follow when doing code reviews?
During a review, we want our engineers to focus on technical implementation rather than code quality.
We leverage our CI/CD process to check code lint and to run unit-testing on 98 percent of the codebase. We built strong and opinionated tools on top of GitHub pull request to ease the review process, checking the Git branch name and the PR title against patterns, associated labels and PR templates.
Each pull request also generates an ephemeral environment, allowing us to quickly test and validate the associated feature or fix. Having all that helps engineers to review any pull request with all the context needed.
"Define best practice with your team beforehand and stick with them.”
When there’s a difference in opinions about how to write certain code, how does your team handle it?
Before starting to build and code a new project, we all get to read the specs of the new feature during a technical design review. For our team, it’s a great moment to design a first technical implementation, and that’s when most of the technical decisions are made.
Once the code is written, reviewers are encouraged to suggest any changes to the author. If there’s a difference in opinions, engineers will very often sit together and rewrite the code together.
Different implementations can lead to the exact same business results. Define best practice with your team beforehand and stick with them.
What advice do you have for fostering a positive code review culture?
At Aircall, engineers are encouraged to review one another’s PRs during daily stand-ups.
“Can you review my PR and I’ll review yours?” is still the best way to get a quick review from the team; we even track the ratio between the number of reviews and pull requests an engineer has made.
The more steps you can automate in your code review process, the more your engineering team will be focused on the right thing. Feedback must be positive and constructive and never forget the main goals of a review: helping your teammate to grow and increasing your engineering standards.
Movable Ink
Movable Ink helps digital marketers create visual experiences that get their message across in stimulating ways. While the artistic designers are putting pen to paper, Software Engineer Kate Ruggeri is putting code to keyboard. Ruggeri said differences in code review can lead to better-proposed solutions on future projects.
What best practices does your team follow when doing code reviews?
I’m lucky to work with such smart, empathetic and curious folks who aren’t afraid to ask questions and challenge each other. Probably the best advice I’ve ever had for giving good code reviews is to not let a single line of code get past you that you don’t understand. The team also has clear goals on incorporating modern Ember practices such as switching over to use Ember Octane. We like to leave things cleaner than we found them.
"A healthy team is a diverse team of backgrounds and opinions.”
When there's a difference in opinions about how to write certain code, how does your team handle it?
It definitely depends on the person and the scenario. Most often there’ll be a thread of people sharing their ideas and concerns. Usually this back and forth will result in an agreement with an even better proposed solution, which is the best case scenario. When it’s something bigger, like if someone has spotted a key issue that got overlooked, we might have an informal meeting to discuss. We don’t believe in any one person “owning” code — the team is collectively responsible.
What advice do you have for fostering a positive code review culture?
A healthy team is a diverse team of backgrounds and opinions. Be critical, be open, be curious and always try to find at least one positive thing to say, especially if you request a lot of changes. Encouragement goes a long way.
ACCOLADE
Accolade
“If we can’t understand each other, we’ll never understand each other’s code,” said Accolade Senior Engineering Manager Ben Horst.
According to Horst, empathy is a key component of code reviews at the healthtech company. On his team, reviewers approach each review with the assumption that the developer had a reason for the way they wrote something. This attitude helps maintain a positive team culture and reduces the opportunity for misunderstandings.
Accolade produces a suite of platforms for employers that help their staff (over 1.5 million employees to date) manage their health benefits and find the best care management. To maintain the code powering the platforms, Horst said his team requires that code be approved by two people before it’s committed. His team also utilizes static analysis with specific linting rules to keep code style uniform.
But it’s not just the line-by-line details of the reviews that lead to the success of his team’s code, Horst said; it’s the effectiveness of human-to-human interactions.
Ben Horst
SENIOR ENGINEERING MANAGER
What best practices does your team follow when doing code reviews?
Our first best practice is that two of your coworkers must approve your code before committing to our test environment. This check helps keep us honest, allows the test environment to run smoothly and keeps us in touch with what our teammates are doing.
Second, our mantra is, “Am I willing to support this code?” We have an on-call rotation that includes everybody on the team.
When we go on call, it’s our job to keep the service up and functional. If the code we approved breaks, will we be happy because we already know how it works? Or upset? Code reviewers should push for code they’re comfortable with and proud of.
"Think about how you’d want to hear the feedback you’re giving.”
When there’s a difference in opinions about how to write certain code, how does your team handle it?
We try to keep opinions out of it as much as possible.
Code style, for example, should not be a matter of opinion. We have static analysis including quite a few linting rules. This prevents some errors, but the main point is to keep code consistent. Readability is often a function of consistency, so we strive to codify our consistency in our static analysis tools. If people disagree on whitespace or ternary formatting or where brackets belong, we hash that argument out. We commit to a change that the linter enforces automatically and we agree to move on. No argument should repeat itself often.
What advice do you have for fostering a positive code review culture?
As an author, put yourself in the reviewer’s shoes. What do you want to see in a code review description? What context do you need? Make sure to write those details out for reviewers. Review your own differences first. If something isn’t self-explanatory, consider making some comments. Consider whether those comments are worth putting directly into the code (they probably are).
As a reviewer, put yourself in the author’s shoes. Try to understand what constraints they’re under to get this code into the product. Think about how you’d want to hear the feedback you’re giving. Choose the battles that will yield the most value for the product and forget the rest.
When things get dicey and you’re not sure how to respond to a particular review, consider sitting down with the person to have them walk you through it instead of typing out a letter on the review. As a reviewer, walk the coder through constructive suggestions on a whiteboard or wherever is most comfortable.
If we can’t understand each other, we’ll never understand each other’s code. The code is for a computer, but it comes from a human.
