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

141–150 of 304 posts

Re: Don’t teach during code reviews

#141

Post by Hermitian909 »

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…

(I'm Not OP) - At the end of the day we're all trying to strike a balance between shipping fast and keeping technical debt down. Asking juniors to only ship code that's good as what a senior would write is very rarely the right balance point. Senior engineers must learn to understand when code is "good enough". If you're a senior and you haven't developed this skill you're bringing your team down.

Concretely, a heuristic I rely on is to understand whether the sub-par code being added "infects" the code base e.g. it changes an important interface, it adds a poorly designed interface other code will rely on etc. These are the places its important to put your foot down. Conversely, if the internals of a one-off function or class could be a little cleaner... eh, ship it.

Re: Don’t teach during code reviews

#142

Post by tremon »

throwaway892238 wrote: post #62
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.

Ask them if they want a lesson first. Not every junior wants their superiors lecturing them.

Yes, and not every kid wants to be told what to do by their parents. But it still happens, because that is part of the parents' role. As it is with senior developers.

Re: Don’t teach during code reviews

#143

Post by Ensorceled »

azov wrote: post #136
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 th…

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

Ah, I see, you think this is a general problem I have. No, in general my senior engineers are excellent and all the current ones I work with are.

I've seen enough of the other, especially while contracting.

Re: Don’t teach during code reviews

#144

Post by makeitdouble »

BlackjackCF wrote: post #126
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.

Depending on what you're pointing at, it might be better to let the other party take whatever time they need to digest your comments and adjust (including comming up with counter points)

For instance if you're telling them about a behavior defined in a RFC, it can have better long lasting impact if they get the time to read and understand it on their own, than if you're over their shoulder shoving info at them.

Basically, live teaching requires you to be good at reading the other person's state and providing the relevant info in a digestible way, while they live code. It can work, but that's a high bar to pass.

Re: Don’t teach during code reviews

#145

Post by [deleted] »

doutunlined wrote: post #122
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…

[deleted]

Re: Don’t teach during code reviews

#146

Post by fatjokes »

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.

Like what? What's that Confucius saying: "I hear and I forget, I see and I remember, I do and I understand"? Nothing beats teaching using a real example that the parties are hands on with.

I get what you're saying. Delaying a milestone to have a long convo in a CR is not good prioritization. But as the technical manager it's on you to budget more time for junior devs to land their changes.

Re: Don’t teach during code reviews

#147

Post by alwaysbeconsing »

Zagill wrote: post #132
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.

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

Re: Don’t teach during code reviews

#148

Post by phailhaus »

> 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 assuming best intentions. A method might seem poorly named, but if you're reviewing a peer then it is very likely that you just doesn't understand some context. I'd never jump in and assume a method is poorly named unless I understand why it was named that way: Chesterton's Fence.

Re: Don’t teach during code reviews

#149

Post by [deleted] »

alwaysbeconsing wrote: post #147
Zagill wrote: post #132

Earlier quoted context omitted.

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.

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…

[deleted]

Re: Don’t teach during code reviews

#150

Post by justatdotin »

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

there are two voices in my head.

the first is annoyed and frustrated. It says "Dave's proposal is just an irrelevant nit pick. Who cares if I make that change or not? it won't affect the product in any meaningful way."

the second is relaxed and easygoing. It says "Dave's proposal is just an irrelevant nit pick. Who cares if I make that change or not? it won't affect the product in any meaningful way."

I listen to the second, accept the revision, and move on.

Post reply on HN