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

81–90 of 304 posts

Re: Don’t teach during code reviews

#81

Post by actually_a_dog »

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…

In my experience, the problem you’re pointing out is a non-problem. I’ve never done an in person code review in my life that didn’t take place in an interview context. Yet, somehow none of my mistakes in writing code (and there have been many) have never become “a public albatross stuck around [my] neck.”

Is this a thing you’ve seen or just something you’re afraid might happen? If the former, I would say that’s an unhealthy work environment. If the latter, then why point it out?

Re: Don’t teach during code reviews

#82

Post by [deleted] »

[deleted] wrote: post #78
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.

[deleted]

[deleted]

Re: Don’t teach during code reviews

#83

Post by ketzo »

sesuximo wrote: post #6

I disagree with the title but found myself agreeing with many points in the article. “Don’t be condescending” seems like generally applicable advice. But IMO, sometimes you just know something the code submitter doesn’t (or vise versa) and discussing that can be useful. And i think that’s pretty much teaching!

Yeah, I think the title is a little provocative to get you to read, but I ended up agreeing with it. Maybe "don't try to be a teacher during code reviews" is slightly more precise?

Re: Don’t teach during code reviews

#84

Post by impalallama »

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.

Socrates was executed for the Socratic method

Re: Don’t teach during code reviews

#85

Post by Ensorceled »

sidlls wrote: post #68
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…

"Don't let the perfect be the enemy of the good" is a fine axiom to work by, but like anything else taken to an extreme it can easily be detrimental.

Sure, we have code reviews for a reason, to make a better product.

There are better places, and ways, to teach junior developers than long, painful code review processes.

Re: Don’t teach during code reviews

#86

Post by twawaaay »

de_keyboard wrote: post #77
twawaaay wrote: post #75

Code reviews are bad anyway. This is the worst time to try to fix anything. The author has just finished (or thinks he finished) the work and any attempt to change anything by the reviewer is going to elicit resentment in most people. Try to tell the person the change cannot pass and you can make an enemy. Or you let the change in because you like the person. Either way, it is bad. And all this for naught because in…

But then you are working on half as many things.

After quarter of century as a developer with 1/3rd of this time in pair programming teams I can say that's not true.

Works goes faster when you work with another person even if for the fact it is harder to procrastinate.

And problems are always easier and cheaper to solve the earlier in the process they are caught.

Then there is the simple fact that a well executed coding review will take significant portion of the time it took to make the change. Most coding reviews are really only cheaper because the reviewer is just skimming the code to see for obvious faults. So it is not really apples to apples comparison.

Re: Don’t teach during code reviews

#87

Post by jeffbee »

twawaaay wrote: post #75

Code reviews are bad anyway. This is the worst time to try to fix anything. The author has just finished (or thinks he finished) the work and any attempt to change anything by the reviewer is going to elicit resentment in most people. Try to tell the person the change cannot pass and you can make an enemy. Or you let the change in because you like the person. Either way, it is bad. And all this for naught because in…

Anyone who resents code review isn't fit for this business. This is exactly why information given by interviewers to hiring committees focuses on the nature of the back-and-forth exchange during the interview, and decidedly not on whether the candidate completed the exercise. The most important feature of a software engineer is how well they fit into organizations.

Same above goes for anyone who believes they have "finished the work" before getting any reviews.

Re: Don’t teach during code reviews

#88

Post by ferdowsi »

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…

Agreed. Too many "senior engineers" fail to recognize when they are simply venting their territorialism by nitpicking in code reviews. The role of a good manager is to restrain these tendencies.

Re: Don’t teach during code reviews

#89

Post by twawaaay »

jeffbee wrote: post #87
twawaaay wrote: post #75

Code reviews are bad anyway. This is the worst time to try to fix anything. The author has just finished (or thinks he finished) the work and any attempt to change anything by the reviewer is going to elicit resentment in most people. Try to tell the person the change cannot pass and you can make an enemy. Or you let the change in because you like the person. Either way, it is bad. And all this for naught because in…

Anyone who resents code review isn't fit for this business. This is exactly why information given by interviewers to hiring committees focuses on the nature of the back-and-forth exchange during the interview, and decidedly not on whether the candidate completed the exercise. The most important feature of a software engineer is how well they fit into organizations. Same above goes for anyone who believes they have "f…

Ever joined a team and needed to get results from them? Usually firing every single person and starting to hire for your high standards is not an option.

There is your idealised view of how software development should work and then there is the real world. I prefer to stay grounded in the real world and figure out actual ways to help people succeed rather than telling them how they should be performing and firing if they can't meet my standard.

Whatever you think about what people "should be feeling", the truth is when they made their last commit almost everybody feels they finished their work and when it is thrown back to them they do not welcome it. I have never seen a hiring process able to only pass people who are happy to get review comments requiring them to review large part of their work.

Re: Don’t teach during code reviews

#90

Post by peteradio »

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…

If you don't want to see that drawn out at code review time, then have the juniors consult with the seniors prior to that stage. If you don't want seniors holding juniors to standards then do away with the junior/senior title separation because it might be meaningless to you.
Post reply on HN