Published on

Code Review Developer Guide

Authors
  • avatar
    Name
    Skim
    Twitter

What to look for:

  1. Design: Is the code well-designed and appropriate for your system?
  2. Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
  3. Complexity: Could the code be made simpler? Would another developer be able to easily understand and use this code when they come across it in the future?
  4. Tests: Does the code have correct and well-designed automated tests?
  5. Naming: Did the developer choose clear names for variables, classes, methods, etc.?
  6. Comments: Are the comments clear and useful?
  7. Style: Does the code follow our style guides?
  8. Documentation: Did the developer also update relevant documentation?

Reasons for writing small PRs:

  • Reviewed more quickly. It’s easier for a reviewer to find five minutes several times to review small CLs than to set aside a 30-minute block to review one large CL.
  • Reviewed more thoroughly. With large changes, reviewers and authors tend to get frustrated by large volumes of detailed commentary shifting back and forth—sometimes to the point where important points get missed or dropped.
  • Less likely to introduce bugs. Since you’re making fewer changes, it’s easier for you and your reviewer to reason effectively about the impact of the CL and see if a bug has been introduced.
  • Less wasted work if they are rejected. If you write a huge CL and then your reviewer says that the overall direction is wrong, you’ve wasted a lot of work.
  • Easier to merge. Working on a large CL takes a long time, so you will have lots of conflicts when you merge, and you will have to merge frequently.
  • Easier to design well. It’s a lot easier to polish the design and code health of a small change than it is to refine all the details of a large change.
  • Less blocking on reviews. Sending self-contained portions of your overall change allows you to continue coding while you wait for your current CL in review.
  • Simpler to roll back. A large CL will more likely touch files that get updated between the initial CL submission and a rollback CL, complicating the rollback (the intermediate CLs will probably need to be rolled back too).

See also: Respectful Code Reviews, Respectful Changes

Source: https://google.github.io/eng-practices