Live data from Hacker News

Don’t teach during code reviews

michaelagreiler.com

171–180 of 304 posts

Re: Don’t teach during code reviews

#171
post #161

As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. it'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't "bes…

galling. gauling is what the french do.

Thanks ... clearly to much Astérix & Obélix.

Re: Don’t teach during code reviews

#172
post #8

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

It’s usually very transparent when someone is pulling this tactic so I like to cut to the chase by providing some token explanation. At the end I then ask “did you have any changes you would like to propose?” Tends not to happen again after that.

Re: Don’t teach during code reviews

#173

> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…

The condescending part is that the reviewer understood how the method worked, but asked to have it explained to them again anyhow. That is dishonest.

That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?

Re: Don’t teach during code reviews

#174
post #166

Earlier quoted context omitted.

Frankly I think everyone should have this attitude, seniors as well. (I try to) I am experienced but I know I write bugs and code that may be not as clear as it could be. We all do: we're only human. Also depending on the codebase (and language to some degree), if someone senior writes a lot of overcomplex or abstract code that the rest of the team can't understand/maintain, that's just as much a problem as anything…

> if someone senior writes a lot of overcomplex or abstract code For me this is a much bigger concern than subtle changes to method names. My biggest headache digging into new codebases is when I run into layers and layers of abstraction that save 3 lines of code but force me to construct an entire mental map of the codebase before I can understand how anything works. Any tips for giving feedback to senior people her…

Is the abstraction so that they can easily swap out pieces of the system without a single headache, or write effective tests, or to make it easy to operate in a soup of services? Because then it makes complete sense.

Like dependency injection is something that a lot of juniors struggle to understand. Or in larger applications DDD and all the crap that goes along with it.

Re: Don’t teach during code reviews

#175

As a manager, it drives me crazy when some feature doesn't make it into the sprint because some "senior engineer" decides that this is the time to teach a junior/intermediate the proper way to do something with a long drawn out "teaching process" via PR comments. it'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't "bes…

This author, and 99% of anybody writing about, and large majority of anybody attempting to do code reviews misses that there are two very distinct and not particularly compatible purposes of code reviews. IMO the only really important one is identifying code that doesn't do what it's supposed to, either because the author miswrote it or misunderstood the requirement. Critically this is just "what", not how or why. This is often really simple stuff, like getting a sign wrong or storing a value to the wrong field/variable or using && instead of ||. 100% objective stuff. The other purpose I'd more broadly describe as "design review", which is focusing on "how" the code should have been written. In my experience this is mostly subjective and that's what consumes all the time. My suggested approach is to officially delineate the two, and whenever someone raises design or style issues in a code review just say "that sounds like a good topic for the design review". You can actually use PRs for both purposes, just front load the "design review" during the WIP phase for a feature branch, and then do the "code review" before merging into a mainline.

Real world example: a company I consulted for a couple years ago had a lead architect known for nit-picking code issues with junior devs ad nauseam (to the point where potential hires were told to expect it and not join if they didn't think they could take it). Their production system was brought to their knees by a doubled for loop, literally just a duplicated line of code. This code copied messages from one place to another, but almost all of their testing used cardinality 1 so it didn't affect the tests. In production all it took was a couple cardinality 2 cases because there was one edge case flow that could route copied messages back through the loop and this caused cascading failures as 2 became 4, 4 became 16, etc. This error would have been obvious to anybody reviewing the code, even if they didn't really know much about it, but as that module was written by said architect no one else felt qualified to review it.

Re: Don’t teach during code reviews

#176

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

I mean these days people take offense when you link them to the documentation or propose a better way to do something. They have too much pride to accept that they might not be hot shit.

I've had people tell me they think its insane how some people can't handle code reviews and would go to their manager about it.

Re: Don’t teach during code reviews

#177

> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…

The question itself would be fine if the asker were actually interested in the answer. What's condescending is asking the question while already knowing the answer, with the unstated ulterior motive of getting the writer to change the method name.

Re: Don’t teach during code reviews

#178

> I’m not sure if I understand the whole idea but could you explain what this method does? What's wrong with this? The author suggests that this is somehow extremely condescending, and that the reviewer should instead review at an "eye-to-eye level". So the solution is to jump to a remedy without fully understanding what the writer meant? If you are going to review at an eye-to-eye level, then you have to go in assum…

What's wrong with it is that the reviewer knows well what the method does. The reviewer just wants the name to be changed to reflect that. He's just playing dumb, perpetrating the Socratic method to guide the reviewee to the conclusion/agreement that the method needs a different name, through the giant detour of having to explain what it does, when both parties already know that.

Re: Don’t teach during code reviews

#179
post #74

Earlier quoted context omitted.

You have to fight to produce quality and you have to do it continuously. You cannot just merge in "working(tm) but not using best practices" and then turn on the quality-switch months or years later. Your attitude is creating a culture where nobody cares and it's why many good engineers end up hating their job.

Wow. You sure are extrapolating a lot about me from a simple comment. While we're extrapolating, are you the type that makes junior developers rewrite their loops as list comprehensions or the type that makes them rewrite their list comprehensions as loops? Because I've seen both in code reviews.

One problem is software developers losing sight of business goals. The business questions include: What's the cost of that code merging as is. What's the cost of the merge being delayed and the feature not being shipped. What are the different options that we can pursue to maximize the value of the business. This is going to be my problem or someone else's problem is not the way to make good decisions.

There is not going to be one answer for every situation. Let's say your startup runs out of money tomorrow, the feature must be demo'd today to raise more money, is this code getting merged or debated? If this software for a life support system the bar is set very differently. What's the cost of failures, what's the cost of future maintenance, etc. - all matters.

If the senior engineer has enough projects/years under his belt, good judgment, has seen various business outcomes, and can weigh this, then I would generally trust them as being closest to the decision point. If those are the senior engineers on your team I don't think code reviews and mentoring juniors is going to be a problem.

Re: Don’t teach during code reviews

#180

Earlier quoted context omitted.

Sure, we have code reviews for a reason, to make a better product. There are better places, and ways, to teach junior developers than long, painful code review processes.

Code review is the most common arena for getting feedback on my code. My team doesn't pair very often, though I think that's more effective. I don't believe I have other opportunities for it...?

If you're getting complete rearchitecture feedback in a code review, something's wrong because that's too late for it. Especially if it means rewriting later changes they haven't put up yet.
Post reply on HN