Our team within the revenue organization recently discussed what professional skills, sometimes referred to as “soft skills,” were important to being a successful engineer. Not surprisingly, the ability to communicate and empathize with peers was a common thread. Since we write code, the way we communicate often tends to be through formalized pull requests.
Pull requests are a chance for coworkers to review our work before it goes live to production. That means we’ll receive suggestions to make the code better or important feedback that requires attention. Most of our teammates are remote, so we can’t leverage body language or happy, in-person smiles like office-dwelling teams. This means close attention to the language and tone of our written feedback is crucial to team happiness and morale.
We established some internal guidelines that we felt were important for code reviews, and they happened to line up nicely with the core values of Vox Media. In particular:
“...seek new perspectives”
“We grow stronger through shared knowledge”
“Listen with an open mind. Practice empathy. Avoid arrogance.”
“Ask for help when you need it. Provide help when asked.”
“Give recognition to coworkers. Take a moment to enjoy wins.”
Here are a few of the guidelines we established for giving good feedback in code reviews.
Disclaimer: Is the Person Looking for Feedback Yet?
Sometimes our pull requests are labeled as “Work in Progress.” This typically means the author wanted to have an established place to view their changes but wasn’t ready to accept feedback yet.
Other times, this could mean the author is looking for feedback about the general direction or approach, but is not yet looking to ship production code.
In either situation, we recommend being aware of the context in which the author is looking for feedback so you don’t come barging into their notifications.
Seek to Understand
A good baseline for giving feedback on our team is the phrase “Seek to Understand.”
Consider this: instead of jumping to conclusions about the way a piece of code is written or immediately offering an alternative, try asking a question about the way the author decided to solve the problem.
Be sure to ask a genuine question! If it feels like you’re asking a question to which you already know the answer, that’s OK — it’s not uncommon to hear back from the author and learn something new from their approach that you wouldn’t have considered otherwise.
This can also take some weight off of the author’s shoulders: instead of having to prove themselves to a teammate — perhaps a coworker who is more senior or experienced than them — they can answer the question honestly without the fear of stepping on toes or hurting feelings.
We collaborate well learn by giving and receiving feedback, and we grow stronger through shared knowledge.
We aim to be inclusive and seek new perspectives to our engineering knowledge. Not only do we want to be inclusive to thoughts and opinions from engineers representing diverse backgrounds, but we also want to promote an environment where a junior-level engineer with little experience in the codebase feels comfortable giving feedback to a senior-level engineer who helped write the original application a decade ago.
We can leverage this form of question-asking feedback in another way. Consider the following internal monologue of a code reviewer:
I want to tell Code Author about Cool Library™ that does the exact thing they are doing. It will save them time!
For example, you could write the following as a question instead of a direct recommendation:
Again, this removes the burden of proof from the author and instead opens a door to discussion. Maybe they’ve already considered using Cool Library™ but ruled it out because of some additional concern. Or — even better — they’re going to take the suggestion and use it, and now everyone is happy!
One little thing from the previous example highlights something our product team also does well: the use of “we.” Our team of engineers as a whole is responsible for maintaining the products we build. Even though some teammates have areas of focus in one app or another, at the end of the day we’re in this together.
By phrasing feedback using the first-person plural “we” instead of the second-person “you,” we foster the sense of community we strive to build for our engineers.
Ready for Lunch? Time for a Code Review Sandwich
Sometimes it’s easier to deliver criticism, especially to your peers, when it’s wrapped in with genuine positive vibes. This is sometimes called a “compliment sandwich.”
If you’re not familiar with the term, picture this:
Simple, right? OK, here’s a “Compliment Sandwich”:
Different, but still tasty. This manifests itself in code reviews, too! I present to you: the “Code Review Sandwich”:
Granted, there are only so many sandwiches a person can eat before feeling full. The same goes with code reviews. As we develop a rapport with our teammates, we can often skip one of the slices of bread and be more direct with our suggestion or critique. This is definitely true for quick bug fixes or for high-priority requests.
This really boils down to respect, which means listening with an open mind, practicing empathy and avoiding arrogance. And it’s a useful technique for engineers at all levels when reviewing someone else’s code — especially for the first time.
Sometimes it’s difficult to convey meaning through text alone with a team of so many remote engineers. Comments can come off as terse or rude, even when the reviewer tried to convey them in a positive and helpful light.
Sprinkling in emojis can help elevate the voice and tone for many of our team members. It helps the author read the comment in the reviewer’s voice, as if they were delivering the feedback in person.
Your mileage may vary, but here’s an example of mild emoji usage:
The two examples above have literally the same text, but the use of emoji amplifies the message in a couple ways. It makes it more celebratory (clapping) and it also caps it off with a sense of excitement and anticipation for the code to be shipped to production (rocket). With just two emojis, we’ve potentially improved author’s morale and fostered a rapport with the reviewer.
Again, this may not but everyone’s cup of tea — but we like that emojis are a resource to improve the code review atmosphere.
We can sprinkle in high fives and emojis all day, but it’s still important to deliver important feedback so the best software ships to production.
If something needs to be addressed, address it!
Many engineers struggle with Imposter Syndrome, so approaching a code review might be overwhelming or daunting. One thing that has helped our team is the following:
If you took time to read through the code and description of the pull request, you should take action by approving it or by leaving additional feedback.
This encourages the reviewer to overcome their fears of feeling unqualified to give feedback on a teammate’s code. Plus, as a reviewer, they will get pinged on any follow-up comments by other reviewers and see what things they may have missed.
This is one of the main reasons our team requires at least two approvals on each pull request. Approval from more teammates means better knowledge-sharing and codebase ownership. It’s easier to demand quality and hold one another accountable when more eyes are on the solution to a problem.
This approach makes leaving code feedback less daunting, even if a reviewer is struggling with Imposter Syndrome.
As we grow as engineers, it’s important to make sure the feedback we give is clear and actionable. Feedback that is impossible to address is often worse than no feedback at all!
If feasible, we recommend offering up a preferred solution or opinion rather than a bunch of different options. Competing opinions living in a single piece of feedback tend to be unhelpful. They require more brain power for the code author to parse and decide for themselves which is the best option.
Consider these two options:
If we focus on describing our preferred method to solve a particular issue, it’s much easier for the code author to simply accept that method or to explain why that method wouldn’t work in their case.
Having a strong opinion, though loosely-held, helps speed up the decision-making process within our organization. Sometimes, if we don’t have a strong opinion, letting the author know how much we care about the given suggestion — on a scale from one to ten, for example — can be helpful.
And if we still don’t agree at the end of the day, we try to disagree and commit so the overall product can maintain forward progress.
Part of demanding quality is asking for help when we needed and providing help when asked.
There’s no reason you can’t offer to help as the last slice of bread in your “code feedback sandwich.”
We can also help the code author by offering up links to documentation or to specific examples in the codebase or other libraries.
We advise our teammates to watch out for certain situations that tend to arise in a code review environment.
The first situation is something we call Pile-on. This is when you’re reviewing a pull request and notice a bunch of related issues with the code. While it’s tempting to comment on each small issue, we suggest pointing out only one specific instance. This allows us to give the author a possible solution and to let the author know that the issue is repeated elsewhere in the code.
It’s also possible that one or more engineers on the team have already pointed out numerous issues on a single pull request. While the additional feedback would be technically helpful, it’s likely going to just “add to the pile” and make the code author feel discouraged or overwhelmed.
It’s perfectly OK to refrain from giving feedback if you see a bunch of comments on a pull request and decide that the author will be rewriting a good portion of the code regardless.
We also recommend messaging the author directly to ask if they need help with anything. It might provide a much-needed boost to their morale and help them come up with an improved version of their code without leaving a single comment.
Another type of code feedback that comes up frequently is the Nitpick. These are typically comments about a missing semicolon or a weird formatting issue. Nitpicks are fine when applied sparingly to a review. But if nitpicks are a frequent part of the code review process, it’s important to extract these concerns to a linter like ESLint or Rubocop.
There are a couple benefits to using a linter. When nitpicks are caught by continuous integration instead of by humans, it frees up our engineers to focus on reviewing the code’s actual purpose rather than how it is formatted. Additionally, linter configurations are generally version-controlled, meaning our team can send linting rules through the code review process before they are enforced.
Make Someone’s Day
Compliments are free to give. Really — they cost $0.00. So why not give them out more often?
By leaving a sincere compliment on someone’s pull request or mentioning something new you learned from their code, you could be making that person’s day — or week. Plus, that compliment might improve confidence for a struggling engineer or help welcome a new member to the team.
Celebrate by giving recognition to coworkers and taking a moment to enjoy wins. Code reviews can be an avenue for this type of celebration.
While these guidelines work for our revenue product organization, they aren’t one-size-fits-all for every company — even other engineering teams within the same company. No single person currently checks all of these boxes, but we strive to give better feedback every day.
If we place empathy, listening and understanding over the value semicolons and lines of code, we believe our teams will be stronger and happier.
Does your engineering team share a set of guidelines for giving feedback on code reviews? Let me know at @jplhomer on Twitter.
Special thanks to Becca Barton and Winston Hearn for the thoughtful edits, to Josh Laincz for the sandwich illustrations, and to the Revenue Product engineers who were part of the original discussion about professional engineering skills and who reviewed this post.