Developers should look forward to code reviews, not dread them

Code reviews are a great place to exchange knowledge. Developers have the opportunity to learn from their peers, to showcase a piece of code they’re particularly proud of, to share what they discover to be a better approach to a known problem, and to leverage the expertise of teammates. Ideally, a working piece of code becomes better through the ensuing dialogue between the author and a reviewer. That’s the goal, that’s the purpose of code reviews, to foster learning, encourage feedback, positive reinforcement and add experiences to a whole that is greater than the sum of its parts.

I enjoy code reviews — as an author and as a reviewer. Once I’m done with a piece of code and have that feeling of a job well done that goes with completing a task, I actively seek my peers and ask them to review my code, because I hope they’ll point out something I missed and I’ll learn something, or they’ll see something they don’t know and try to learn from it, or they’ll just say “hey that’s cool, I like the way you’ve done that”. Sometimes, they save me from an embarrassing bug. Whatever it may be, I’m looking forward to what they have to say.

So why are they too often a source of fear and existential dread? Why are there plenty of code review horror stories? Even Linus Torvalds has become famous for his tirades and rants, and had to take a break after recognizing that the way he treated some other developers was not right (coincidently, I’ve socialized a few times with the developer on the receiving end of this brutal review, and when I reached out to offer moral support, he defended Linus and justified his behavior, much to my surprise, which leads me to assume that this behavior is deemed acceptable in certain ways). If some developers have come to dread code reviews, it’s because of… other developers. It’s very odd that the same people who dislike code reviews are the ones responsible for them, this would seem like an easy problem to fix, so what’s wrong?

I started asking myself why code reviews are something I’ve come to look forward to when many people dread them, and I’ve tried to come up with ideas on how to make them a positive experience.

Don’t be a jerk

My teammates are not jerks, and it’s not only about who they are as individuals, it’s also about who we, as a team, decide to be. My manager wouldn’t tolerate me being a jerk on pull requests, and I wouldn’t tolerate that from my team members either. So rule #1 is the golden rule: treat others the same way that you would like to be treated. That one’s not hard (and it’s applicable everywhere). It starts from the top, and it’s taught by example. Sure, I’ve had bursts of anger where I see the same mistake made by the same developer over and over. A negative reaction would be to lash out, “how many times do I have to to tell you not to do this”. A more positive and constructive reaction is to simply ask: “hey, I see you do this over and over, what is it about this pattern that’s so appealing?” without emitting judgement either way, and instead of conflict, there is dialogue.

Developers need to submit code free of preventable mistakes

When reviewing code, there’s a natural tendency to want to leave a comment, any comment. Even if there’s nothing wrong with the code, there’s probably a curly brace that’s not aligned, or a typo somewhere. After all, if someone is tagged as a reviewer and doesn’t leave a comment and just approves, they’re not doing their job, right? Wrong! A junior developer recently shared their experience interning at a large social media company. They fell in a slump because their PRs kept getting picked apart with comments that add little value: “remove extra space here” or “rename this variable”. Every comment and associated correction would send an email to a bunch of other people, adding more scrutiny and pressure, until the developer came to feel that no code contribution would ever be good enough and stopped opening pull requests. That should have raised all kinds of alarms to the lead developer/team manager, but instead the developer was reprimanded for “falling behind” and not opening enough PRs. It’s easy to see how this became a toxic environment.

To avoid unhelpful nitpicking, the team should have:

• A properly configured linter, supplemented with custom rules if necessary
• An agreed upon style guide. Defer to an established standard like Google, AirBnb, Sun/Oracle to avoid battles of opinions
• A common formatting configuration for IDEs (like editorconfig)
• An automated code formatter using a git hook (like husky/lint-stage/prettier)
• Documentation for known anti patterns
• Documentation detailing what to look for in a code review. (e.g: scope, correctness, design and style, in that order. This is from CS50)
• Unit tests with adequate coverage

Document this under a team agreement that is available to all team members. If one of those choices comes under attack, it becomes a whole lot easier to say “this is detailed in the team agreement” rather than “I like my way of doing things better”.

Small common mistakes are easy to pick up and to fret over. They’re common because they’re easy to make. Many tools exist to avoid these common missteps and can significantly bring up code quality. By using tools that prevent them from finding their way in the code, reviews can focus on substance over style.

Reviewers should be helpful and meaningful

If reviewers are not looking at code that’s poorly formatted or breaking standards, they can now focus on more substantive quality criteria, like scope and correctness. Hastily written comments can lead to confusion which leads to frustration. It is the role of reviewers to be easy to understand when leaving comments.

• In an international context, use simple global English, free of idioms and cultural references.
• When possible, reference official documentation and established best practices.
• Emojis are useful to convey context in the absence of body language.
• Comments should be meaningful. I once left a comment that was just one line of code in a pull request. This degenerated in a full blown debate on whether to use map or reduce. After about half a dozen comments from my team mates, I ended the conversation by explaining that the point of my comment was to show the developer that their for loop was not necessary and could be replaced by a call to an Array method. I obviously failed to make my point. My comment should have been “You can shorten this code by using reduce” with a link to the MDN documentation.
• Comments are not meant to be conversations. Use your phone/slack/corporate instant messenger/email. If you’re in the same office, walk over to that person’s desk. Value human interactions.
• Remember the golden rule, don’t be a jerk, don’t be condescending, don’t be pedantic and don’t be patronizing.
• Learn how to give and receive feedback: assume positive intent, be clear, be helpful, think about a time where you may have given/received similar feedback, etc…
• Comments can be used to give praise!

Finally, lead/senior developers need to earn their title and remember that they’re meant to be mentors, and they need to lead by example. When a pull request is way off, leaders need to ask themselves if they did everything that they were supposed to do: offer proper guidance, make sure the requirement was understood, the scope of the work clearly defined, and that the task is within the skill area of the developer, regardless of level. If the author of a particularly wrong pull request is a junior developer, someone needs to sit with that person. Start the conversation with “I was reviewing your code, and there are a few things I didn’t understand”, or “hey, I checked out your branch, and I think we can improve this, let’s look at it together”. Junior developers should be in an environment where they feel safe to ask for that if it’s not happening.

In the end, we developers are responsible for conducting code reviews, and it is up to us to make them what they should be, an enriching conversation.