Code Review Matters: Best Practices From 4 Boston Engineers

Written by Janey Zitomer
Published on Feb. 07, 2020
Brand Studio Logo

Code reviews require collaboration rooted in problem solving. 

Reviewing work for quality assurance’s sake means engineers must open up their hard work to constructive criticism along the way. With that in mind, it’s important that teams set themselves up for success by detailing best practices to keep developers on the same page and confident in the final outcome. 

Panorama Education Software Engineer Jemma Issroff recommends using “I” statements instead of “you” language when explaining why a certain method either hits the mark or falls short of expectations. Tom Donahue, human-robot interaction engineer at Piaggio Fast Forward, asks his team to consider the context under which the code was developed before making any assumptions. 

“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,” said Donahue.  

Four Boston-based engineers shared additional advice for making the most of each code review and addressing differences of opinion, below.

Code Review Best Practices

  • Make sure the pipeline of reviews is standardized and clear
  • Be transparent about where a feature is in the code review process
  • Institute formal sit-down reviews, where developers walk through the review process with their peers
  • Use "I" or "we" language instead of "you" to demonstrate that the author and reviewer share the same goals
  • Have two or more engineers review the same code to ensure what you're putting out is clean and functional
Piaggio Fast Forward
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 Automate

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

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
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.

 

Responses have been edited for length and clarity. Images via listed companies.