DEV Community

CodingBlocks

Google’s Engineering Practices – How to Navigate a Code Review

As we learn from Google about how to navigate a code review, Michael learns to not give out compliments, Joe promises to sing if we get enough new reviews, and Allen introduces a new section to the show.

For those reading this via their podcast player, this episode’s full show notes can be found at https://www.codingblocks.net/episode134.

Sponsors

Survey Says

Anonymous Vote
Sign in with Wordpress
How many hours per week do you work on average?
  • Strictly 40. Work life balance is a must.
  • Less than 40. I value my time.
  • More than 40 but less than 60. Why do I do this to myself?
  • More than 60. Please help me.
vote

News

  • Thank you, we appreciate the latest reviews:
    • Stitcher: Jean Guillaume Misteli, gitterskow

LGTM

Navigating a CL in Review

A couple starting questions when reviewing a CL (changelist):

  • Does the change make sense?
  • Does the CL have a good description?

Take a broad view of the CL

  • If the change doesn’t make sense, you need to immediately respond with WHY it shouldn’t be there.
    • Typically if you do this, you should probably also respond with what they should have done.
    • Be courteous.
    • Give a good reason why.
  • If you notice that you’re getting more than a single CL or two that doesn’t belong, you should consider putting together a quick guide to let people know what should be a part of CL’s in a particular area of code
    • This will save a lot of work and frustration.

Examine the main parts of the CL

  • Look at the file with the most changes first as that will typically aid in figuring out the rest of the CL quicker.
    • The smaller changes are usually part of that bigger change.
  • Ask the developer to point you in the right direction.
  • Ask to have the CL split into multiple smaller CL’s
  • If you see a major problem with the CL, you need to send that feedback immediately, maybe even before you look at the rest of the CL.
    • Might be that the rest of the CL isn’t even legit any longer if the major problem ends up being a show stopper.
  • Why’s it so important to review and send out feedback quickly?
    • Developers might be moving onto their next task that built off the CL in review. You want to reduce the amount of wasted effort.
    • Developers have deadlines they have to meet so if there’s a major change that needs to happen, they need to find out about it as soon as possible.

Look at the rest of the CL in an appropriate sequence

  • Looking at files in a meaningful order will help understanding the CL.
    • Reviewing the unit tests first will help with a general understanding of the CL.

Speed of Code Reviews

  • Velocity of the team is more important than the individual.
  • The individual slacking on the review gets other work done, but they slow things down for the team.
    • Looking at the other files in the CL in a meaningful order may help in speed and understanding of the CL.
  • If there are long delays in the process, it encourages rubber stamping.
  • One business day is the maximum to time to respond to a CL.
  • You don’t have to stop your flow immediately though. Wait for a natural break point, like after lunch or a meeting.
  • The primary focus on response time to the CL.
  • When is it okay to LGTM (looks good to me)?
    • The reviewer trusts the developer to address all of the issues raised.
    • The changes are minor.

How to write code review comments

  • Be kind.
  • Explain your reasoning.
  • Balance giving directions with pointing out problems.
  • Encourage simplifications or add comments instead of just complaining about complexity.
  • Courtesy is important.
    • Don’t be accusatory.
    • Don’t say “Why did you…”
    • Say “This could be simpler by…”
  • Explain why things are important.
  • It’s the developer’s responsibility to fix the code, not the reviewer’s. It’s sufficient to state the problem.
  • Code review comments should either be conveyed in code or code comments. Pull request comments aren’t easily searchable.

Handling pushback in code reviews

  • When the developer disagrees, consider if they’re right. They are probably closer to the code than you.
  • If you believe the CL improves things, then don’t give up.
  • Stay polite.
  • People tend to get more upset about the tone of comments, rather than the reviewers insistence on quality.
  • The longer you wait to clean-up, the less likely the clean-up is to happen. Better to block the request up front then move on.
  • Having a standard to point to clears up a lot of disputes.
  • Change takes time, people will adjust.

Resources We Like

  • Google Engineering Practices Documentation (GitHub)
  • Navigating a CL in review (GitHub)
  • Speed of Code Reviews (GitHub)
  • How to write code reviews comments (GitHub)
  • Handling pushback in code reviews (GitHub)
  • The CL author’s guide to getting through code review (GitHub)
  • Writing good CL descriptions (GitHub)
  • Small CLs (GitHub)
  • How to handle reviewer comments (GitHub)
  • The Myers diff algorithm: part 1 (blog.jcoglan.com)
  • Yagni (MartinFowler.com)
  • You aren’t gonna need it (Wikipedia)

Tip of the Week

  • Build your own Pi-hole for network-wide ad blocking (pi-hole.net)
    • Joe’s Pi-picks:
      • Vilros Raspberry Pi 4 4GB Complete Kit with Clear Transparent Fan Cooled Case – $99.99 (Amazon)
      • Ubiquiti Networks EdgeRouter 12 – $228.99 (Amazon)
      • GeeekPi New Raspberry Pi Cluster Case – $39.99 (Amazon)
    • uBlock Origin – Browser based plug-in for content-filtering, including ad-blocking. (Wikipedia)
  • FREE (!!!) O’Reilly site reliability engineering books made available by Google. (landing.google.com)
  • Remove *all* background noise with your NVIDIA RTX card, using NVIDIA RTX Voice (Nvidia.com)
  • scoop – A command-line installer for Windows. (scoop.sh)
  • kubefwd – Kubernetes bulk service port-forwarding (kubefwd.com)

Episode source