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.
At Dailypay, for example, engineers start to review other engineers’ code their first week. Vice President of Engineering Paul Rodgers said it’s comforting to know that every member of the team experiences code reviews and that feedback is welcome from junior- to senior-level employees.
We talked to five 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.
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.
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.
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 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 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.
