How to do code review?

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

Design

  • Do the interactions of various pieces of code in the CL make sense?
  • Does this change belong in your codebase, or in a library?
  • Does it integrate well with the rest of your system?
  • Is now a good time to add this functionality?

Functionality

  • Does this CL do what the developer intended?
  • Test CLs well-enough that they work correctly by the time they get to code review.
  • Think about the edge case: looking for concurrency problems, try to think like a user, making sure that there are no bugs that you just see by reading the code

Complexity

  • Is the CL more complex than it should be?
  • Are individual lines too complex?
  • Are functions too complex?
  • Are classes too complex?
  • Is this code over-engineering, added functionality that isn’t presently needed by the system.
  • Encourage developers to solve the problem they know needs to be solved now, not the problem that the developer speculates might need to be solved in the future.
  • The future problem should be solved once it arrives

Tests

  • Ask for unit, integration, end-to-end tests as appropriate for the change.

Naming

  • Did the developer pick good names for everything? A good name is long enough to fully communicate what the item is or does, without being so long that it becomes hard to read.

Comments

  • Did the developer write clear comments in understandable English?
  • Are all the comments necessary, comments should explain why some code exists, and should not be explaining what some code is doing
  • If the code isn’t clear enough to explain itself, then the code should be made simpler.

Consistency

  • What if the existing code is inconsistent with the style guide?
  • The style guide is the absolute authority: if something is required by the style guide, the CL should follow the guidelines.

Documentation

  • If a CL changes how users build, test, interact with, or release code, check to see that it also updates associated documentation, including READMEs, g3doc pages, and any generated reference docs.

Good Things

  • If you see something nice in the CL, tell the developer, especially when they addressed one of your comments in a great way.
  • Code reviews often just focus on mistakes, but they should offer encouragement and appreciation for good practices, as well.
  • It’s sometimes even more valuable, in terms of mentoring, to tell a developer what they did right than to tell them what they did wrong.
Edit this page
Last updated on