Benevolent Code Reviews As a Busy Manager
How to nurture, rather than micro manage
Today’s Crafting Tech Teams issue is all about code reviews from the perspective of a busy manager. But before we begin:
Important PSA—As you may be aware Slovenia has been hit by a massive flash flood wave and heavy torrential storms in the past few days that seems to continue throughout the weekend. Below are the details to a public Karitas support account in Slovenia.
Families, schools and kindergartens have been evacuated across entire cities in areas of Slovenia, displacing nearly 5000 people overnight. Many emergency responders, volunteers, soldiers and working in harsh conditions to rescue and treat children and elderly in the affected areas. Any help in the form of donations will be much appreciated by affected families and rescuers.
Slovenska karitas
Kristanova ulica 1
1000 Ljubljana
TRR: SI56 0214 0001 5556 761
Note: POMOČ NEURJE
Sklic/reference: SI 00 624
Thank you for your support 🙏
Today’s topics:
Be clear. Reviews should educate, support and unblock. Don’t leave the owner more confused than they were before the review. I’m often guilty of this myself.
Review last. Speak last. Review last. A manager’s role can quickly oscillate from ignorance to over-managing. Learn how you can benefit from being more time conscious about your involvement.
Conserve your time. Learn how setting goals can help you review hundreds of PRs per day, instead of 3.
Treat it like an async 1 on 1. Reviews can be bug hunts. Or they can be opportunities to give granular, ongoing feedback to the team members that rely on your mentorship. Ideally, do both. If you can’t, skip the bug hunt.
Make it easy to disagree and commit. Defensiveness and ego can get in the way of anyone who’s under a bit of stress. Learn how to help your team overcome this, rather than trigger them more.
Be clear
🅇 Not Clear
“I don’t like this.”
“Refactor this please.”
“???”
“Don’t use singletons."
“Iterators are bad, convert to for loop.“
“This is wrong, look up project X“
“lgtm or?”
☑️ Clear
“Ouch, you had to touch a messy, complex module here and it’s getting worse. Let’s refactor it. If you do X and Y you can quickly get rid of code smell A and B <give link to craftingtechteams or refactoring.guru>. That should make it much easier to read and pass through review faster.“
“Sorry it seems I am missing context to understand this and I can’t extract it from the code along. It’s a bit hard to understand the intent. Could you explain it to me so we can find alternate names for doTheThing?“
“I’m afraid this is touching on a critical subsystem and I can’t let it past code review with test coverage. Sorry, company policy. Here are some suggestions for test names to get your started: …“
The key ingredient here is empathy. No one likes to be judged, but everyone likes to learn new cool things. You can’t learn if you feel judged. Emphasise the importance of us being in the same boat. The code belongs to the company, not the individual. What is being reviewed is the individual’s contribution towards the code base and what resulting output it will adapt.
Teach them to fish as much as possible instead of getting in their way.
Review last.
Don’t steal your team’s problems. Avoid getting in the way when the team members can figure it out on their own. Skim over a PR and triage first whether your unique input. Is necessary.
Prioritize reviewing after your team members. You give them the space to collaborate and iterate on their own. This approach fosters autonomy and encourages ownership of the codebase. You also get to witness the working or not working before you interfere. That will help you set guidance and direction in the upcoming days, weeks or 1-on-1 cycles.
Conserve your time
PRs can be bottomless pits. Some are 2 lines of code, some are thousands. Your strategy needs to adjust the scale. You can either follow through and read everything, only to realise you wasted your time 2 hours later. Or you can be smart about it and introduce some speed reading techniques:
Check the overall number of LOC changes
Review <10 LOC PRs instantly
Reject >500 LOC ones to be split
Check risk—large PRs and PRs touching critical infrastructure should have clear business outcomes specified in the PR header, code header or ticket. Make sure these match. Don’t waste time reviewing output that is not aligned with the outcome
Check the age of the branch. Risk increases with branch age. Integrate asap. Very old branches should be accepted instantly and cleaned post-integration.
Do the squint test. Scroll over the diffs and check for weird spacing and excessive indentation. That’s your first readability test
Check for typos and naming intent.
Encourage testing. If no tests were added, ask for someone on the team to suggest 1 test case than can easily be introduced. Or do it yourself
Verify for least surprise. Structure, design and naming should not be surprising. Leave comments and request changes on clever tricks, hacks or overly obfuscated (premature) performance improvement
Treat it like an async 1 on 1
I often shadow performance reviews with the teams I coach. A common pattern I spot is setting personal initiatives and growth plans, then not having any granular adjustments or feedback day-to-day throughout an entire quarter. Sometimes even longer than that. This results in the entire mentoring being compressed into two 45minute meetings 3-6 months apart.
Code Reviews are your secret sauce to enabling and mentoring a team member without investing high intensity to coaching sessions.
Don’t review the code. The code is the output of effort and process. Instead, review the individual effort and the team’s process with each open PR. Adjust your language and level of detail to the individual or pair that opened the request.
With a team member you have good relationships with you can use the PR to teach them something you know they struggle with by influencing them to go towards their growth area of discomfort.
Make it easy to “disagree and commit”
Here’s a simple trick to handle defensiveness: Amazon’s battle-tested leadership principle, Disagree and Commit. I observed engineers, especially less experienced ones, operate in two modes when it comes to PRs.
Business as usual
Panic
When panicking, they want the review done as fast as possible. There can be many reasons: perhaps they are late, they might have promised something, they want a hotfix moved out to fix something they feel personally responsible for, … Whatever the cause, a panicking engineer will be extremely defensive towards any review. They don’t want to learn, they don’t want to improve, they are not thinking rationally.
They want the PR to be accepted and merged asap.
If you identify the owner of the PR being in such a state, assume that they will disagree with any suggestions. That doesn’t mean they won’t do it—but they certainly won’t contribute any extra energy towards it.
This means you double-down on clarity and over-explaining. Your mantra on this type of PR follows: calm them down by giving them structure and certainty.
“In order to get this reviewed ASAP, please change A to B. Have the code snippet ready for them if it’s trivial.“
“I see you’re in a hurry. Let’s stay calm and keep refactoring to the simple code smells. I’ll approve immediately if you pull out line A to B out into a new method called X.“
But most importantly: if you can’t find something simple to push for approval and it will be rejected, call them for pairing instead. Don’t give them feedback of disapproval and then leave them hanging. This is how engineers get overwhelmed and start making stupid mistakes.