Google's Engineering Practices - What to Look for in a Code Review

We learn what to look for in a code review while reviewing Google's engineering practices documentation as Michael relates patterns to choo-choos, Joe has a "weird voice", and Allen has a new favorite portion of the show.

Are you reading this via your podcast player? You can find this episode's full show notes at https://www.codingblocks.net/episode133 where you can also join the conversation.

Sponsors

• University of California, Irvine Division of Continuing Education – One of the top 50 nationally ranked universities, UCI offers over 80 certificates and specialized programs designed for working professionals. Registration is NOW OPEN! Sign up and reserve your seat today!
• Datadog.com/codingblocks – Sign up today for a free 14 day trial and get a free Datadog t-shirt after your first dashboard.

Survey Says

News

• Thank you to everyone that left us a review:
  • iTunes: codewith_himanshu, SpaceDuckets, akirakinski
  • Stitcher: Anonymous (from Croatia), Llanfairpwllgwyngyll (Wikipedia), Murali Suriar
• Watch Joe solve LeetCode Problems (YouTube)

// TODO: Insert Clever Subtitle Here

Design

• This is the MOST IMPORTANT part of the review: the overall design of the changelist (CL).
• Does the code make sense?
• Does it belong in the codebase or in a library?
• Does it meld well with the rest of the system?
• Is it the right time to add it to the code base?

Functionality

• Does the CL do what it's supposed to do?
• Even if it does what it's supposed to do, is it a good change for the users, both developers and actual end-users?
• As a reviewer, you should be thinking about all the edge-cases, concurrency issues, and generally just trying to see if any bugs arise just looking at the code.
• As a reviewer, you can verify the CL if you'd like, or have the developer walk you through the changes (the actual implemented changes rather than just slogging through code).
• Google specifically calls out parallel programming types of issues that are hard to reason about (even when debugging) especially when it comes to deadlocks and similar types of situations.

Complexity

• This should be checked at every level of the change:
  • Single lines of code,
  • Functions, and
  • Classes
• Too complex is code that is not easy to understand just looking at the code. Code like this will potentially introduce bugs as developers need to change it in the future.

A particular type of complexity is over-engineering, where developers have made the code more generic than it needs to be, or added functionality that isn't presently needed by the system. Reviewers should be especially vigilant about over-engineering. 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 and you can see its actual shape and requirements in the physical universe.

Google's Engineering Practices documentation

Tests

• Usually tests should be added in the same CL as the change, unless the CL is for an emergency.
  • Emergencies were discussed in episode 132.
• Make sure the tests are correct and useful.
• Will the tests fail if the code is broken?
• Are the assertions simple and useful?
• Are the tests separated appropriately into different test methods?

Naming

• Were good names chosen?
  • A good name is long enough to be useful and not too long to be hard to read,

Comments

• Were the comments clear and understandable, in English?
• Were the comments necessary?
  • They should explain WHY code exists and NOT what it's doing.
  • If the code isn't clear enough on its own, it should be refactored.
    • Exceptions to the rule can include regular expressions and complex algorithms.
• Comments are different than documentation of code. Code documentation expresses the purpose, usage and behavior of that code.

Style

• Have a style guide. Google has one for most of the languages they use.
• Make sure the CL follows the style guide.
• If something isn't in the style guide, and as the reviewer you want to comment on the CL to make a point about style, prefix your comment with "Nit".
  • DO NOT BLOCK PR's based on personal style preference!
• Style changes should not be mixed in with "real" changes. Those should be a separate CL.

Consistency

• Google indicates that if existing code conflicts with the style guide, the style guide wins.
• If the style guide is a recommendation rather than a hard requirement, it's a judgement call on whether to follow the guide or existing code.
• If no style guide applies, the CL should remain consistent with existing code.
• Use TODO statements for cleaning up existing code if outside the scope of the CL.

Documentation

• If the CL changes any significant portion of builds, interactions, tests, etc., then appropriate README's, reference docs, etc. should be updated.
• If the CL deprecates portions of the documentation, that should also likely be removed.

Every Line

• Look over every line of non-generated, human written code.
• You need to at least understand what the code is doing.
• If you're having a hard time examining the code in a timely fashion, you may want to ask the developer to walk you through it.
  • If you can't understand it, it's very likely future developers won't either, so getting clarification is good for everyone.
• If you don't feel qualified to be the only reviewer, make sure someone else reviews the CL who is qualified, especially when you're dealing with sensitive subjects such as security, concurrency, accessibility, internationalization, etc.

Context

• Sometimes you need to back up to get a bigger view of what's changing, rather than just looking at the individual lines that changed.
  • Seeing the whole file versus the few lines that were changed might reveal that 5 lines were added to a 200 line method which likely needs to be revisited.
• Is the CL improving the health of the system?
• Is the CL complicating the system?
• Is the CL making the system more tested or less tested?
• "Don't accept CLs that degrade the code health of the system."
  • Most systems become complex through many small changes.

Good Things

• If you see something good in a CL, let the author know.
• Many times we focus on mistakes as reviewers, but some positive reinforcement may actually be more valuable.
  • Especially true when mentoring.

Resources We Like

• OWNERS files (chormium.googlesource.com)
• Modern Code Review: A Case Study at Google (research.google)
• Google Engineering Practices Documentation (GitHub)
• What to look for in a code review (GitHub)
• Comparing Git Workflows (episode 90)
• Google Style Guides (GitHub)
• Perl Special Variables Quick Reference (PerlMonks)
• Email Address Regular Expression That 99.99% Works. (emailregex.com)

Tip of the Week

• List of common misconceptions (Wikipedia)
• The unofficial extension that integrates Draw.io into VS Code. (marketplace.visualstudio.com)
• Use Dataproc's Cluster properties to easily update XML settings. (cloud.google.com)
• Bonus tip: Include a Dockerfile (or Docker Compose) file with your open source project to help it gain traction.