The art of polite code reviews
Taking the time to write meaningful comments while providing feedback to your coworkers can go a long way. Yves shares some of the principles he follows to articulate positive criticism during code reviews.
It can be hard to be on the receiving end of feedback.
If you're a developer, you probably submit and review pull requests every day or so. How many times have you felt a little hurt or frustrated by the comments of a fellow developer on a piece of code you wrote? Maybe a mistake you made gave you the wrong impression and triggered many doubts about your abilities?
I've noticed that it can be a common occurrence in development teams to deliver criticism in a heavy-handed way throughout the process of code review. It can be challenging to provide constructive feedback when there is an urgent deadline to meet or if you're preoccupied with your own work. Sometimes, we clumsily engage in a hard conversation related to severe issues in the code at stake. Even worse, it can be tempting to adopt an approval process that does not really go beyond giving a thumbs up.
Nobody's code is perfect. Having another pair of eyes take a closer look at the feature you just developed is crucial to delivering quality code, no matter how experienced you are. It’s unfortunate that we can not trust ourselves to always deliver faultless code, even if we are highly reliable. There will always be mistakes that slip the attention of the person who committed the code. However, these imperfections should not be an occasion to open fire on the author of the perceived defect.
How do you provide this feedback in a way that stays professional, on point, and doesn't hurt anybody's feelings?
Backed by several years of experience in this field, I’ve noticed that the following approaches help tremendously.
The feedback sandwich
There is another line of work in which giving bad news is an important part of the job: medicine. In hospitals and clinics, doctors are often trained to use what is called the feedback sandwich. The premise of this method is fairly simple and also very efficient: First, say something positive to the employee. Then, deliver the bad news. Finally, say another positive thing to the employee. In other words, the goal is to soften the blow and, yet, deliver the information that is crucial for the employee to grow.
The aforementioned method is a good candidate to provide overall feedback on a pull request. Something as simple as "Nicely done! I noticed a couple of issues. Keep up the good work!" is a lot better to handle for the reviewee than a lacking "I commented on some issues." Not writing a review summary seems somewhat even worse since it does not provide a general impression of the code changes. The human mind often creates its own negative scenario due to our obsessions to understand things. We try to fill the gaps and we imagine the worst. Stating a few reassuring words can go a long way in giving the right impression.
It's not personal
There’s no I in team. Avoid using terms such as "I think," "right" versus "wrong," and other phrases that put people on the spot. Instead, point to what you noticed precisely and make it impersonal to avoid confrontation or hurting someone's feelings. Whenever you’re tempted to say “I think” or “I believe,” consider using one of these alternatives:
- It seems that (this does not work as intended)...
- It looks like (this is the wrong type for this variable)...
- I noticed that (this line causes an error)...
- I suspect that (this could be implemented differently)...
- There might be (a more elegant solution to solve this problem)...
- It could be beneficial (to write a function to avoid repeating the same code)...
It’s not git blame. Avoid blaming the author with phrases like "you should" when you see bad code. Instead, provide the reason why changing the code would be beneficial, preferably drawn from your own experience. For example, if you notice that the author of the pull request is not using the same case to name their variables, you can say something like this: “I've noticed that using the same case throughout a codebase helps with the readability of my code. How about using camelCase for function and variable names?”
Actionable comments
Provide an alternative. Something that can be frustrating about criticism is when the person who finds the fault focuses on the problem. Instead, I would suggest looking for solutions right away in order to put the developer on what seems like a better track. You don’t like the name of a new function? Provide one or two options that float your boat in your comments. A piece of code seems to be located in the wrong file? Don’t bark at the author. Let them know what would be a more convenient home for this new class. Even better, GitHub and GitLab let reviewers add code suggestions that can be committed straight from the pull request.
Documented feedback
Google It™. Sometimes, it can be hard to admit that what we did is not optimal. It can be tempting to respond with defiance and ask for proof. Before the conversation goes south, you might find it beneficial to take the time to check your assumptions about how, for example, that built-in method works. If your suspicion is correct, I would advise you not to take a lecturing tone about it. Instead, you can simply state that the documentation you consulted seems to suggest that the method in question works differently than what the added code implies. Make sure that you provide a link to a reliable source like the Mozilla Developer Network documentation or a trustworthy Stack Overflow question. Doing so can help provide more context about the issue and can be a great learning experience for both the reviewee and the reviewer. And, don’t hesitate to express when you discovered something new you didn’t expect!
Take it outside
Instead of diving into a long conversation, offer to chat on Slack or hop on a video call. It can get a little frustrating when an exchange keeps going on and on without a resolution. It can also create misunderstandings or give the impression that something is more important than it seems. Taking it out outside of the code review can help everybody get on the same page. When you notice a problem that could snowball, don’t hesitate to write something like: “Hey! I have some thoughts about how we could refactor this function. I sent you a message on Slack. Let's put a video call on our calendars.” This can be a great way to de-escalate possible conflicts that might emerge due to different approaches to programming.
Give the benefit of the doubt
Avoid the clash of titans. You might have strong opinions about how to write code. The author of the pull request might as well. It does not seem like anybody would benefit from seeing two developers headbutt in a pull request. In doubt, ask the author why they wrote the code this way and look carefully at their answer. Maybe they did not think about the shortcoming of their approach. Hopefully, they will catch themselves before making the same mistake next time. Or, they might have some interesting suggestions you did not think about on how to resolve a problem you didn’t see.
Be supportive
It might be a little discouraging when there are a lot of issues with a pull request or when the same mistakes are repeated. No matter how horrible you think the code looks, show a little empathy. Don’t make it seem like it’s the end of the world for the author. If you’re dealing with an inexperienced developer, highlight the progress they’ve made.
Establish guidelines
Finally, the last piece of advice I would like to give is to set style and code review guidelines. Involve the entire team in this process. Make sure that you establish rules together instead of dictating them. Think hard about the reason behind each rule you set to determine whether it is justified or not. You might want to use an existing style guide to save yourself some work such as the Airbnb JavaScript Style Guide. Set up linters to avoid spending too much time nitpicking.
Specify the review process as well. Answer the questions that a new employee might have about it. Who reviews the code? How many reviews are necessary per pull request? When are these code reviews performed?
As a result, you will have a guide you can refer to as a supporting document during code reviews and, hopefully, a team that fully adheres to it.