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

31–40 of 304 posts

Re: Don’t teach during code reviews

#31

Post by johtso »

why-el wrote: post #22

When one reviews another person's PR, it's as if you are reviewing your own. This is not a mere superficial verbiage; a few times, I'll review someone else's code, and actually block on small modifications and comment on them asking for more details, only to discover that the small change was to something I myself wrote and I forgot it so deeply as for it to appear entirely new. As one is kind to oneself often, one n…

I really resonated with the way you frame it. For me it's absolutely key that ego doesn't get in the way of teaching/learning and the quest for improvement. People need to feel safe so they don't feel bad about having their output critiqued in front of co-workers. They should relish in it! Teaching is a gift! We're all perpetually learning, forgetting and making mistakes. The enemy is the idea that we need to maintain an image of getting things right all the time.

Re: Don’t teach during code reviews

#32

Post by pavlov »

throwawaymaths wrote: post #27
coldtea wrote: post #25

Earlier quoted context omitted.

> like, you don't want to teach your partner how to dance while you're dancing Well, that's how you teach someone how to dance...

Gp probably meant on the social dance floor which is generally considered to be taboo, and doing so is a mark of either a jerk or a noob.

It really is surprisingly infuriating.

A good friend of my wife once did this to me at a wedding. She had started a pair dancing hobby maybe six months earlier and was very enthusiastic about it. When I asked her to dance, I didn’t mean I wanted a lesson. It was so out of sync with my expectations that I left with barely an excuse in the middle of the song.

Re: Don’t teach during code reviews

#35

Post by silveroriole »

HL33tibCe7 wrote: post #7

The proposed approach to code review in the mentioned medium article makes my skin crawl. So patronising and passive-aggressive.

There’s no link to the original medium article but I have a feeling the author was female or some other minority in the industry. I might be totally off base with that though. I agree that the wording is very cringy, but if I may offer one defence: I also learned to do this sometimes because otherwise juniors wouldn’t listen to me at all. You had to make them think it was their own idea or they were magnanimously granting you, an idiot (notice how they apologise for being slow), a rename to help your tiny brain understand. Otherwise you’d be stuck in an insane week-long code review with the junior furiously dismissing every single instruction until you looped in a coworker with more social power. I see this article is also by a woman and I’m happy that she’s in an environment where she doesn’t have to do that. I don’t think the other article’s wording is praise-worthy but I do think it’s a somewhat rational adaptation to an environment of hostile coworkers where you need to make yourself submissive to get anything done.

Re: Don’t teach during code reviews

#36

Post by kdazzle »

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.

Otherwise it’s kind of like - “why didn’t you just do this yourself if you had something so specific in mind?”

Re: Don’t teach during code reviews

#37

Post by amatecha »

phreack wrote: post #19

Off-topic: In case the author is around, please consider removing the email capture popup. It not only interrupted my reading of the article before getting to the main point if it - it had an animation that literally startled me, and I immediately closed the site. I can't believe I got jump scared by an ad in an article but it was incredibly offensive.

Yeah the high-contrast aspect (very dark background overlay and bright white animating block) makes it especially startling! That was pretty nuts. I also immediately closed the page, even though I wasn't done scanning/critiquing the content.

Re: Don’t teach during code reviews

#38

Post by amatecha »

cat_plus_plus wrote: post #26

s/teach/be petty/. Nobody cares about a method name and if I care, I can send my own cleanup change later instead of holding up work of others. A good compromise is to approve and still suggest a different name in comment and then trust competence of my coworkers to decide for themselves. On the other hand, if someone is loading images on a main thread of a UI app, teaching is a primary priority compared to getting t…

Yeah, totally agreed. My general principle on method names is: suggest an alternative, but it's a purely-optional suggestion. Sometimes I can think of a far more effective name for something, and I'll suggest it, but say "not required, just suggesting". There are a ton of things in code reviews that could be improvements, but are also not a big deal, or quite subjective. Then there are the things that seriously affect code quality and future maintainability, which would be the kind of thing that should hold up the review until improved or corrected. In either case, being completely clear and straightforward is always a solid approach. Asking weird rhetorical questions (as opposed to clear and direct questions) does not help the process and generally elicits uncertainty and self-doubt in the code submitter.

Re: Don’t teach during code reviews

#39

Post by bsder »

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 don't write the best code. Sometimes we don't know something that should be universal knowledge.

My correcting your mistake or lack of knowledge shouldn't get recorded for all to see.

With the way things currently are, I now have to do two code reviews. One to correct actual problems and teach/mentor and the other for the public checkin to the system codebase.

Re: Don’t teach during code reviews

#40

Post by [deleted] »

pavlov wrote: post #32
throwawaymaths wrote: post #27

Earlier quoted context omitted.

Gp probably meant on the social dance floor which is generally considered to be taboo, and doing so is a mark of either a jerk or a noob.

It really is surprisingly infuriating. A good friend of my wife once did this to me at a wedding. She had started a pair dancing hobby maybe six months earlier and was very enthusiastic about it. When I asked her to dance, I didn’t mean I wanted a lesson. It was so out of sync with my expectations that I left with barely an excuse in the middle of the song.

[deleted]
Post reply on HN