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

221–230 of 304 posts

Re: Don’t teach during code reviews

#221

Post by bdangubic »

mplanchard wrote: post #219
rerdavies wrote: post #184

Earlier quoted context omitted.

I most recently worked on a 100% code-review team. Fabulous! In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity i…

What do you mean by 100% code review?

Every line of code must be code reviewed before it goes to stage/prod

Re: Don’t teach during code reviews

#222

Post by konfusinomicon »

simplotek wrote: post #192
jvans wrote: post #166

Earlier quoted context omitted.

> 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…

> Any tips for giving feedback to senior people here? A lot of the complexity from abstraction is very hard to quantify and experienced people can have arguments that sound reasonable. Perhaps a starting point for this discussion is to point out that the DRY principle is nice and all, but there is also WET. Premature abstractions are bad code, a liability and hinder development. It's always better to have two indepen…

the rule of three comes to mind. cleverness is confusion

Re: Don’t teach during code reviews

#223

Post by nonethewiser »

femto113 wrote: post #175
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…

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. Th…

The what vs how vs why is a great distinction. Feedback on how has a place but I hesitate to give a lot of it and tend to make it non blocking.

Re: Don’t teach during code reviews

#224

Post by [deleted] »

thinking4real wrote: post #47
password11 wrote: post #17

Earlier quoted context omitted.

I think it's more of the author having a misguided opinion of what teaching is. I've had a lot of good teachers in my upbringing but not one of them has ever thought that a vague/condescending question like " I’m not sure if I understand the whole idea but could you explain what this method does? " would be an effective teaching method. Don't make teachers the punching bag for bad programmer code review. Programmers…

Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. I mean going through engineering school and rigorous STEM degrees I can say that stuff is baked into the formula. You’re derided and dogged and gaslit from the onset. Is it surprising these people graduate, become senior and perpetuate the mental unhealth?

[deleted]

Re: Don’t teach during code reviews

#225

Post by cramjabsyn »

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…

> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line

I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback.

I’m looking for basics. Will it work? is the code clear? is it commented/documented? is the approach simple and maintainable?

Some submitters really need coaching on how to receive feedback, and how to plan their timelines to incorporate revisions.

Re: Don’t teach during code reviews

#226

Post by nonethewiser »

Ensorceled wrote: post #112
peteradio wrote: post #90

Earlier quoted context omitted.

If you don't want to see that drawn out at code review time, then have the juniors consult with the seniors prior to that stage. If you don't want seniors holding juniors to standards then do away with the junior/senior title separation because it might be meaningless to you.

If I thought the junior was not up to the task I assigned them, I would have had them consult with a senior developer during the task. I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint. Saying "this can't ship without significant rework" is something that…

This absolutely does happen and is a problem. Funny seeing so many replies mischaracterizing what you're saying.

Re: Don’t teach during code reviews

#227

Post by nonethewiser »

schrodinger wrote: post #206
Ensorceled wrote: post #112

Earlier quoted context omitted.

If I thought the junior was not up to the task I assigned them, I would have had them consult with a senior developer during the task. I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint. Saying "this can't ship without significant rework" is something that…

I'm an engineering manager with a coding background 12 years coding, 3 years managing, and "assigned as task" throws a red flag for me. I believe we should be (a) understanding what preferences and skills each engineers have (b) have a conversation on how they can best make impact using these (always comprises, sometimes a project just needs to be done and they're the only one free, but by that not being the norm, th…

> E.g. expose an internal REST endpoint providing these parameters with this latency and scale by two weeks, and letting them determine the tasks needed to get there.

But that is a task

Re: Don’t teach during code reviews

#228

Post by Aeolun »

cramjabsyn wrote: post #225
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…

> I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line I feel this, and have personally experienced submitters taking feedback far too personally, to the point of even using sunken cost as justification of their design, completely overlooking valid points of feedback. I’m looking for basics. Will it work? is the code clear? is it commented/documented…

To be fair, some people are just pedantic. Just because you are the reviewer doesn’t mean things have to be done exactly the way you say.

If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated?

I aim for one or at most two rounds of review, because anything else is just depressing.

Re: Don’t teach during code reviews

#230

Post by Aeolun »

jvans wrote: post #166
alwaysbeconsing wrote: post #147

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…

Huh, I have the opposite experience. Any time I push for a little bit more abstraction a lot of people have good arguments for why not.
Post reply on HN