How to Give a Great Code Review
How to Give a Great Code Review
|Claim: A great code review is thorough, constructive, kind, and prompt.
- Effective code review requires that you understand what the PR is trying to accomplish. If there is a ticket or issue associated with the PR, start by opening it and reading the acceptance criteria. Then read through the PR description. Think about how you might approach the problem if you had written the code yourself. What are the constraints? What are some edge cases you would want to watch out for? If you are unclear about any aspect of the problem the PR is solving, talk to the author to clarify before you start looking at their code.
- If the author has deployed their code to a QA environment, take some time to try out the new functionality. See whether the actual behaviour matches your assumptions. If not, go back and check the acceptance criteria. If you have any questions or you think you’ve found a bug, talk to the author before jumping in.
- Once you are satisfied that you fully understand the problem being solved, have a look at the PR stats. How many files were changed? How many lines were added and removed? Larger PRs will require more time and mental energy to review, but they are also the most important to review thoroughly because they are the most complex and have the most potential for bugs. Make sure you have set aside enough time to give the PR the attention it deserves. If necessary, find a quiet area free of distractions where you can give the code review your full attention.
- Find a tool and workflow that helps you review code effectively and comfortably. Sometimes the web UI isn’t ideal for this: on GitHub, for example, you can’t click through to function definitions, and files are often in a random order. You may find it easier to check out the code locally so you can follow along in your IDE. If you use VS Code, extensions are available for GitHub, BitBucket and GitLab to help you review right inside your editor.
- You may prefer to do your review in more than one pass. Often it’s easier to read through a larger PR first to get a bird’s-eye view, then go back and focus on particular parts. You can also use your first pass over the code to look for small things the author may have missed: stray debugging statements, commented code,
TODOs, or typos. On your next pass, you can focus more on the deeper and more valuable questions: correctness of logic, soundness of tests, clarity of code, and higher level questions of design.
- You have two main goals as a reviewer. The first is to make sure that the code you are reviewing works properly. The second is to ensure that is well-designed and maintainable.
- What does it mean for code to work properly? It needs to fulfill the acceptance criteria. It should be resilient in case of unexpected inputs or errors. It should correctly handle edge cases. It should be secure. It should also provide acceptable performance in real-world situations. A comprehensive test suite helps you verify that the code works correctly. Tests can be a form of documentation that allows a reviewer to see what scenarios the author considered. As a reviewer, you should be looking for gaps in test coverage. Are there any possible inputs that weren’t adequately tested? Does every test assert on all conditions you would expect it to, given its description? If there are snapshot tests, are all the snapshots correct? Did you read every line of every snapshot? (If not, it may be a sign that the snapshots are too large to be effective and that other testing strategies might be more appropriate. That's a valuable thing to mention in a code review!)
- How do you know whether code is maintainable? If you have trouble understanding a piece of code, it’s very likely that the next person to read it will also have trouble understanding it. Imagine that someone who just joined the team had to refactor or extend the code: would they be able to understand it? What if the original author left? If you encounter some code that doesn’t make sense to you during a code review, you may be tempted to skip over it. You may wonder whether it’s even possible to give good feedback on code you don’t understand. If you notice yourself thinking this, resist the urge to move on. That feeling of confusion is a valuable signal that you should work with the author to improve the clarity of the code or add documentation. Think about what would make the code easier to follow. Could the names of any variables or functions be improved? Could any complex functions be split up into smaller named functions? Is there a library that could help to reduce complexity? In some cases, inline comments that explain the “why” behind particular logic can be helpful, though comments should not be treated as a substitute for clear and self-documenting code.
- Try to avoid focusing too much energy on small things that don’t affect correctness such as code style or formatting. A good rule of thumb is that if something could be enforced using a linter, then it probably shouldn’t be brought up in a code review; instead, identify a lint rule that would have prevented the issue, get consensus from the team, and add it to the lint config so all future instances of the issue will be caught automatically.
- If others have reviewed the code before you, read their comments carefully. Try not to leave comments that conflict with comments left by other reviewers. If you disagree with another reviewer, first communicate with them to resolve the conflict outside the context of the PR, then work with them to update the feedback on the PR if necessary.
- Sometimes it’s easier to show than tell. If you have an idea for a refactor or bug fix, consider leaving a sketch of your proposed solution as code (or pseudocode) in a comment.
- Kindness is about having empathy for the author. It’s not about lowering your standards! In fact, kindness is essential to maintaining high standards because without kindness, your teammates may come to dread your code reviews and look for ways to avoid them. A code review should be a cooperative process, not an adversarial one. Both the author and the reviewer have the same goal: to make the code better.
- Be mindful that people may have an emotional connection to the code they write: it’s their baby. Remember that not every comment has to be an action item. Sometimes you may just want to call out some especially elegant code or a creative solution to a tricky problem. Every PR has something worthy of praise in it. Challenge yourself to leave at least one genuine piece of positive feedback on every PR you review.
- Try to avoid overwhelming the author with too many comments. If you have a lot of feedback on a PR, it may be best to review the code in person with the author. An in-person code review is often less intimidating. It is a conversation between author and reviewer that gives both the opportunity to ask questions and verify assumptions in real time.
- Too many rounds of feedback can also be frustrating for the author. If you are not ready to give the author a +1 after a couple rounds of comments, consider offering to pair with them to address any remaining issues.
- You may think you’re doing the author a favour if you approve their code without any comments, but the best gift you can give the author is a code review that helps them identify a bug before it lands in production. Ideally, the feedback you provide helps the author write code that they are proud of and ship to production with more certainty that it will work correctly.
- Review from a place of curiosity, not authority. Rather than telling the author to make a change, ask: have you considered this alternative? Maybe they have, and they know something that you don’t. Remember that they wrote the code and are familiar with the constraints of the problem.
- Slow code reviews kill team productivity. Make sure you know if the author is blocked on your review. If they are blocked, prioritize their PR ahead of writing any new code. Most PRs should be waiting for review for hours, at most, not days. Even if the author is not blocked on your review, you should aim to review their PR while they can still remember all the context from when they wrote their code.
- Code reviews on large PRs can take a long time and you may not always be able to review the code immediately. Let the author know when you expect to get time to review their PR and then hold yourself to it. If you are busy with your own work and don’t expect to have time to review the PR for a while, consider asking another team member to review it instead.