Don’t Teach during Code Reviews

In this post, I’ll show you a common pitfall you should avoid when you set out to teach during code reviews.

You probably heard it already but, yes, code reviews are an excellent way to teach, learn and help each other.

But, and here comes a big BUT, please do not “play teacher” during code reviews.

So, what do I mean by that?

Well, recently I did an exhaustive search on what people recommend when giving code review feedback for one of my upcoming code review workshops. While I saw some excellent pieces of advice, such as here and here, I also came across quite a few examples that are in strong contrast to the yearslong experience I have improving and optimizing code review processes at Microsoft and beyond.

Also, the advice strongly contradicts the findings on the latest code review research.

I thought what better way to share my perspective and reveal some code review pitfalls, than using real-world examples. Today’s one is about how (not) to “teach” during code reviews.

Making the Code Submitter Think

I’d like to start with an example from a blog post on Medium*. With over 900 claps on Medium, it seems quite a few people agree with the proposed feedback style. In this post, the author describes how a reviewer should phrase feedback. In this particular case, the reviewer thought that changing the method name to openRequest() would make it clearer what the method does and therefore increases the readability of the code.

But instead of addressing the found issues directly, and telling the code submitter that you suggest a rename of the method, the blog post author suggests writing the following code review feedback.

I’m not sure if I understand the whole idea but could you explain what this method does?

So, why would you write such a vague comment instead of directly saying what you think?

Well, the blog post author’s intention is to let the code submitter reflect and think about the code change on their own.

The author further explains that the first comment should “possibly” make the code submitter think that the function should be renamed. This would mean the code submitter found the problem by themselves, and the learning effect is multiplied.

The blog post author further explains that only after the code submitter explained the whole method to the code reviews, the reviewer should write the following:

“I get it now, sorry for being slow! I’m not sure but do you think we could rename this method to openRequest() or something along those lines?” 

Great Intention – Poor Execution

Well, it seems the post author wants to help the code submitter. What a great intention. Still, I completely disagree with this style of code review feedback, and here is why:

First of all, I find the fact one person actively makes the other person “think”, extremely condescending.

This style of “teaching” indicates strong hierarchies and a clear wish for master and apprentice roles.

It’s not bad to “teach” in code reviews, but it should happen on an eye-to-eye level. Also, thinking about learning rather than teaching in code reviews enables a more balanced reviewer-reviewee relationship.

It’s not about who teaches who, it’s about helping each other and learning from each other.

So, a nice way of phrasing this comment is to directly address the problem you see with the code and also explain why you suggest a change.

The why part is where the code submitter can understand your intention and learn from your perspective. In fact, stating what you suggest and explaining your reasons opens up a genuine collaboration opportunity where both parties can learn. Because let’s face it, it’s not always the code submitter that learns. It can very well be the code reviewer that learns something new.

Code Reviews Slow Us Down

Another substantial problem with the proposed feedback loop is that it slows the code review process down.

First of all, based on this vague comment you can only hope that the code submitter “possibly” gets what you are hinting at. Maybe they won’t come to the conclusion that the method name should be changed. Definitely, they will spend valuable time thinking about and trying to distill your message.

Then, they will spend even more time explaining a method to you, that you already understood. Just so you can tell them how you think the code can be improved.

This back and forth takes time. Valuable time. And, I do not see the benefit for either of the people involved.

Also, don’t forget slow code reviews and reduced productivity are one of the major pitfalls during code review. So intentionally dragging this process out is extremely counterproductive.

Code Reviews – An Equal Partner Relationship

Can you imagine how the code submitter feels if they would realize the whole question-answer part was not genuine but constructed by you to make them think?

Well, code reviews can easily become toxic places. And condescending feedback, master-apprentices relationships, and teachers-hats are contributing to such negative environments.

To end with a positive note, I want to wrap this post up with some tips for fast, productive, respectful feedback cycles. But first, let me show you how I’d phrase the feedback for this example:

“I had a hard time grasping what the method does. What about changing the method name to openRequest() to make the methods objective clearer and improve code readability?”

The learning in this type of comment happens implicitly from the explanation you give about why you request that change. In addition, I paid attention to phase the feedback as a question, not a demand, I used I-Messages, and I talked about the code, not the person writing the code.

In general, I’d suggest to

  • always stay humble,
  • make sure your feedback is genuine and concrete,
  • state the why for your particular change request,
  • let the code submitted know which solution you have in mind (a sketch might be enough),
  • also, keep in mind that the code submitter might be able to come up with a better solution to a problem as s/he is deeper involved in the problem,
  • and keep the context and the background of the code submitter in mind. This influences how much detail you put into explaining the “why part” of your feedback and the alternative solutions.

I’m sure this post creates some discussion. Don’t hesitate to let me know your thoughts and ideas in a respectful and constructive way below!

There is more for you

Have a look at the long list of code review articles I already wrote, such as how code reviews work at Microsoft or how Google optimizes their reviews for speed.

Also, if you liked this post, stay tuned for more articles on code reviews by hopping on my mailing list. As a thank you, I’ll send you an exclusive Code Review e-Book to help you remember the code review best practices. Get the 20-page insights to code reviews now. Not a subscriber yet? Just sign-up.

Get Weekly Software Engineering Tips Directly in Your Inbox

* indicates required

* I did not link to the original blog post on purpose. I also slightly changed the wording to obfuscate the OP, while keeping the exact same meaning. I did so because I do not want to call people out or shame anybody. Still, I wanted to give some context by mentioning the blog post, as it shows this feedback style reflects what quite a few people deem constructive and helpful code review feedback.

Dr. Michaela Greiler

I help companies improve their software development processes, like code reviewing or software testing. I work for corporations such as Microsoft, but also help smaller businesses and start-ups to ensure a productive, satisfying and efficient software engineering process.

Leave a Reply

Your email address will not be published. Required fields are marked *