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

241–250 of 304 posts

Re: Don’t teach during code reviews

#241

Post by britch »

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…

I agree approve with comment and trust is the way to go.

I will say if I submit a change and my reviewer immediately opens a change to make minor edits I'm going to be annoyed.

Either it's important enough to bring up in review, or it's too minor to bother with.

Re: Don’t teach during code reviews

#242

Post by rubyist5eva »

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…

Do you talk about your “senior engineers” this way to their faces? Using scare quotes to refer to the titles of your colleagues? Do you refer to them as minions too?

You sound like an absolutely insufferable person to work under.

Re: Don’t teach during code reviews

#243

Post by nonethewiser »

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…

I think the example is too contrived. Like you said, make a non blocking comment about the name and be done.

For less trivial examples, oftentimes I think honestly asking "why did you do it this way?" Before making a suggestion is a good idea. Often we dont have the same context nor know the intention.

Emphasis on "honestly." If you just want them to do something differently then suggest that directly.

Re: Don’t teach during code reviews

#244

Post by nonethewiser »

tyingq wrote: post #121
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…

Considering context is helpful also. Like perhaps they implemented some function in an odd way because other similar functions in the existing library are also that way.

That's why _honestly_ asking why they did something a certain way first can be a good idea.

Re: Don’t teach during code reviews

#245

Post by jerglingu »

ordu wrote: post #191
phailhaus wrote: post #148

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

> What's wrong with this? I do not know what happens. Do reviewer really cannot understand, or he is trying Socrates on me? Or maybe he have some undisclosed anxiety issues, and keep himself vague on purpose, because he is afraid to show me some inner processes of his mind? Maybe it is an imposter syndrome at work or something like that? What should I do in such a situation? Should I answer his question in a direct w…

This comment is perfect and elicited the long-buried inner rage I used to feel towards a former staff in an old company. I should not have had to navigate a staff engineer’s mind minefield like my BPD ex. Additionally, a person with that degree of influence should not hate directness and potential conflict more than he hates himself

Re: Don’t teach during code reviews

#246

Post by turdprincess »

bcrosby95 wrote: post #209
simplotek wrote: post #185

Earlier quoted context omitted.

> 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. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit s…

I'm gonna call bullshit on all of this. The time tax on not leaking layers in an application that doesn't already leak layers is nearly zero. The problem is people don't want to expend the very minor extra amount of effort. So you do it once. Then the next person does it because hey we already do it there so the code is not even subjectively worse, it's objectively the same, cuz you already crossed that line. After a…

In my experience, codebases never maintain their level of quality. Unless you are taking deliberate action to improve the codebase with every commit, it's actively getting worse like you described above.

The interesting thing is that this isn't a technical problem, it's a human problem. Most folks want to clock in, do their job and clock out, and going the extra mile to improve things isn't worth the effort.

I think its the same human tendency that leaves so much pee on the toilet floor - some humans will clean up their own, but almost none will clean up someone else's.

Re: Don’t teach during code reviews

#247

Post by lkbm »

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…

A misleading method name can cause serious screw ups down the line. At best, they make reading through the code later much more confusing. It's not petty to ask for accurate and comprehensible method names.

The problem here is the mind games. If you think a method name is a problem, just say so. Don't hint and waste people's time and energy. Just say what you mean. (If you can't do that, you're in a terribly unhealthy work environment, and fixing that should be a top priority.)

Re: Don’t teach during code reviews

#248

Post by fshbbdssbbgdd »

simplotek wrote: post #185
doutunlined wrote: post #122

Earlier quoted context omitted.

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…

> 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. That says nothing without context. If you want to move fast then it's ok to break things, and leaking layers is far from a non-negotiating tradeoff. Software is soft, and you can always revisit a piece of code to refactor it to suit s…

This possibly be true in some case, but I find it to mostly be wrong. The best time to fix up some code is when you are already modifying it, because that’s when you are already fully in context to understand the effects. Going back later to clean stuff up is much less likely, because you have to pay that marginal comprehension cost again and again for each issue you fix (and are less likely to even notice the problem). If you’re making this excuse, that suggests you have internalized that it’s ok to makes your codebase worse and you are probably degrading it progressively.

Re: Don’t teach during code reviews

#249

Post by fzeroracer »

hrayn3 wrote: post #183
Ensorceled wrote: post #123

Earlier quoted context omitted.

> If your only measurement of code quality, is that something is working, I would consider that a dangerous practice. I'm really confused as to why there are so many people assuming I don't give a shit about quality when I have CODE REVIEWS. Why are you assuming I want them all to just be rubber stamps? And what God Complex do you have to have that you assume the "senior developer" is ALWAYS in the right, and the jun…

Your comments are very reminiscent of non-technical managers I've known - they often have a very shortsighted view of the value of code reviews, because they don't need to work on the code. 'It works - why don't we just merge it? Keep velocity high!' A code review is exactly where it's worth spending time making sure: the code is maintainable, doesn't degrade the quality of the repo, and above all teaches the junior…

Your and some of the other comments I've read are very reminiscent of some of the worst senior developers I've worked with, because they treat code reviews as a way of molding the code base to their own personal expectations rather than improving it especially when a junior engineer finds fault in their code. They waste time using code reviews as a teaching exercise that gives bad habits to junior developers that need to be broken later.
Post reply on HN