Don’t teach during code reviews • Hacker News Board

Re: Don’t teach during code reviews

#51

Post by photochemsyn » Sun, Feb 05, 2023, 9:18 PM UTC

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 don't know how easy it is to avoid coming off as condescending. For example:

> "Let's say someone makes the argument that the Socratic method has major flaws related to how it is applied in practice, and that it's better to explicitly state issues with someone's approach or line of reasoning? How might you respond to such a disagreement among your cohort of senior engineers when it comes to code review practices?"

It's always going to come across as "Look I'm patiently trying to lead you arond to the conclusion that you made a mistake or don't understand things correctly, because trust me, I know better."

Also, a lot of junior people know all about how this game is played and will play along, faking the whole 'dawning realization of the error they made' thing and expressing appreciation for the master's wisdom, while secretly thinking, 'am I going to have to go through this every time I make some mistake or other?' I certainly spent quite a bit of time doing that in the past.

I think it's just a lot faster and more efficient to point out errors and issues as you see them, and any decent engineer will absorb that information pretty quickly.

Re: Don’t teach during code reviews

#52

Post by simplotek » Sun, Feb 05, 2023, 9:19 PM UTC

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.

I seriously doubt a PR is the right medium for the Socratic method, or that trying to employ it in that venue can sound anything other than condescending.

PRs are the end result of the developer's work to address an issue. Communication through a PR is asynchronous and inefficient wrt time. Engaging in gratuitous chatter in PRs actively blocks the developer from delivering his work. Pushing for needless iterations with cryptic open-ended questions where you force a developer to be on the defensive reads as if you're standing in the way while gratuitously questioning someone's ability in a very public way through a medium designed to persist discussions til the end of time.

If a PR has an issue just do the right thing and go straight to the point. If you feel the need to get into what you feel are teachable moments, reach out to the developer through your choice of direct chat.

Re: Don’t teach during code reviews

#53

Post by m463 » Sun, Feb 05, 2023, 9:20 PM UTC

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.

what some people don't get is that they're gatekeeper and have to balance that out.

Re: Don’t teach during code reviews

#54

Post by photochemsyn » Sun, Feb 05, 2023, 9:21 PM UTC

azov wrote: post #16

I’m not sure if I understand what this article has to do with teaching?.. Oh, I get it now, sorry for being slow! Do you think we could rename it to "Don't be an asshole and lie about (not) understanding things" or something along those lines? :) PS. But, titles aside, do we actually want to do teaching during code reviews? There are many activities when teaching and doing are better kept separate (like, you don't wa…

Concisely, teacher-student relationships should be formally agreed upon in advance of the actual interaction.

Re: Don’t teach during code reviews

#55

Post by thinking4real » Sun, Feb 05, 2023, 9:21 PM UTC

hannes0 wrote: post #23

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.

Seriously, I read the first sentences and got so distracted from the pop up. Wtf is that animation? The close button way too high (on iOS), so I had to scroll up again and then I had to search where I left off. I just closed the tab instead.

How else are they going to get spam email lists based off poorly advised articles?

Re: Don’t teach during code reviews

#56

Post by dan-robertson » Sun, Feb 05, 2023, 9:22 PM UTC

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…

I’ve written plenty of bad code before, and some of it was released and some of that led to tricky bugs. I don’t feel particularly ashamed of having written bad code though. Maybe I’m thinking of a different kind of feedback? Like, hopefully the worst problems can be figured out before much code is written, and if someone made some asshole critical comment, I would probably consider it reflecting poorly on them rather than me (indeed I find the permanent record of the comments I left and since regretted weighs more heavily than the record of the comments received – or the poor code committed). But I guess different people react to feedback differently, especially critical feedback and maybe you’re imagining some kind of ‘the whole thing is totally wrong and terrible’ discussion? But that feels to me like a case where a bunch of blame belongs to whatever allowed a bunch of totally wrong code to begin to be written.

Re: Don’t teach during code reviews

#57

Post by zac23or » Sun, Feb 05, 2023, 9:23 PM UTC

I hate, hate code reviews. Seems to attract crazy people. An example: My old boss did a code review on my code and sent ALL CHANGES via chat. Not instructions, code. "Please do it". I did. And in the reassessment he hated the code I wrote, which was his. He publicly cursed me out on the review tool. It is written for anyone to read years later.

He considered himself an excellent code reviewer. I've never worked with the author of the article, but the line "I'm not sure I understand the whole idea, but could you explain what this method does?" Remember my old boss. It started like that... and the next few days were a review hell.

Re: Don’t teach during code reviews

#58

Post by thinking4real » Sun, Feb 05, 2023, 9:29 PM UTC

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

Re: Don’t teach during code reviews

#59

Post by sidlls » Sun, Feb 05, 2023, 9:29 PM UTC

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…

I agree that submissions that bad shouldn't have a permanent record. If I see something egregious I'll ask the person in person, or via our chat software, and make a vague comment like, "Please make the changes we discussed over lunch," or whatever, in the comments of the submission.

Re: Don’t teach during code reviews

#60

Post by watwut » Sun, Feb 05, 2023, 9:31 PM UTC

password11 wrote: post #17

Someone wrote: post #8

Isn’t that more “don’t give vague feedback” or even “don’t misdirect the reviewee”? “I’m not sure if I understand the whole idea but could you explain what this method does?” misdirects by suggesting the reviewer thinks they need education, rather than that the reviewer thinks the code can be clearer.

I think it's more of the author having a misguided opinion of what teaching is. I've had a lot of good teachers in my upbringing but not one of them has ever thought that a vague/condescending question like " I’m not sure if I understand the whole idea but could you explain what this method does? " would be an effective teaching method. Don't make teachers the punching bag for bad programmer code review. Programmers…

Good teachers teach you in the first place. The test part and telling you what you had done wrong comes later.

Meanwhile, teaching at code review means that student gets exercise, donit without guidance and then gets list of everything that was wrong with it. And then gets told that wherever his opinion preference differs from the reviewers one, he is wrong too.

No good teacher does that.