Mentorship in code reviews

February 15, 2017

At Workiva, we take our code reviews seriously. We've worked hard to develop a reputation for reliability and quality—a reputation we maintain by being very exacting (some would say ruthless) when it comes to code reviews. There are benefits to code review beyond just a laser-like focus on high-quality code. They are a powerful weapon in our constant struggle to better not just our code, but ourselves.

Gauging your audience

Many of us know the numbness in the extremities which results from asking Senior Curmudgeon to review our code. We've fought our way through the fiery crucible of demands for excellence and we've emerged stronger on the other side. Our skins are thickened, our skills are refined, and our respect for the Oxford comma is unwavering. We are not, however, the typical engineers at Workiva. We grizzled veterans are an increasingly smaller proportion of the development team as we add interns and recent grads to our R&D organization.

Less-experienced developers can often benefit from a more nuanced approach to teaching via code review. Even comparative veterans feeling the ever-present tickle of imposter syndrome at the base of the spine can sometimes gain more from a style that favors critical thinking over error correction. If we can provide feedback in a manner that leads someone to discover the solution themselves, rather than just being provided with an answer, then that person is equipped to handle these situations in the future.

Dust in the wind, dude

With the pace that our industry experiences sweeping change, there is a tendency to assume that the problems we face are unique to our time, and that no one could possibly assist. After all, how could the troglodytes of the past possibly understand your latest and greatest CAP theorem-defeating algorithm? They spoke HTML 4.01 (transitional) for Pete's sake! So primitive!

Of course, nothing could be further from the truth (about the past—any disdain for HTML 4.01 is totally justified). In many cases, it's not just the previous generation of technology that we can learn from, but some really old, dead dudes. The challenges of mentoring and teaching have been around since humans first had any knowledge to share with one another. As it turns out, one of the most-effective techniques for teaching was formally described back in ancient Greece.

Even if you are not familiar with Socrates (the aforementioned old, dead dude) or the Socratic method, I can guarantee you've experienced it before. The definition of the Socratic method is:

Socratic method, also known as maieutics, method of elenchus, elenctic method, or Socratic debate, is a form of cooperative argumentative dialogue between individuals, based on asking and answering questions to stimulate critical thinking and to draw out ideas and underlying presumptions.

Socratic method, Wikipedia

That's a mouthful. Let's wash away some of the noise, shall we?

The Socratic method is a form of dialogue based on asking and answering questions to stimulate critical thinking.

That's better. It's pretty unlikely that you've managed to get through all of your years on this planet without encountering this, even if you weren't aware of it at the time.

Why do YOU think you're in trouble?
—Your mom or dad. That one time. You know the one.

 

Often when confronted by someone engaging us in Socratic dialog, the tendency is to believe that he or she is taking the lazy way out of a conversation or does not know the answer. Either or both of these might be true. That does not change the fact that you have been presented a learning opportunity.

Philosophy is boring. Weren't we talking about code reviews?

Let's have a look at an example. Here's a Python decorator that has been specially crafted in the Forges of Mordor.

 

 

def no_llamas_allowed(func):

  """

  Decorator to block the keyword argument 'llamas'.

  """

  def wrapper(*args, **kwargs):

      """

      Here's where we do the essential llama blocking.

      """

      if 'llamas' in kwargs:

          raise ValueError('NO LLAMAS ALLOWED!!!!')

      return func(*args, **kwargs)

  return wrapper

This is, of course, a contrived example, but it mirrors code I've seen in the wild. Let's take a look at some possible feedback we might dole out for this decorator.

example_1.png

This is a good example of the type of feedback that is common between experienced developers. It's the type of comment I would make to remind someone of something he or she knows but has forgotten. This isn't going to lead someone who doesn't know about functools to any greater understanding, but it can help facilitate learning.

example_2.png

Okay, I'll grant that there's a question being asked here, but no. Just. No.

example_3.png

This is more like it! Here we're asking a question (a respectful, relevant question) and not including the answer. We're not dictating that they do something; we're pointing them in the right direction. The process of figuring out what to use here and why will make a far more lasting impression on someone than the process of copying and pasting an answer.

Be flexible

At the top of this post, I spoke of gauging your audience. Not all developers are at the same stage of their learning trajectories. Oftentimes, I will be intentionally non-Socratic when giving feedback to developers with whom I have a good relationship and for whom I have a strong understanding of their skills. If I know a developer typically uses functools.wraps() and does not on his or her llama decorator, a simple "Use functools.wraps" reminder is sufficient.

When in doubt, err on the side of caution and take the opportunity to give thoughtful, meaningful input on your reviews. It is better to provide knowledge to someone who doesn't need it, than to deny it to someone who does.

Finding the time?

One objection I have heard to this sort of review feedback is that it takes too much time to be this thorough. If you approach your code reviews as mentoring opportunities, they will inevitably take you longer than a standard cowboy-shooting-from-the-hip review will take. This is expected and desired. We could write more code if we never took the time to write any tests. Doing careful reviews and working to mentor our peers is just as much about ensuring quality as writing tests are.

Authors can help

Responsibility for this type of review feedback doesn't need to be placed solely on the shoulders of the reviewers. There are things that you can do as the author of code under review, to draw out better feedback. If you find yourself facing terse copy-and-paste-worthy comments on your code, feel free to request further clarification or some justification from your reviewers. Avoid the temptation to run with the given input in the pursuit of a quick +1. Take the extra time to improve yourself along with your code.

Summary

Whenever possible, we should take the opportunity to not just critique, but also to mentor, when reviewing another's code. By asking appropriate leading questions, we can stimulate critical thinking in our peers and help them to not just correct errors in their current code, but to learn the skills and techniques that will serve to prevent errors in their future code.

No Senior Curmudgeons were harmed in the making of this blog post.

 

Add new comment

CAPTCHA
This question is for testing whether or not you are a human visitor and to prevent automated spam submissions.