Effective Code Review

If you work in a team then there's a good chance code review is going to be your single most consistent, real-world practice to learn and grow as a developer.

As an added bonus for a remote employee / distributed teams, code review is probably our most routine form of collaboration.

So how can we get the most out of it?

I've put a lot of effort into trying to support my team with attentive and thoughtful reviews, and here's some thoughts on how to provide effective code reviews, and what to look for in the code you're reviewing.

1. Dedicate ample time to the task

Someone spent hours / days / weeks thinking about it and writing it. We should dedicate our attention to understanding it. There is nothing LESS reassuring in my opinion than getting the ol’ “LGTM 👍”.

2. Ask questions

What you know is valuable, but also what you don’t know is valuable!

Just for example: If we're changing z-index from 2 to 10, why?

If you don’t know what the element sits above or below, you can ask. Or if the return value of a function was changed form a string to an array, you can just ask.

If it's hard to find the answer, that might encourage the author to alter a function name, or return value, or add a JSDoc to make it clearer to other devs who may not have the full context.

Anything you review, you might have to work on in the future, so getting an introduction from the author can be extremely valuable. On the flip side of that, you might have worked on the feature in the past and have helpful information to share with the author working on it now.

3. Communicate your level of concern with your comment

I will often flag a comment as a 'nitpick' or 'style-guide' for things where I'm being fussy or a stickler for detail.

For notes when I think there is a 'better' code choice, I will try to explain my reasoning, or pose it as a question:

"What would you think if we did this as XYZ... LMK if i'm missing something here!"

"Optimization here: We're calling .flat().map(...), can we combine as a single loop .flatmap(...)"

Just because I'm adding a note doesn't mean it has to be accepted. Sometimes a partial fix has to be done for the sake of a marketing deadline or release cadence.

4. Work IN the code

If you’re not sure about a suggestion, take it to your IDE. If I think an opinion that isn’t fully clear, I will check out the MR branch and stub out the changes to see if an idea is worth pursuing. Sometimes it's not, but now I understand better why the author chose what they did.

5. Have conversations

Code review can and should be a dialog where the author and reviewers work together to transfer knowledge on a feature or bug.

I strongly encourage the use of Slack huddles for 1:1 discussion if there’s a code block, line, or component you want to understand better, or have thoughts that could be better surfaced in conversations. I've heard it called the "quick chat" method before, just ping the author and ask if they have some time for a quick chat to help you understand the MR.

If someone gets upset that they have to explain something once, thats on them.

6. Feel free to leave compliments too!

Sometimes there’s legacy code or cruft that is finally being dealt with - I will occasionally leave comments just to express appreciation!

---

General Guidelines

The basic principles I try to keep front-of-mind in code review:

1. If you’re approving something, you should come away understanding exactly what it does, and why.

2. Are we following coding standards / style guides:

   • Are style-guide spacing / formatting and preferences being followed?
   • Is code organized in a logical way?
   • Have we grouped like with like?
   • Are functions and variables named in ways that express what they do / what value they store?
     • You’ve probably heard some version of the expression: “Naming things is hard” - and the second half of that phrase is “…but naming them badly makes things harder.”
   • Are complex functions broken into smaller easily understood pieces?

3. Ask questions to the author, but also to yourself:

   • Is this code expressive: Is it easy to make sense of what it's doing just by reading it?
   • Does the function name describe what it returns? Or how it handles arguments?
   • Do the variable names align with the values they contain?
   • Is this code choosing the best methods for the job?
   • For example: don’t choose Array.map when you only want to find one value in the iterable.
   • Is this code performant?
     • When in doubt, and a lot of people will fight me on this, benchmark the options and let performance decide. There are various tools for this, I find JSBench.me to be easy to use. I understand that optimizing JS is often fool's gold in the overall scope of performance, but I never found that to be a compelling argument NOT to optimize.
   • On a similar note, Is the code doing work it doesn’t have to?
     • For ex: if there’s an early-return or null-return condition, are we performing computations we might not need in that case? How LIKELY is the early return case?
   • Does this code live in the right place / Does it duplicate logic we have elsewhere?

4. Code review should have limits

   • You do NOT have to understand every single thing that provides the input, or consumes the output.
   • I might look at the unchanged lines to understand the MR, but they're out of scope for review. If I make a comment on an unchanged line, it's usually only to ask for a TODO, or FIXME note or to flag something we should split to a separate ticket.

---

Don't ignore the Self-Review

When you're submitting a merge request, take the time to review it before assigning others.

This might be your first chance to have a fresh perspective on your changes, and you might catch something obvious you didn't spot in your IDE.

Taking a dedicated approach to code review will help develop a clearer sense of “code quality”, and how to apply those standards when writing your own code.