Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Don’t teach during code reviews

michaelagreiler.com

Post reply on HN
Print view View original

121–130 of 304 posts

Re: Don’t teach during code reviews

#121

Post by tyingq »

cat_plus_plus wrote: post #26

s/teach/be petty/. 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. 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 t…

Considering context is helpful also. Like perhaps they implemented some function in an odd way because other similar functions in the existing library are also that way.

Re: Don’t teach during code reviews

#122

Post by doutunlined »

moomoo11 wrote: post #63

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship ( 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 leav…

Your comment implies juniors are only juniors for a temporary amount of time. I've worked with many SWEs (some with years of FAANG experience) who consistently code like juniors. They are aware of things like the separation of concerns, but as soon as they run into a situation wheres its easier for them to leak one layer's concern into another they will do so. Consistently doing this over months erodes the quality of a codebase.

Re: Don’t teach during code reviews

#123

Post by Ensorceled »

razemio wrote: post #110
Ensorceled wrote: post #99

Earlier quoted context omitted.

I agree. Wait, do you think I'm non-technical?

At least I think so because of your previous comment. Since I can only take this as context their might be a misunderstanding. Still I would like to point out a few thinks, I did not like about your comment. In my opinion, you are looking at the problem from the wrong angle. If code gets worse (non working) after a code review, the reviewer made some serious mistakes and/or was not guiding his colleague to the correc…

> If your only measurement of code quality, is that something is working, I would consider that a dangerous practice.

I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS.

Why are you assuming I want them all to just be rubber stamps?

And what God Complex do you have to have that you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager just don't respect "quality" at all?

Re: Don’t teach during code reviews

#124

Post by doutunlined »

Ensorceled wrote: post #104
bt4u wrote: 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.

Why would you rewrite them as loops?

Re: Don’t teach during code reviews

#125

Post by doutunlined »

Ensorceled wrote: post #65

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…

"It works" is perhaps the lowest bar you can possibly have for merging code.

Re: Don’t teach during code reviews

#126

Post by BlackjackCF »

Ensorceled wrote: post #65

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…

I’ve always thought if it’s a “teaching moment” that it makes more sense to just pair with the engineer and go through it together. Otherwise it’s a really long turnaround to go back and forth in PR comments that just frustrates all parties.

Re: Don’t teach during code reviews

#127

Post by Ensorceled »

mjlawson wrote: post #115
Ensorceled wrote: post #104

Earlier quoted context omitted.

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.

I think that these kind of reviews can potentially be squashed by building consensus around what kind of feedback is right for PRs, what conventions you agree upon using, etc. Far too often, these kind of "best practices" only exist in the senior's head (if they're even truly best practices at all), so it becomes a very frustrating moving target for a junior. I agree you should push back against this as a manager, bu…

> I agree you should push back against this as a manager, but it can be hard to do so tactfully from my experiences.

Just review this thread ... it can be utterly painful. There are at least as many senior devs who don't like to take feedback on how to deliver feedback as junior devs who don't like to take feedback.

Re: Don’t teach during code reviews

#128

Post by userbinator »

to the yearslong experience I have improving and optimizing code review processes at Microsoft and beyond.

Sorry, but that's not a brag. Quite the contrary, actually.

On the topic of code reviews, I think the style varies so greatly between teams that it's hard to generalise. I've had teams where the others were very eager to learn and ones where they'd rather not.

Re: Don’t teach during code reviews

#129

Post by [deleted] »

Ensorceled wrote: post #104
bt4u wrote: 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.

[deleted]

Re: Don’t teach during code reviews

#130

Post by Ensorceled »

doutunlined wrote: post #124
Ensorceled wrote: post #104

Earlier quoted context omitted.

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.

Why would you rewrite them as loops?

"It's much easier to read a loop than a list comprehension, please rewrite all the list comprehensions as loops."

I squashed that.

Post reply on HN