Making code reviews teachable moments

How applying concepts from education theory to code reviews can improve knowledge transfer across your engineering teams.

Improving your software development processes through regular code reviews is easier said than done.

Though there are ample merits of good code reviews, a pull request (PR) only ever really receives a passing glance and a thumbs up – or the more traditional, “Looks good to me!” 

In cases where teams do rally to perform code reviews, one of two likely scenarios will play out. The first will follow Parkinson’s Law of Triviality, with developers bikeshedding and nitpicking at (what should be) minor changes. The second, in instances where the team faces a large changeset, will have programmers avoiding the changes and trying to make them someone else’s problem as soon as possible. This leads to some serious cognitive dissonance around code reviews.

While much has been written about code reviews, from the issues with rubber stamping PRs, to how bikeshedding can affect psychological safety, there’s not much on how they can very effectively transfer knowledge.

Code reviews are teachable moments

“A teachable moment is an unplanned opportunity that arises in the classroom where a teacher has a chance to offer insight to his or her students.” Beth Lewis

Not every code review is going to be a teachable moment. Sometimes we really are just pushing pixels, updating strings, or modifying config files. Other times, our teams are delivering solid, well-documented features.

But there will be times when one of us has some valuable insight about a change that everyone could benefit from.

The importance of teachable moments

For Senior+ developers, sharing knowledge is one of the best routes to broadening impact and helping teammates grow. Teachable moments allow us to efficiently and effectively transfer knowledge. This could represent the sort of tribal knowledge that we hope to avoid developing, but if we endeavor to document some of this information, it can be shared endlessly without the risk of internalizing. Despite this, people rarely learn from only consulting documentation. People learn from other people, so pinpointing and acting on these teachable moments is paramount. 

Identifying teachable moments

Among all the noise in a PR, there are often some signals that indicate a teachable moment.

Novelty

New can mean a lot of things depending on the context, and in this case, we’re applying it very broadly to the PR and its author. There are some questions you can ask yourself to recognize potential teachable moments.

• Is this a brand-new feature?
• Does this introduce a new convention or configuration?
• Is it the author’s first time approaching this kind of problem?
• Is this their first PR to this repository?
• Are they new to the team?

Criticality

Critical code is that scary stuff that no one wants to touch unless they absolutely have to. These are the parts of the codebase you might describe with ‘here be dragons’ or the ones that keep the lights on. In this case, consider whether the code changes something that is mission-critical for users, or perhaps it modifies something that is a frequent source of bugs.

Redundancy

Redundant code can look like many things. Perhaps a more junior developer hasn’t followed the don’t repeat yourself (DRY) principle and duplicated code that already exists as a utility or helper method. Maybe a developer unfamiliar with the codebase, language, framework, etc., has made a change that has already been implemented by a native application programming interface (API) or a well-known open-source library.

Red flags

There can be any number of things that you might consider a red flag (like code that isn’t idiomatic), but there are some terms and phrases that should stand out and prompt further investigation. These are just suggestions for the kind of things you might notice in a changeset’s comments, PR description, commit messages, etc., that warrant a closer look.

• // TODO:: it’s common to leave a reminder of something that needs to be implemented or updated, but a PR that includes a TODO should be reviewed carefully.
• // FIXME:/// HACK:: developers have different conventions for identifying when they feel like they’ve done something in the wrong way. This is the student asking for your help.
• Hacky/janky/wonky/inconsistent: these keywords should tell you that they don’t think something is working as expected.
• Removed/updated later: anything that the author acknowledges will need to change in the near future is something that you should take extra care to read and understand.
• But it works/it seems to work: inconsistent behavior or something that seems like it shouldn’t work, but does, is a code smell that you should definitely look into.

None of these are inherently problematic and none of them should discredit a PR or its author. Rather, think of these red flags as emergency flares the author is igniting to ask for assistance.

Seizing a teachable moment

Once you’ve identified a teachable moment, your attempt to seize it will depend on how much the author trusts you and how you attempt to teach them.

Trust is the most important factor impacting our ability to step out of the role of reviewer and into the role of teacher; it ensures more frequent and more effective teachable moments.

Building trust slowly

Trust is built slowly over time. Once earned, it will give you a baseline to work with when you identify a teachable moment.

There are several ways you can establish trust over time. Sponsor your team, ask them to add to the discussion in meetings, and be sure to give credit where it is due. Fostering a more trusting environment will also require you to accept blame when you are wrong or admit when you are unsure of something.  

Building trust in the moment 

There are also ways to quickly build trust during a code review. You can establish a rapport by being authentic and humanizing yourself. Share a story about how you ran into a similar problem in the past. Admit when you aren’t sure and help them find answers to their questions. Provide links to back up your suggestions and allow them to learn more.

Learning to teach

You’ll need to apply different modes of teaching depending on the specific concept, student, and situation you’re dealing with.

Some of us want to learn by breaking things and figuring out how to fix them. Some of us want to learn by reading articles and reflecting on them. Some of us want to learn by pair programming and having a rubber duck. Some of us want to learn by watching videos and following along with a tutorial.

There is no “right” path to learning and no “right” way to teach.

Here are some approaches you can employ depending on the situation: 

• Question things: use the Socratic method to get them to think critically about their approach and explain it to you; both of you might learn something.
• Cite others: providing links to reference videos, blogs, benchmarks, discussions, etc., gives them a chance to dig into the research on their own.
• Show and tell: provide a live read evaluate print loop (REPL), or relevant playground, that allows them to experiment with your suggestions.
• Tell your story: humanize yourself by telling them how you learned the lesson you are trying to impart. 

Becoming the student

You won’t always find yourself on the giving end of a teachable moment; sometimes you will want to learn from one, too.

Maybe you just joined a new team, project, or company. In those situations, you don’t want your PRs to be rubber-stamped; you need to take advantage of every teachable moment you can get.

There are ways you can entice someone into seizing teachable moments that you create in your own PRs.

Reverse roles

It takes a lot of activation energy to review a PR. You can help lower that barrier to entry by approaching our own PR with the mindset of a reviewer. A handful of these comments can convince someone to engage in the conversation by highlighting areas where you want their input:

• Point out things that may seem confusing to others.
• Explain how you arrived at this approach.
• Ask if terse code is idiomatic or conventional.
• Ask if an open-source library could replace this code.
• Describe how you might refactor given more context.

Boldly be wrong

This approach requires confidence and the ability to set our egos aside – working best in environments with good psychological safety. 

Put up a PR addressing a problem, even if it isn’t ideal or idiomatic – even if it’s inaccurate. According to Cunningham’s Law, someone will show up to correct you if you are wrong:

“The best way to get the right answer on the Internet is not to ask a question, but to post the wrong answer.” Cunningham’s Law

For instance, if you really want to understand a confusing or dense section of the codebase, refactor it and then listen carefully to the reasons your PR reviewer gives for why no one has updated it. Does it have a convoluted third-party dependency? Does updating it have a cascading effect on other parts of the codebase or other codebases in the organization?

Frustrated by a poorly documented method or endpoint? Submit a PR that uses it incorrectly or that changes it in a way that breaks some functionality. Use the explanation someone gives you for why this will break to create more comprehensive docs that everyone will benefit from.

Wave the red flag

Sometimes you need to guide someone to the code you really need them to look at. You can use some of those red flags to prompt a more thorough examination of your code. Be sure to highlight temporary code, hacks, missing requirements, and things that aren’t idiomatic.

If your flags are ignored, call them out yourself by reversing roles.

Ask “why?”

Sometimes reviewers might not provide enough information.

Maybe they left a terse, cryptic, or unhelpful response? Maybe they just suggested some code changes without context?

You don’t want to drive people away by endlessly asking, “Why?” But you can follow the guidelines of a critical thinking game for children to lead them into a more thorough explanation.

Ask open, positive questions like:

• Why is this more [performant/readable/maintainable]?
• Why is this the convention in the codebase?
• What is the benefit of this approach? Are there any drawbacks to it?
• How could we compare performance benchmarks for that?
• How could I learn more about this?
• What if we did [insert approach here] instead?

You don’t want to start an argument; you want to encourage the reviewer to give you more insight and context.

Always be teaching

As Senior+ developers, we should be taking advantage of any chance we get to share our knowledge. The tendency to rubber stamp or nitpick PRs prevents us from being able to capitalize on these teachable moments that PRs can present.

We need to move beyond bikeshedding and superficial reviews, moving toward a pattern of identifying, embracing, and capitalizing on the teachable moments that PRs present. Doing so will ultimately educate, empower, and encourage everyone to keep learning and growing as developers.