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

131–140 of 304 posts

Re: Don’t teach during code reviews

#131

Post by doutunlined »

lmz wrote: post #113
kdazzle wrote: post #36

Earlier quoted context omitted.

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.

Your code reviews are doomed regardless if the feedback cycle is 1 day.

Re: Don’t teach during code reviews

#132

Post by Zagill »

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've always had a pretty positive attitude towards code reviews (still a junior dev), because I know there's probably something I missed or a technique I'm not familiar with or some language quirk I didn't know about. If I submit a PR with a bunch of new functionality, I'm going to be way more concerned if I don't get a handful of comments on it than if I do.

Re: Don’t teach during code reviews

#134

Post by makeitdouble »

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's especially gauling if the original PR was working, reasonably well written, no major flaws [...] wasn't "best practices".

You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices.

I'd guess you also see half of the time your team spends on coding as some useless nitpicking you're letting them get away with by sheer generousity or bcause you don't have a choice...From the looks of it that can't be a healthy team dynamic

Re: Don’t teach during code reviews

#135

Post by [deleted] »

PragmaticPulp wrote: post #64
sidlls wrote: post #24

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 The Socratic method for teaching is actually quite good, and it can be employed without condescension.

> The Socratic method for teaching is actually quite good, and it can be employed without condescension. Socratic Method is hard to use without coming off as patronizing, even if you think you're being careful. If you're genuinely jumping in to ask real questions to understand the problem, that's great. Most of the time when I see people use Socratic Method in the workplace it's because they think they're doing the o…

[deleted]

Re: Don’t teach during code reviews

#136

Post by azov »

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…

Well, here is one way to read your comment:

"A feature not making it into a sprint" is your problem. You will get in trouble for that, it will make you look bad in front of your boss, etc. "Not following best practices" is not your problem. If the code turns into a mess, it's not you who will have to deal with the consequences, at least not you directly. As a manager you will demand that your senior developers fix that (while still shipping features, of course). After all, that's what expected of senior developers. So, the incentives for you and for your senior engineers are not aligned.

Of course I don't know if that's even remotely close to your reasoning. I don't know you or your project or your priorities, I have no idea what "best practices" we're talking about and how reasonable it is to follow or not follow them in a given situation. I might be totally wrong, you may have all the right reasons to be mad about those reviews. But the comment makes it sound like your senior engineers are idiots who block PRs for no reason, and that can't be good for your team.

Re: Don’t teach during code reviews

#137

Post by l33t233372 »

kissgyorgy wrote: post #96
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…

People without technical background shouldn't be allowed to be managers of highly technical teams.

I don’t see what here implies the grandparent commenter does not have a technical background.

Re: Don’t teach during code reviews

#138

Post by strix_varius »

Ensorceled wrote: post #123
razemio wrote: post #110

Earlier quoted context omitted.

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

Not OP, but your final comment confuses me -

Given a senior developer, a junior developer, and a non-technical manager, in the context of a code review, the vast majority of the time you should absolutely listen to the senior developer.

If that's not true in your organization, then hiring, leveling, and leadership should all be questioned.

Re: Don’t teach during code reviews

#139

Post by penjelly »

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.

i disagree, it is the place. The work shouldnt be at risk of missing release to begin with, timelines should not assume a senior dev works the issue.

Re: Don’t teach during code reviews

#140

Post by rogercafe »

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 "senior" idea over a "junior". 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 "good enough" quality and a risk/benefit analysis can be made with more information.
Post reply on HN