Don’t teach during code reviews

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change.<p>My job is to ship software, and as an engineer&#x2F;team lead it is my job to ensure the code we ship (&lt;&lt;&lt; keyword) is maintainable, tested, and documented as far as any gotchas or hacks go. Ideally we want to improve the code base as well, especially when dealing with legacy code.<p>I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leave any comments or get any on my PRs for them since we are all on the same wavelength and for the most part know what we are doing.<p>A junior engineer is a… junior engineer. They are going to have a lot of comments telling them how to write better code and a good suggestion will include a code suggestion and links to read.<p>It’s how they will get better, and for bigger tickets or super juniors, I sometimes pair with them to go through their ticket together.<p>At a certain level you just know how to identify patterns and understand how to build good software. You read books like Clean Architecture or Designing Data Intensive Applications to level up.<p>Juniors tend to just write yolo shit code or add more shit to a legacy shitcode repo instead of trying to improve it. They’ll add to the mess instead of writing a new clean layer on top to which we can easily maintain and modify. But that’s why you have to show them.<p>The only thing that helps people get better is time, being humble, and being genuinely interested themselves in becoming better at their craft. I remember my junior engineer days, and seeing my PRs filled with comments. But if I didn’t have those I don’t think I’d have leveled up so fast.

I disagree with the title but found myself agreeing with many points in the article.<p>“Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!

Isn’t that more “don’t give vague feedback” or even “don’t misdirect the reviewee”?<p><i>“I’m not sure if I understand the whole idea but could you explain what this method does?”</i> misdirects by suggesting the reviewer thinks they need education, rather than that the reviewer thinks the code can be clearer.

I&#x27;ve had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, &quot;don&#x27;t teach during code reviews&quot; is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review, which is to catch bugs before they escape to the field, is lost as well.<p>For new people or junior people who have the right attitude, this is poor advice. Where else will people get habituated to the standards, preferred idioms and implementation quirks of the team&#x2F;company&#x2F;product&#x2F;subsystem?

Off-topic:<p>In case the author is around, please consider removing the email capture popup. It not only interrupted my reading of the article before getting to the main point if it - it had an animation that literally startled me, and I immediately closed the site. I can&#x27;t believe I got jump scared by an ad in an article but it was incredibly offensive.

The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.

Great article, I like the thought here. I&#x27;ve fallen for this myself, trying to &quot;teach by asking questions&quot;, but in reality just come off as patronizing or disrespectful to your counterparty. That kind of thing is a lot more suited for a discussion where it&#x27;s legitimately important that someone come to the conclusion for themselves (eg, a political argument). A code review is different, both parties already have an implicit obligation to be receptive and open to feedback. You can be direct, and talk somewhere else about the &quot;grand lesson&quot; if you want.

There is a balance to be found between pointing out what is going to cause problems and what you find personally inelegant. What I find personally inelegant I typically leave as a suggestion, while approving the MR as a whole. The contributor - who has spent more time than me thinking about his code in the context of the issue - can finish things up on their own, taking my feedback into account where they see fit. This works well for most people. Very junior developers or those new to the team&#x2F;tooling might need more explicit guidance on more mundane matters, so I will be more stringent on those matters, at least for the first few tickets.<p>If I have a serious concern I will also message&#x2F;talk to the person directly. We need to reach an agreement going forwards, rather than trying to &quot;play the game&quot; and slip code past each other&#x27;s standards. Direct communication creates good relationships and honest intentions between developers.<p>MRs can be a good opportunity to publically ariculate design decisions and trade offs. In that sense, they are good for learning. Small, well-defined, succinct issues&#x2F;MRs can be an invaluable form of documentation, helping you to reconcile bugs with expected behaviour months or even years down the line.<p>I think it&#x27;s important to keep MRs fast. The more friction there is to merging work, the less likely developers are to break up and integrate their work in clean, intelligible, atomic commits.

s&#x2F;teach&#x2F;be petty&#x2F;. Nobody cares about a method name and if I care, I can send my own cleanup change later instead of holding up work of others. A good compromise is to approve and still suggest a different name in comment and then trust competence of my coworkers to decide for themselves.<p>On the other hand, if someone is loading images on a main thread of a UI app, teaching is a primary priority compared to getting the change merged. Obviously they are misunderstanding big parts of platform they are developing for and, until I get them up to speed, they will have a hard time being productive. So even if it takes an extra week, it&#x27;s important that they learn the best practices and how to apply these in specific cases.<p>I am absolutely against mind games like asking vague questions and holding up work until the author gives me my preferred answer, neither of us have time for that.

I agree with the everything the author says in this article. But their advice includes teaching during code reviews, so the title is misleading. Explaining the &#x27;why&#x27; of a code review comment is teaching, nothing wrong with that IMO.

When one reviews another person&#x27;s PR, it&#x27;s as if you are reviewing your own. This is not a mere superficial verbiage; a few times, I&#x27;ll review someone else&#x27;s code, and actually block on small modifications and comment on them asking for more details, only to discover that the small change was to something I myself wrote and I forgot it so deeply as for it to appear entirely new. As one is kind to oneself often, one needs to be kind to the others, and trust them. It starts from there, and you co-learn and re-learn (which I think is a subtle point in the article), since whether you wrote it or them, it does not matter.

&gt; I’m not sure if I understand the whole idea but could you explain what this method does?<p>What&#x27;s wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an &quot;eye-to-eye level&quot;. So the solution is to jump to a remedy without fully understanding what the writer meant?<p>If you are going to review at an eye-to-eye level, then you have to go in assuming best intentions. A method might <i>seem</i> poorly named, but if you&#x27;re reviewing a peer then it is very likely that you just doesn&#x27;t understand some context. I&#x27;d never jump in and assume a method is poorly named unless I understand why it was named that way: Chesterton&#x27;s Fence.

How you develop people using code reviews depends on the person you&#x27;re developing. How you ensure quality code depends on who&#x27;s submitting the code. A lot of people aren&#x27;t receptive to feedback or change requests until you make them think through the issue a bit more. It&#x27;s imprudent to make hard and fast rules about code review etiquette, but a lot of the suggestions here count as useful guidance to consider.

As a manager, it drives me crazy when some feature doesn&#x27;t make it into the sprint because some &quot;senior engineer&quot; decides that this is the time to teach a junior&#x2F;intermediate the proper way to do something with a long drawn out &quot;teaching process&quot; via PR comments.<p>it&#x27;s especially galling if the original PR was working, reasonably well written, no major flaws and they forced the junior to rewrite because it wasn&#x27;t &quot;best practices&quot;.<p>Some of the comments here seem more like sabotaging junior developers career by drawing out how long it takes them to get through code reviews than teaching them anything.

Code reviews are bad anyway.<p>This is the worst time to try to fix anything. The author has just finished (or thinks he finished) the work and any attempt to change anything by the reviewer is going to elicit resentment in most people. Try to tell the person the change cannot pass and you can make an enemy. Or you let the change in because you like the person. Either way, it is bad.<p>And all this for naught because in my experience the law of sunken cost comes into action and people will try to push it as much as possible unless it really has a critical flaw.<p>For this and other reasons I prefer pair programming as an alternative to code reviews. Work progresses faster when two people do it (vs slower when one has to finish it and then another review it). You actually have two people understanding what happened. And you can avoid all of the drama because problems get fixed as the solution is being designed and written.<p>And yes, this is a good (or at least better) time to teach. Though I try to not be preachy and instead prefer to demonstrate how I solve problems and comment on why.

<i>to the yearslong experience I have improving and optimizing code review processes at Microsoft and beyond.</i><p>Sorry, but that&#x27;s not a brag. Quite the contrary, actually.<p>On the topic of code reviews, I think the style varies so greatly between teams that it&#x27;s hard to generalise. I&#x27;ve had teams where the others were very eager to learn and ones where they&#x27;d rather not.

&gt; &quot; do you think we could rename this method to openRequest() or something along those lines?&quot;<p>&gt; I find the fact one person actively makes the other person “think”, extremely condescending<p>The only thing I learned from this and the other example given in the blogpost is that some stranger named Greiler thinks that &quot;teacher&quot; means &quot;person who tries to be kind.&quot;<p>The message might work better if it were reframed as &quot;code review feedback should be direct and succinct.&quot;

I tend to agree with all of the principles here, with some caveats.<p>What if you find yourself giving so much direct feedback that you&#x27;re basically rewriting the code via comments, time and time again?<p>Feedback or instruction that&#x27;s not super direct has its place - we have to foster independence somehow. If I&#x27;m in a lead position, I have to be able to ask you to go work on a bug or think about something on your own, even if I could probably figure out an answer in a short period of time myself.<p>Trust must always be in the room in order to ask someone to work, whether it&#x27;s in code review or elsewhere. If that trust is not there, more direct feedback&#x2F;instruction can help rebuild trust, but it is not the end-all-be-all.<p>To tie this directly to the example: there may be points where I don&#x27;t give a suggested name or solution in my comment simply because I haven&#x27;t thought of one, and the reviewee may need to be able to accept that without them thinking I&#x27;m being passive aggressive. (I would do my best to communicate that context in those instances.)

I believe she had a good intention but her idea is not good at all. The Socratic Method is a fantastic way to trigger a health conversation about a topic without forcing a &quot;senior&quot; idea over a &quot;junior&quot;. In my experience, asking someone to explain their intention behind a piece of code is a good way to: 1) Help the individual validate that the code is communicating their intentions 2) Discover if there is an different angle that the individual is not considering 3) Discover that the individual has actually produced a &quot;good enough&quot; quality and a risk&#x2F;benefit analysis can be made with more information.

I would suggest reading the article before commenting.<p>The suggestion is to not make vague statements and instead say what you mean, not that code reviews shouldn&#x27;t be educational.

Using the Socratic method <i>is</i> being direct. It&#x27;s directly showing the junior what their thought process should entail when they&#x27;re writing code. I don&#x27;t ask these questions because I get off on sounding superior, or want them to play guessing games. I ask these questions because they&#x27;re the questions I ask myself when it&#x27;s my work, and part of my job is demonstrating how they should approach their tasks.

On the flip side, try not to be the kind of person who can&#x27;t be taught at every moment.<p>Like when I was a dev and a senior was reviewing my code - I really wanted to know how and why they thought so I could learn, and I made that known.<p>Emotions and defensiveness are a thing and you can&#x27;t change how open someone else is, but to the extent that you can keep yourself as open to and welcoming&#x2F;soliciting of feedback, the faster you grow.

The author&#x27;s example doesn&#x27;t work. It&#x27;s too retrospective.<p>&gt; “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?”<p>That suggestion requires grasping what the method does.<p>In the original article the feedback is given after the submitter had explained.

The whole main short-term, medium-term and long-term points of code reviews is to teach. Teaching is a gift. It doesn&#x27;t mean you have to slow things down but it&#x27;s always a gift. Anything implying otherwise negates the experience of previous engineers helping even if it seems &quot;difficult&quot; and takes extra time.

Disagree with &quot;Don&#x27;t teach during code reviews.&quot;<p>I&#x27;ve found them to be invaluable opportunities for knowledge exchange

Here’s a list of practices I call mindful code reviews. You shouldn’t unnecessarily extend the loop, but you also shouldn’t rush or auto approve. <a href="https:&#x2F;&#x2F;max.engineer&#x2F;mindful-code-reviews" rel="nofollow">https:&#x2F;&#x2F;max.engineer&#x2F;mindful-code-reviews</a>

I hate, hate code reviews. Seems to attract crazy people. An example: My old boss did a code review on my code and sent ALL CHANGES via chat. Not instructions, code. &quot;Please do it&quot;. I did. And in the reassessment he hated the code I wrote, which was his. He publicly cursed me out on the review tool. It is written for anyone to read years later.<p>He considered himself an excellent code reviewer. I&#x27;ve never worked with the author of the article, but the line &quot;I&#x27;m not sure I understand the whole idea, but could you explain what this method does?&quot; Remember my old boss. It started like that... and the next few days were a review hell.

At the end of the day, since we’re talking about a human relationship between two individuals, it depends what you can and can’t do. In my opinion, do whatever you have to do to express your talent vicariously by completing your work to whatever standards matters most to the company you work for. Individuals can do whatever they want in code reviews. My advice is conduct yourself as the author says with humility and strict purpose. Teach, don’t teach, Who cares? Like, seriously. Jr devs, think for yourselves. So many snakes out here, watch your back. That’s the advice you should be getting sometimes.

100% agree with this, especially the part about slowing down code reviews. Don’t be condescending but give your opinion directly, asking if it makes sense. I personally like “what do you think about renaming this… was thinking because…”

It&#x27;s saying don&#x27;t be roundabout trying to play teacher in code reviews and other stuff. IMO code reviews often drag on longer than they ought to, so be direct and clear offering additional clarification az needed. Sometime even use Slack or video meet if that clears things up quicker and only record the outcome on the review.<p>Respect people and their time. Also be open to sugestions and for reviewers not every suggestion is a must, make clear which ones are.

&gt; Today’s one is about how (not) to “teach” during code reviews.<p>&gt; It’s not bad to “teach” in code reviews, but it should happen on an eye-to-eye level.<p>&gt; But first, let me show you how I’d phrase the feedback for this example:<p>&gt; “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?”<p>So the headline is just wrong, it&#x27;s about not being a dick or vague when writing code reviews.

This post is too one-size-fits-all.<p>It all depends on who the parties are, their relationship to one another, their relative knowledge of the tech stack, application and problem domain, and the nature of the code being reviewed itself.<p>Your reviewing style should ideally be tailored to the above contexts. Conversely, if you use the same exact process everywhere, you’re probably doing it wrong.

This is correct, except the ones between new team member and their mentor during onboarding. Those are specifically designed to give newbie all small ifs and buts we never put into written documentation. But at this time no one expects perfect velocity on those tasks.

Often when reviewing a change I will frame my feedback in the form of a question, not because I am trying to be a Socratic jackass but because I&#x27;m not sure. Only a fool thinks they are an expert on C++ so when I say something like &quot;does this const-qualified variable declaration in namespace scope necessarily imply internal linkage?&quot; it&#x27;s because I want to know, not because I already know. And if it&#x27;s not clear to me, it also won&#x27;t be clear to the next person who reads it.<p>I do like some of this author&#x27;s articles on code review but I think they emphasize too little that the author of the change is not one of the interested parties in the review. The review is the opportunity for the organization to defend the interests of future maintainers of the code. The interests of the person proposing the change are a distant second and they should be ready and able to either advocate for their decisions or acquiesce to requests for changes.

<i>&gt; I’m not sure if I understand the whole idea but could you explain what this method does?</i><p>The worst part about this is that it forces more unnecessary communication in the code-review process...<p>If you&#x27;re reviewing, then say what you think needs to change and why...

&gt; And condescending feedback, master-apprentices relationships, and teachers-hats are contributing to such negative environments<p>Wtf. I’m sure glad I cut my teeth before advice like this became widespread

I have a manager who does the &quot;teaching&quot; thing poorly. If you&#x27;re making a request in the form of a question, fine. But if you&#x27;re trying to make me THINK, fuck off.

Agree with the approach, but it&#x27;s a very aggressive one. It makes the submitter doubt himself and conclude conclusions, and it&#x27;s not a fun experience.

Clickbait title but I generally agree being direct in code reviews is great especially if it reduces the feedback loop &amp; doesn’t drag out the code review process.

Not sure if I agree with this. Maybe because at my place the culture is much more direct. It&#x27;s more about what u r &#x27;teaching&#x27;. If u r right and proposing a better solution and u back up with evidences ppl r more than happy to consider ur comments. If u r proposing something like an alternative, then that&#x27;s not a teaching and would turn into the discussion.<p>There is no playing such games in our code review. Saves everyone&#x27;s time and directs to the points

Bad headline. If you’re not teaching what’s the point.

Clickbait title.

I’m not sure if I understand what this article has to do with teaching?.. Oh, I get it now, sorry for being slow! Do you think we could rename it to &quot;Don&#x27;t be an asshole and lie about (not) understanding things&quot; or something along those lines? :)<p>PS. But, titles aside, do we actually want to do teaching during code reviews? There are many activities when teaching and doing are better kept separate (like, you don&#x27;t want to teach your partner how to dance <i>while</i> you&#x27;re dancing). Do code reviews fall into this category? Should we consider them a doing phase or a teaching phase?..

I use junior engineers’ code review submissions as an opportunity to teach, and any senior engineer who doesn’t is committing professional malpractice. They have to learn, and that requires someone (in the case of a code review) to teach them. One can do it without being condescending, though<p>The Socratic method for teaching is actually quite good, and it can be employed without condescension.

The post isn&#x27;t about not teaching during code reviews. It&#x27;s about not doing it badly, duh? I&#x27;m actually shocked at his example. It&#x27;s obviously bad practice (and just jerk&#x2F;toxic behavior (don&#x27;t fucking be coy, EXPLICIT &gt; implicit)). It seems like strawman or cherry picked. I never experienced in 25 years.<p>Quotes from article<p>&gt; It’s not bad to “teach” in code reviews<p>after example of &quot;proper&quot; review<p>&gt; The learning in this type of comment<p>Should absolutely &quot;teach&quot; during code reviews. Developers should always be teaching and learning from each other.

&gt; I’m not sure if I understand the whole idea but could you explain what this method does?<p>Maybe...It&#x27;s going to depend on the culture. If you have a passive aggressive culture, something like this is a good fit. Otherwise, to me, you&#x27;re adding friction.<p>Yes, it might be better to make the submitter think. But if you have to be anti-truth-seek to do it, that&#x27;s a net loss.

The worst thing during code reviews is people nit-picking my code or making comments that aren&#x27;t important questions or mandatory changes. I didn&#x27;t post this code review to get your two cents! I need to ship this code! Either give it a thumbs up, ask a relevant&#x2F;important question with context, or ask me to change something that <i>needs</i> to be changed. Otherwise, shut up.<p>I can handle nit-picks, compliments, curiosities, etc, but post them in Slack, not in the middle of my work. I wouldn&#x27;t nit-pick you during a presentation (&quot;this use of bullet points isn&#x27;t very efficient, I would do it a different way&quot;) or critique your wardrobe at the water cooler. Don&#x27;t do it to my code when I&#x27;m trying to get work done.

Why care much about &quot;style&quot; if you already know intention clearly ?<p>In worst case, the submitter will get &quot;ignored&quot; later with good advice and it&#x27;s bad for him and the team itself.<p>You don&#x27;t need to pass code review to get merged. There&#x27;s always a DoD for it.<p>What to do in this case ? Just answer it the way you feel good for the team. Don&#x27;t care much about style.<p>In my case, i&#x27;m always grateful for being taught by teammates. I don&#x27;t care much about &quot;teaching&quot; or anything like that. Intention is all you care.