If you're working on a codebase with more than one contributor you likely participate in code review. Also known as the act of signing off on someone else's code before it can be merged into the main branch.
The idea behind code review is incredibly sound. We all benefit from an extra pair of eyes. It helps team members gain context on other pieces of the code base. It ideally keeps the patterns and choices consistent, helping people onboard down the line.
Yet the words "code review" can often strike fear into the minds of those who hear it. Those who have endured bad code reviews often describe the experience as demoralizing or condescending. We don't want to make people feel that way, so let's talk about what it takes to be a good reviewer.
What not to do
Let's start by explaining what code review is not.
- Code review is not being a human linter
If a given piece of syntax should never show up in your codebase, add an automatic linter rule. It's a waste of time to make this a manual process and it doesn't provide a ton of value. If it's not worth it to add the rule then it's probably not worth it to point out in the code review either. If you do, you're being pedantic.
- Code review is not about proving how smart you are
The goal of code review is entirely about the person who wrote the code growing their skills, and your codebase getting more robust. None of that is about your ego or showing off how much you know.
So how do I do it right?
Now that we have that out of the way, we can talk about what you should do.
This is a two way conversation
Yes, there are power dynamics at play in code review. The reviewer may have a more senior title or have more experience in the codebase at hand. The original author may take that into account when weighing the recommendations, but as a reviewer you need to recognize that what you say isn't gospel. You may be missing context, or misunderstand what the original author is trying to do.
Given that, it's better to ask questions when phrasing recommendations. Instead of "You should do it Y way", try "Can you talk about your reasons for choosing X? In most cases I'd use Y, is there a reason not to here?". Typically, this has the same outcome but it feels more collaborative and leaves open an opportunity for all participants to learn.
This isn't just about what is wrong
Code reviews are asynchronous, but they don't have to read that way. The goal is to provide feedback and not all feedback is negative. It's just as valid to say "Cool! I didn't know you could do that". And you should. It will balance out the overall tenor of the review and leave the author with a sense of where they can improve as well as where they should double down.
Not everything is blocking
This may seem contrary to my comment above about being a human linter. However, not all things can be linted. Naming is a great example of this. Sometimes, you may have a recommendation that should not prevent the code from moving forward, but you want to note anyway.
Marking these things as "NB" or non-blocking can be a great way to provide a quick note that an author can consider but doesn't have to listen to. This is especially helpful if you have an idea for an improvement but don't feel strongly about it. Or you have a question but don't want the back and forth to hold up getting the feature in.
What am I looking for?
Now that we've talked about how to give feedback, let's talk about what to give feedback on. If code review isn't for pointing out syntax improvements, what is it for?
- Integration points
Is there a spot that may cause friction with another system? Do you need to loop someone else into the review? Does there need to be a synchronous conversation? Point that out.
- Error boundaries
Is there an edge case that isn't considered or handled? Talk about it. Make sure to address it before the code gets merged.
- Unecessary bloat
Does the code propose including a new library or system of some kind? Does it need to? It's worth discussing.
- Pattern deviation
Are you deviating from the way you've handled this type of functionality or data elsewhere in the codebase? Why? Let's talk about it.
- Scalablility
Is the way the is code written going to cause problems down the road? Bring it up early even if you choose to go for the short term solution.
This is not an exhaustive list, but it should give you an idea of areas of concern that reviews can help aleviate.
Level up your team
The goal of a code review is to help level up the team and improve the long term health of the codebase. So focus on learning opportunities for your colleagues and yourself. And keep practicing--code review is a skill.
Top comments (5)
Love the idea of tagging comments with "NB" if they are non-blocking. I'm definitely going to be implementing this into my future reviews!
Definitely more about design and architectural choices than linting or formatting.
For example a couple of times I've seen people using the wrong data structure which made the code complex and maybe even unsolvable (and definitely full of errors). Other times the code wasn't following the approach that was discussed, so getting into the reasons for that is also useful.
It's also useful if developing code is an ongoing conversation.
You discuss what design to choose, what approaches to take, then people go off and do it, then the code review is just a continuation of the discussion and a test to see in what ways those approaches succeeded or failed.
If reviewer decides to keep his ego aside, rest all shall very easily be followed.
This tips are great to move reviews from just verifying checklists to have productive discussions.
Very good points. In addition, a code review should not become personal. Wether for the writer (you are not your code) nor the reviewer (be human). This is highly important because you have the same goal: good working code.