Coding Blocks

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

Survey Says

How likely are you to advocate for working from home in the future?

Take the survey at: https://www.codingblocks.net/episode133.

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)
Regarding the OWNERs file …

// 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.
  • 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

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.
Direct download: coding-blocks-episode-133.mp3
Category:Software Development -- posted at: 8:01pm EDT

We dig into Google’s engineering practices documentation as we learn how to code review while Michael, er, Fives is done with proper nouns, Allen can’t get his pull request approved, and Joe prefers to take the average of his code reviews.

In case you’re reading this via your podcast player, this episode’s full show notes can be found at https://www.codingblocks.net/episode132. Be sure to check it out and join the conversation.

Sponsors

Survey Says

Do you *always* include (new or updated) unit tests with your pull requests?

Take the survey at: https://www.codingblocks.net/episode132.

News

  • Thank you to everyone that left us a review:
    • iTunes: Jbarger, Podcast Devourer, Duracce
    • Stitcher: Daemyon C

How to Code Review

Code Review Developer Guide

Q: What is a code review?

A: When someone other than the author of the code examines that code.

Q: But why code review?

A: To ensure high quality standards for code as well as helping ensure more maintainable code.

What should code reviewers look for?

  • Design: Is the code well-designed and appropriate for your system?
  • Functionality: Does the code behave as the author likely intended? Is the way the code behaves good for its users?
  • 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?
  • Tests: Does the code have correct and well-designed automated tests?
  • Naming: Did the developer choose clear names for variables, classes, methods, etc.?
  • Comments: Are the comments clear and useful?
  • Style: Does the code follow our style guides?
  • Documentation: Did the developer also update relevant documentation?

Picking the Best Reviewers

  • Get the best reviewer you can, someone who can review your code within the appropriate time frame.
    • The best reviewer is the one who can give you the most thorough review.
      • This might or might not be people in the OWNERS file.
      • Different people might need to review different portions of your changes for the same pull request.
    • If the “best” person isn’t available, they should still be CC’d on the change list.

In Person Reviews

  • If you pair-programmed with someone who was the right person for a code review, then the code is considered reviewed.
  • You can also do code reviews where the reviewer asks questions and the coder only speaks when responding to the questions.

How to do a Code Review

The Standard of a Code Review

The purpose of the code review is to make sure code quality is improving over time.

  • There are trade-offs:
    • Developers need to actually be able to complete some tasks.
    • If reviewers are a pain to work with, for example they are overly critical, then folks will be less incentivized to make good improvements or ask for good reviews in the future.
  • It is still the duty of the reviewer to make sure the code is good quality. You don’t want the health of the product or code base to degrade over time.
  • The reviewer has ownership and responsibility over the code they’re reviewing.

Reviewers should favor approving the changes when the code health is improved even if the changes aren’t perfect. There’s no such thing as perfect code, just better code.

  • Reviewers can actually reject a set of changes even if it’s quality code if they feel it doesn’t belong in “their” system.
  • Reviewers should not seek perfection but they should seek constant improvement.
    • This doesn’t mean that reviewers must stay silent. They can point out things in a comment using a prefix such as “Nit”, indicating something that could be better but doesn’t block the overall change request.

Code that worsens the overall quality or health of a system should not be admitted unless it’s under extreme/emergency circumstances.

What constitutes an emergency?

A small change that:

  • Allows a major launch to continue,
  • Fixes a significant production bug impacting users,
  • Addresses a legal issue, or
  • Patches a security hole.
What does not constitute an emergency?
  • You want the change in sooner rather than later.
  • You’ve worked hard on the feature for a long time.
  • The reviewers are away or in another timezone.
  • Because it’s Friday and you want the code merged in before the weekend.
  • A manager says that it has to be merged in today because of a soft deadline.
  • Rolling back causes test failures or breaks the build.
Mentoring

Code reviews can absolutely be used as a tool for mentoring, for example teaching design patterns, explaining algorithms, etc., but if it’s not something that needs to be changed for the PR to be completed, note it as a “Nit” or “Note”.

Principles
  • Technical facts and data overrule opinions and/or preferences.
  • The style guide is the authority. If it’s not in the style guide, it should be based on previous coding style already in the code, otherwise it’s personal preference.
  • The reviewer may request the code follow existing patterns in the code base if there isn’t a style guide.
Resolving Conflicts
  • If there are conflicts between the coder and reviewer, they should first attempt to come to a consensus based on the information discussed here as well as what’s in the CL Author’s Guide or the Reviewer Guide.
  • If the conflict remains, it’s probably worth having a face to face to discuss the issues and then make sure notes are taken to put on the code review for future reference and readers.
  • If the conflict still remains, then it’s time to escalate to a team discussion, potentially having a team leader weigh in on the decision.

NEVER let a change sit around just because the reviewer and coder can’t come to an agreement.

Resources We Like

  • Google Engineering Practices Documentation (GitHub)
  • Code Review Developer Guide (GitHub)
  • How to do a code review (GitHub)
  • The Standard of Code Review (GitHub)
  • Emergencies (GitHub)
  • The CL author’s guide to getting through code review (GitHub)
  • Technical Writing Courses (developers.google.com)
  • Ruffles Potato Chips, Cheddar and Sour Cream (Amazon)
Flawless Execution

Tip of the Week

  • William Lin’s competitive programming channel (YouTube)
  • Register for the free Microsoft Build digital event, May 19-20. (register.build.microsoft.com)
  • Apple to host virtual Worldwide Developers Conference beginning June 22 (Apple)
  • Checkstyle helps Java developers adhere to a coding standard. (checkstyle.sourceforge.io)
    • CheckStyle-IDEA – An IDEA plugin that uses Checkstyle but isn’t officially part of it. (plugins.jetbrains.com)
  • Black – The uncompromising code formatter for Python. (pypi.org)
Direct download: coding-blocks-episode-132.mp3
Category:Software Development -- posted at: 9:52pm EDT

1