Live data from Hacker News

Don’t teach during code reviews

michaelagreiler.com

231–240 of 304 posts

Re: Don’t teach during code reviews

#231
post #71

Earlier quoted context omitted.

> Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. No, instead you have your ego brutalized by spending half your youth (not to mention tens or even hundreds of thousands of dollars) getting an undergraduate and master's degree and teaching certification...only to receive poverty wages, pay for your own supplies, be abused by students and parents and a…

What kind of idiot would choose that life for themselves?

[deleted]

Re: Don’t teach during code reviews

#232

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…

Ironically, I think the people responding to and freaking out about your response proves the OPs point, that many engineers treat PRs as an exercise in ego stroking rather than reinforcing software design.

When I was a junior engineer, nothing annoyed me more than engineers trying to 'teach' during code reviews and doing so in one of two ways:

1. Being vague and unhelpful under the guise of having me 'figure it out'. They would point to a spot and say 'This needs fixing, rework this', without saying how to rework it other than to figure it out. Eventually I would need to drag them into a call to get them to tell me how they personally wanted it fixed up. Often times the way they wanted it fixed was incorrect because they misunderstood the work or were just wrong.

2. Giving incredibly bad 'improvements' to the code. Things like 'change X loop style to Y loop style' where Y doesn't affect the readability or performance of the code, or things just for the sake of adding comments.

So nowadays when I do PRs for juniors and such I try to have direct, actionable comments with rationale and room for disagreement, or if something needs to be reworked I try to assess why they did it that way and do a quick call/discussion with them. Junior engineers are generally not dumb and too many seniors treat them as such.

Re: Don’t teach during code reviews

#233
post #71

Earlier quoted context omitted.

> Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. No, instead you have your ego brutalized by spending half your youth (not to mention tens or even hundreds of thousands of dollars) getting an undergraduate and master's degree and teaching certification...only to receive poverty wages, pay for your own supplies, be abused by students and parents and a…

What kind of idiot would choose that life for themselves?

The ones who are currently teaching the kids right now?

Also, varies by country: the second world has "pedagogical universities/colleges" which are focused on producing school-level teachers in every subject taught in the schools — maths, chemistry, CS, you name it... and usually they have the lowest requirements compared to any other universities, so people apply there for a "last resort higher education" so to speak.

And then some people are bright enough to do well there, and some are not quite, and after graduation the smarter ones generally manage to find a better job than to be a school teacher while the dumber ones, well, they apply to schools to teach. And they get employed because schools almost always lack teachers. Yay...

I imagine things are better in the first world.

Re: Don’t teach during code reviews

#234
On the flip side, try not to be the kind of person who can't be taught at every moment.

Like when I was a dev and a senior was reviewing my code - I really wanted to know how and why they thought so I could learn, and I made that known.

Emotions and defensiveness are a thing and you can't change how open someone else is, but to the extent that you can keep yourself as open to and welcoming/soliciting of feedback, the faster you grow.

Re: Don’t teach during code reviews

#235
post #229

Earlier quoted context omitted.

Every line of code must be code reviewed before it goes to stage/prod

… how else do people operate? Complete yolo?

Maybe rubber stamp PR? We can say we review everything 100%, but practically this sometimes just means clicking the approve button.

Re: Don’t teach during code reviews

#236
post #213
post #188

Earlier quoted context omitted.

> Where else will people get habituated […]? Standards documentation, code labs, pair programming, instructor-led group trainings. I agree that code review is a necessary piece of the puzzle, but there are other places for engineers to acclimate too.

Aside from the first, which is almost always out of date, the rest have been nonexistent at every company I have worked. And I do not even know what 'code labs' even means. My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.

I mean, at every company I worked at 100+ emloyees people were extremely enthusiastic about you scheduling a meeting for any reason.

Re: Don’t teach during code reviews

#237

Earlier quoted context omitted.

If I thought the junior was not up to the task I assigned them, I would have had them consult with a senior developer during the task. I'm literally talking about the case where a senior developer decides that this task was not implemented to their personal standards AND makes it a "teaching moment" that means it doesn't get delivered this sprint. Saying "this can't ship without significant rework" is something that…

I'm an engineering manager with a coding background 12 years coding, 3 years managing, and "assigned as task" throws a red flag for me. I believe we should be (a) understanding what preferences and skills each engineers have (b) have a conversation on how they can best make impact using these (always comprises, sometimes a project just needs to be done and they're the only one free, but by that not being the norm, th…

[deleted]

Re: Don’t teach during code reviews

#238
post #71

Earlier quoted context omitted.

Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. I mean going through engineering school and rigorous STEM degrees I can say that stuff is baked into the formula. You’re derided and dogged and gaslit from the onset. Is it surprising these people graduate, become senior and perpetuate the mental unhealth?

> Because to become teachers, you typically don’t have to have your ego brutalized jumping through irreverent hoops. No, instead you have your ego brutalized by spending half your youth (not to mention tens or even hundreds of thousands of dollars) getting an undergraduate and master's degree and teaching certification...only to receive poverty wages, pay for your own supplies, be abused by students and parents and a…

I was actually surprised when I looked up the pay bands for my old high-school and found that (head) teachers were earning near programmer salaries (this in Europe).

Re: Don’t teach during code reviews

#239

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

The condescending part is that the reviewer understood how the method worked, but asked to have it explained to them again anyhow. That is dishonest. That is why the suggested rephrasing was to say: I had a hard time understanding how the method worked. How about renaming it?

I agree. And it can be good to rephrase your opinion into a question sometimes. You may think it should be done differently but it's good to know the reasoning behind things.

But if it's the simple renaming example then by all means lead with the suggestion.

Re: Don’t teach during code reviews

#240

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

I just want people to be clear and concise. I hate the vague shit sandwich rhetorical questions. Just tell me why you don’t like the function name instead of trying to take me on a little thinking quest for my little brain

I agree but I think honestly asking "why did you do it this way?" is important sometimes. The answer may completely change your suggestion.

For the contrived example about renaming a method, no I dont think you should ask why they named it that way.

Post reply on HN