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:

1. Verify that the code is a correct and effective solution for the requirements at hand
2. Ensure that your code is maintainable
3. Increase shared knowledge of the codebase
4. Sharpen your team’s skills through regular feedback
5. Not be an onerous overhead on developer time

Guidelines

When posting a code review

Pick the right reviewer(s)

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.

Always have a primary reviewer

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.

Preparing your code for review

Communication is key

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.

Smaller is better

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.

Make your code easy to review

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.

Read your diff before you send it out for review

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.

During a Code Review

Avoid major changes during code review

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.

Respond to all actionable code review feedback

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.

Code reviews are a discussion, not a dictation

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.

When reviewing code

You are responsible for the code you review

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.

Time is of the Essence

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.

Review for correctness

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.

Review for core logic and architecture, not just style

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.

Review for Adequate test coverage

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.

How to communicate on code review

Link to relevant style guidelines

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.

As mentioned above, communication is key and code reviews are a discussion, not a dictation.

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.

Checklists

When posting a code review

1. Communicate context and requirements with reviewers
2. Identify the best person/people to review your change
3. Communicate your change and what it’s purpose to your reviewers
4. Establish your timeline with all reviewers if you need to ship by a specific date
5. Identify a primary reviewer
6. Define each reviewer’s responsibilities
7. Carefully read your code before publishing
8. Read your diff
9. Make sure your diff clearly represents your changes.
10. Can your code review be broken into smaller chunks?
11. Make sure your code is easy for reviewers to follow
12. Style check your code
13. Make any relevant documentation easily available for reviewers
14. Confirm that your reviewers are aware of any major changes (if any) you plan on making during review
15. Discuss feedback
16. Respond to all code review feedback. Discuss any feedback you disagree with

When doing a code review

1. Understand the code
2. Make sure you completely understand the code
3. Evaluate all the architecture tradeoffs
4. Check code quality
5. Verify code correctness
6. Check for well-organized and efficient core logic
7. Is the code as general as it needs to be, without being more general that it has to be?
8. Make sure the code is maintainable
9. Enforce stylistic consistency with the rest of the codebase
10. Verify that the code is tested well
11. Confirm adequate test coverage
12. Check tests having the right dependencies and are testing the right things
13. Make sure the code is safe to deploy
14. Ask if the code is forwards/backwards compatible. In particular, be on the lookout if the code is changing the serialization / deserialization of something
15. Run through a roll-back scenario to check for rollback safety
16. Check for any security holes and loop in the security team if you’re unsure
17. Answer: If this code breaks at 3am and you’re called to diagnose and fix this issue, will you be able to?