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.
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?
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.
This should be checked at every level of the change:
Single lines of code,
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
Usually tests should be added in the same CL as the change, unless the CL is for an emergency.
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.
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.
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.
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.
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.
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.