Code Review Guidelines
These guidelines stem from what code review should accomplish. It’s impossible for us to lay out guidelines which will be applicable to every situation so staying mindful of these goals will help you adhere to “the spirit” of code reviews even when you encounter a situation they don’t cover.
Code reviews should:
You should choose reviewers who can confirm that your code is correct, well-architected, and conforms to conventions within a reasonable timeframe.
Communication is key to prevent yourself from getting blocked on code reviews. If you find that your reviewers are bottlenecking the process, work with them to find more appropriate reviewers or determine a timeline that works for all of you.
A primary reviewer is responsible for the overall code review. They are as responsible for the final code as the person who wrote it. You should always explicitly have a primary reviewer listed so that everyone knows who has final responsibility.
Don’t ship code without approval from your primary reviewer unless you are experiencing an emergency and your primary reviewer is unreachable.
Clearly define the responsibilities of each reviewer
To avoid redundancy when multiple people are reviewing a piece of code, clearly define what section(s) each reviewer is responsible for and who will be the primary reviewer.
This allows each person to focus on their area of expertise (in the case of people like DBAs) and keeps discussions manageable.
Give your reviewers context on your change. Ideally, you should speak with them before any non-trivial review or document the changes you’re making inside of the review’s description.
Make sure to summarize the change you’re making, why you are making those changes, and link to a ticket or spec to provide any additional context. As we mentioned early on, set a timeline with your reviewers so they know how quickly you need feedback (see: “Faster is Better” for high-level guidelines).
Sometimes the most efficient way to resolve a disagreement is a direct conversation (e.g: offline or in chat). If you find yourself having long discussions on your code reviews, reach out to your reviewers to resolve any disagreements in a timely manner.
If you have discussions offline, summarize the discussion and plan of action in the code review to reference later and provide context to other reviewers.
Keep your code reviews small so that you can iterate more quickly and accurately. In general, smaller code changes are also easier to test and verify as stable.
To help keep your code reviews small, keep code reviews that change logic separate from reviews that change code style. If you have changed both, submit the code style changes as a branch and then follow-up with a branch to change logic.
Code should contain both high-level and in-line comments. High-level comments explain how all the components fit together and how it handles any exceptional cases while in-line comments describe why the code takes its current shape. This will make your code easier to understand for maintainers and reviewers.
If you feel your code review is confusing even after adding documentation, you should specify a starting point for your reviewer and detail which parts of code can be ignored.
Before submitting for review, you should review your own diff for errors. Try to do this through the eyes of someone who has never seen the code before.
Look for any decisions that may cause confusion and may need preemptive explanation. If you feel the code is too confusing, you may want to further refine your code before sending it out for review. Catching these issues early will save both you and the reviewers time.
Major changes in the middle of code review basically resets the entire review process. If you need to make major changes after submitting a review, you may want to ship your existing review and follow-up with additional changes.
If you need to make major changes after starting the code review process, make sure to communicate this to the reviewer as early in the process as possible.
It’s fine to conduct a “drafting” review to solicit preliminary feedback, but make sure to explicitly communicate this with the reviewer to avoid confusion. You’ll then want to communicate with your reviewer when your review has left “drafting” state or open a new code review.
You should understand every piece of feedback from your reviewer and respond to each actionable piece. Even if you don’t implement their feedback, respond to it and explain your reasoning. If there’s something you don’t understand, ask questions inside or outside the code review. See “Communication is key” for more information.
You can think of most code review feedback as a suggestion more than an order. It’s fine to disagree with a reviewer’s feedback but you need to explain why and give them an opportunity to respond.
You are equally as responsible for the code shipped as the person who wrote the code. You are responsible for making sure that the code is correct, well-architected, secure, and maintainable. If you’re not confident that the code meets these standards, ask a teammate to help complete the code review.
The more quickly you can return a code review to the submitter, the better. Ideally code reviews should be returned within 24 hours to maintain project momentum. This is obviously much more practical with smaller code review (see “Smaller is Better” for more info). For larger-scale code reviews, expectations should be discussed between you and the reviewee.
Keep in mind that the entire code review doesn’t need to be finished in one sitting. If the review is large, review a chunk of code at a time and communicate your progress. Never ship code until you have reviewed all of it.
When reviewing code, you should make sure that it is correct. Check that the code is bug-free, solves the intended problem and handles any edge cases appropriately. If you are dealing with data serialization/deserialization check that the code is rollback and roll-forward safe.
Think carefully about the architecture of the code. You should be able to understand each piece and how they all fit together. Confirm that the logic of each component is efficient and that the architecture is flexible but not over-engineered.
When in doubt, optimize for readability and maintainability.
Generally speaking, all code in a codebase should be tested. Spend time reviewing the testing strategy to ensure that all code is well tested. This could include unit tests, integration tests, regression tests, and so on. Make sure that the unit tests are well isolated and don’t have unnecessary dependencies.
If you point out style that needs to be changed to conform to your team’s style guidelines, link to a relevant document that outlines this. If you’re highlighting a style change that isn’t covered in your team’s guidelines, think about whether it should be in the guidelines.
If you find yourself commenting on style frequently, you should automate codestyle through a pre-commit hook. This will allow you to focus on review the code’s for correctness rather than style.
Whether you are reviewing code or having your code reviewed, communication is critical and both parties need to address that feedback is a suggestion that’s open for discussion. This may require some compromise and architecture choices. That said, as a reviewer, you should not give the code a “ship it” if you’re unsatisfied with the mitigation of an open issue.
Remember your job as a reviewer is to foster discussion so be sure to encourage open communication on and offline.