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

111–120 of 304 posts

Re: Don’t teach during code reviews

#111

Post by justahuman74 »

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…

On the other hand, some people actually write stuff that isn't great but visually seems ok. You have the senior engineer to say 'no lets not create a mess'

Re: Don’t teach during code reviews

#112

Post by Ensorceled »

peteradio wrote: post #90
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…

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 needs to get escalated, not ground out in a code review.

Re: Don’t teach during code reviews

#113

Post by lmz »

kdazzle wrote: post #36
koof wrote: post #20

I tend to agree with all of the principles here, with some caveats. What if you find yourself giving so much direct feedback that you're basically rewriting the code via comments, time and time again? Feedback or instruction that's not super direct has its place - we have to foster independence somehow. If I'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, e…

I’m with you. If I don’t think a chunk of code is readable, I’m not going to rewrite it all for them off the bat. I’ll just say that it’s not clear and could probably use some cleaning up. And they can either push back, or do something on their own, or ask if I have anything in mind, or seethe silently and ignore me. If it’s complicated or seems like they’re struggling then I’ll ask if they want to pair on a solution…

This works better if you both are in roughly the same time zone. It's not so great when you have to play "guess what the reviewer wants" with one day of latency between iterations of the review.

Re: Don’t teach during code reviews

#114

Post by Ensorceled »

justahuman74 wrote: post #111
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…

On the other hand, some people actually write stuff that isn't great but visually seems ok. You have the senior engineer to say 'no lets not create a mess'

Yes. Why do you think I have code reviews if not to stop messes from getting made?

I'm not talking about that.

Re: Don’t teach during code reviews

#115

Post by mjlawson »

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.

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, but it can be hard to do so tactfully from my experiences. You either have to say, "no," or engage in protracted debates on subjective ideas around readability and maintainability.

Re: Don’t teach during code reviews

#116

Post by justahuman74 »

throwaway892238 wrote: post #61

The worst thing during code reviews is people nit-picking my code or making comments that aren't important questions or mandatory changes. I didn'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/important question with context, or ask me to change something that needs to be changed. Otherwise, shut up. I can handle nit-picks, compliments, curiosities,…

> I didn't post this code review to get your two cents!

I'd say that you actually did, that's why it wasn't a yolo-merge

Re: Don’t teach during code reviews

#118

Post by j-bos »

throwaway892238 wrote: post #61

The worst thing during code reviews is people nit-picking my code or making comments that aren't important questions or mandatory changes. I didn'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/important question with context, or ask me to change something that needs to be changed. Otherwise, shut up. I can handle nit-picks, compliments, curiosities,…

I dunno, while I often DM people about minor improvements rather than comment directly on their code. There is value to public comments that the entire team can see. And as a bonus comments can go in with an approval, signalling their optionality.

Re: Don’t teach during code reviews

#119

Post by lelandfe »

Ensorceled wrote: post #85
sidlls wrote: post #68

Earlier quoted context omitted.

"Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.

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...?

Re: Don’t teach during code reviews

#120

Post by bigyikes »

baby wrote: post #106
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 agree to some extent. Personally, I do spend the time to teach in PRs but also approve the PR to unblock unless there's much needed changes. It unblocks people, and you quickly see if they cared about your comments if they follow up with another PR addressing them.

Yes, this is how you have your cake and eat it.

It works in reverse too, when you’re the one being reviewed and you receive feedback you disagree with.

Instead of dying on the hill and delaying your merge, just implement the feedback and then argue. You can make your point while still accommodating your teammate and moving things along.

(Obviously, this practice shouldn’t apply to feedback which is truly bad)

Post reply on HN