To start, it is true that each code review is a unique task, and that the code, ticket, and a variety of other factors change what the reviewer should be looking for or assessing on each review. That said there are a number of commonalities that we can address using a few best practices that we can all go into.
Set Aside Time For Reviews:
Instead of avoiding answering requests for review, take ownership, which means to address the request as soon as is reasonably possible but doesn’t necessarily mean to drop everything to act on it.
Example
❓ [Developer]: Hey. Can you take a look at this PR?
https://github.com/big-corp/hydra-cms/pull/ft-dynamic-navbar. Thanks.✅ [Reviewer]: Yes I can take a look. I’m in the middle of something just now. I usually set aside 30-60 minutes each morning for code reviews so that I can knock them out all at once. It also helps that everyone is usually online around this time so they’re available if I have questions. I’ll make sure to get to this one when I come online tomorrow. If that’s not fast enough I’d suggest reaching out to X, Y, or Z who are also approvers for this repository and they may be able to help you faster.
This let’s them know what to expect and puts the imperative back on them to spend their time productively rather than waiting around for a review that may come in the next few minutes or not for several hours.
Understand the Context:
Before diving into the code, take a moment to understand the context. Familiarize yourself with the associated ticket or task details, the purpose of the changes, and how these changes fit into the overall project.
Example:
❓ [Developer]: Hey. Can you take a look at this PR?
https://github.com/big-corp/hydra-cms/pull/ft-dynamic-navbar. Thanks.
Now at this point as a reviewer you need to ask yourself:
Do I know anything about
hydra-cms?If the answer is no, you should decline, and instead direct them to a subject matter expert on the space if they are available.
If there are no other possible reviewers, then it’s clear the company or team expects you to have knowledge of this space, and you should take the time out to get familiar with the space before reviewing the code. You might only have 15-20 minutes to do this and it might be best to have the submitting developer give you an overview of the space to onboard you quickly.
Am I familiar with the
dynamic-navbarfeature?- If you aren’t (but you know the space) you can review the ticket, ask the submitting developer to give you their understanding of it (but do not rely solely on their interpretation, which might be inaccurate), or discuss with the rest of the team.
If you’ve looked at the change request prior to doing the above, you are doing it wrong, this is an indisputable fact. You are attempting to read or review something you do not understand. Which is the same as trying to critique of a scientific research paper, written in a language you can read, speak, and write, but is otherwise on a topic you have no prior knowledge of.
You might understand the words and sentences, but without understanding of ("Reverse Synergizing Backwards Overflow", "Quantum Entanglement of Spaghetti Strands", "The Aerodynamics of a Toast Landing Butter-side Down", or "The Thermodynamics of a Cup of Tea in a Snowstorm"), your critique would likely miss the mark.
Once you’re familiar with the context you can then…
Start Reviewing the Code
Start the actual code review.
🎬 Scenario #1:
You open the request and find that someone else, someone senior to you, has already reviewed and approved the changes.
Don't let this deter you. Regardless of who has reviewed the code before you, it's essential to perform your own independent review. You may catch something that others have missed or bring a fresh perspective to the table.
Furthermore, it is important to hold one another accountable. Code review, sadly, is often treated as a chore or an afterthought and does not always receive its due attention. Did the other reviewer miss obvious deficiencies in the code? Did the request get approved by this other reviewer 2 minutes after it opened when the change request has over 10 files and dozens of lines of code altered? These could be signs that the previous review may not have been as thorough as necessary. In such cases, it's even more important to conduct a detailed review and provide comprehensive feedback.
🎬 Scenario #2:
You open the request and find that someone else has already left feedback citing the need for corrections or further changes.
Instead of allowing this to deter you, instead start by reviewing the existing feedback. Do you agree with the feedback? Is the feedback in scope? Is the feedback constructive and seeking to prevent an issue or is it needlessly delaying the release of otherwise functional code? If any of the above rings true you should leave feedback (on the feedback) citing why it is or is not valid. This can help smack down frivolous requests for changes and encourage a more focused review process. After reviewing the feedback, proceed to review the code independently. Your perspective might identify issues that were previously overlooked or missed.
🎬 Scenario #3:
You’re examining the 10 changed files and, on the third file, you find an issue.
Don't Stop At The First Issue:
Instead of stopping at the first issue, complete a full review to identify all potential problems. This will provide you with expanded context and can lead to more comprehensive feedback. Once the full review is completed, you can then provide your feedback, highlighting all the issues identified. This approach reduces the back-and-forth between the developer and reviewers, saving time and reducing frustration.
If you prefer to leave comments as you go be sure to resolve or remove comments that are rendered invalid.
💡 Example:
You find that the code is being written in a strange or otherwise nonsensical way. The developer is using snake casing and unnecessarily long names for his variables. You decide to leave a comment on this requesting them to use casing more in line with the typical casing and naming conventions used by the team.
If you were to stop here it would cause the developer to have to either, fix the issue and come back to you (waiting 12-72 hours typically for another review), or to wait to discuss it with you. Meanwhile other reviewers might see that there is already feedback and avoid reviewing the changes altogether until they are resolved (even though, for reasons we’ve gone into, this is not the correct response. It is still very common).
Instead (because you are an everyday hero) you decide to continue reviewing the rest of the code and find that the casing actually follows a pre-established convention which, while not common for your team, is common in this project that your team had inherited (and therefore has practices not observed by your team). Changing all of these would be time consuming and is likely not in scope for the current ticket.
At this point you should return to the previous comment and follow up, marking it resolved, citing why you’re resolving it (as others might arrive at the same initial conclusion you did but not continue the full examination) and saving the rest of the team the time and trouble of assessing this particular problem themselves.
🎬 Scenario #4:
You arrive second on the scene to a change request and find a large amount of feedback already in place. Comments are made across several files. Much of which is out of scope or is demanding changes which are otherwise not productive.
You should make it a point to leave feedback here, possibly even get a live group discussion going to address this, each team has its own style and so these comments might be appropriate depending upon the team but just because feedback is valid does not mean that it is productive.
💡 Example:
A previous reviewer has left a comment on a method that is unrelated to the change request. They're suggesting a complete rewrite of the method, stating that it could be optimized. While this might be true, your own assessment finds that this code performs at an acceptable level, and that the time involved in addressing this feedback would yield little business value.
Instead, this suggestion should be documented separately for future consideration. As a fellow reviewer, you should address this by commenting on the feedback, explaining that it's out of scope and suggesting that it be moved to a separate ticket or task. This will help keep the review focused and prevent unnecessary delays.