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

101–110 of 304 posts

Re: Don’t teach during code reviews

#101

Post by ferdowsi »

bt4u wrote: post #74
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…

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.

The vast majority of questions around quality that end up surfacing in code reviews can be much more easily settled by commonly shared tooling that enforces organizational style and security practices. Leaving "best practice" up to the code review stage just ends up allowing senior engineers to demand code that they themselves would write, not what's best for an organization.

Re: Don’t teach during code reviews

#103

Post by guilhermefront »

thinking4real wrote: post #58
gitgud wrote: post #49

> I’m not sure if I understand the whole idea but could you explain what this method does? The worst part about this is that it forces more unnecessary communication in the code-review process... If you're reviewing, then say what you think needs to change and why...

Not to mention when my mind comes up with the logic to solve a coding problem, it’s absolutely not in a format that lends itself to explaining to another human So now you’re making me sit and “look stupid” because I have to actually parse out in human language why I think the way I think, meaning I have to sit and reason out why I did something This is such a terrible approach

It has something to do with "making the person realize the problem by themselves", because some people get offended if you directly tell them "do Y because it's better", but I completely agree the way is being done there is not effective.

Re: Don’t teach during code reviews

#104

Post by Ensorceled »

bt4u wrote: post #74
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…

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.

Re: Don’t teach during code reviews

#105

Post by drewcoo »

> " do you think we could rename this method to openRequest() or something along those lines?"

> I find the fact one person actively makes the other person “think”, extremely condescending

The only thing I learned from this and the other example given in the blogpost is that some stranger named Greiler thinks that "teacher" means "person who tries to be kind."

The message might work better if it were reframed as "code review feedback should be direct and succinct."

Re: Don’t teach during code reviews

#106

Post by baby »

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.

Re: Don’t teach during code reviews

#107

Post by SilasX »

bsder wrote: post #39
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.

I think the real problem here is that because everything is going online-only, we're falling into the trap of making everything public and recorded forever . Maybe I'm just too damn old, but I don't like this trend. When code reviews were in person, as a senior person, I could deliver feedback without that feedback becoming a public albatross stuck around someone's neck. We all screw up and miss things. Sometimes we…

>When code reviews were in person, as a senior person, I could deliver feedback without that feedback becoming a public albatross stuck around someone's neck. We all screw up and miss things. Sometimes we don't write the best code. Sometimes we don't know something that should be universal knowledge.

Heh, I actually ran into a funny situation because of permanence of code review.

One time I was onboarding a new employee (call him Bob) to how pull requests worked in GitHub, and I wrote up a sample one from his computer. Just to be silly, and because I didn't realize the implications, I said "okay and you fill out the body, describing what you did. As an example, let me just put in some filler text, 'hey, you people suck'."[1]

I was planning to delete the obvious-garbage PR, but I didn't realize ... GitHub doesn't let you delete PRs, only close them! And it triggers emails!

Mercifully, no one said anything. But then months later, another co-worker (call him Charlie) was venting to me about what he didn't like about Bob: "And, another thing, one time, that asshole wrote up this pull request, where he said, hey, you people suck!"

So I owned up: "Oh, uh, Charlie, that ... was actually me. I was writing a dummy pull request to onboard Bob but sent it by accident."

Then Charlie said, "Well ... he's still an asshole!"

[1] I think I wasn't planning to submit it at all, but after a while Bob probably wanted to see it in action and I forgot to remove that part. (And yes, I also now make sure to use more innocuous filler text.)

Re: Don’t teach during code reviews

#108

Post by [deleted] »

njharman wrote: post #95

The post isn't about not teaching during code reviews. It's about not doing it badly, duh? I'm actually shocked at his example. It's obviously bad practice (and just jerk/toxic behavior (don't fucking be coy, EXPLICIT > implicit)). It seems like strawman or cherry picked. I never experienced in 25 years. Quotes from article > It’s not bad to “teach” in code reviews after example of "proper" review > The learning in t…

[deleted]

Re: Don’t teach during code reviews

#109

Post by rmk »

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 is more prevalent than one thinks, particularly at large companies where senior developers will ruthlessly put roadblocks, either as a way of carrying out group politics, or keeping control over things where the risk of a new idea or paradigm exceeds the possible downsides, however minor, of rocking the boat.

However, if this is in a small team, then it's a management problem that needs to be solved by the people manager. Either the hiring process is not eliminating bad performers or people with bad attitudes, or team dynamics have deteriorated considerably.

From the senior's viewpoint, though, if they are going to be held solely responsible for janitorial work to avert juniors' substandard work blowing up down the road, or picking up the pieces when that does happen, then it makes perfect sense for the senior to put up roadblocks, junior dev's career be damned. Again, this is likely a management problem where people are not held accountable and made to maintain systems they come up with (tenure is so short, especially among junior people, that this is likely to be a systemic problem).

Re: Don’t teach during code reviews

#110

Post by razemio »

Ensorceled wrote: post #99
kissgyorgy wrote: post #96

Earlier quoted context omitted.

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

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 correct solution like it should be. Also if working code gets rejected because of bad practice, you should not be mad about it but be thankful that someone caught that before release. If your only measurement of code quality, is that something is working, I would consider that a dangerous practice.

Post reply on HN