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
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)
