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

181–190 of 304 posts

Re: Don’t teach during code reviews

#181

Post by [deleted] »

Hermitian909 wrote: post #141
razemio wrote: post #110

Earlier quoted context omitted.

At least I think so because of your previous comment. Since I can only take this as context their might be a misunderstanding. Still I would like to point out a few thinks, I did not like about your comment. In my opinion, you are looking at the problem from the wrong angle. If code gets worse (non working) after a code review, the reviewer made some serious mistakes and/or was not guiding his colleague to the correc…

(I'm Not OP) - At the end of the day we're all trying to strike a balance between shipping fast and keeping technical debt down. Asking juniors to only ship code that's good as what a senior would write is very rarely the right balance point. Senior engineers must learn to understand when code is "good enough". If you're a senior and you haven't developed this skill you're bringing your team down. Concretely, a heuri…

[deleted]

Re: Don’t teach during code reviews

#182

Post by yuliyp »

revskill wrote: post #14

Why care much about "style" if you already know intention clearly ? In worst case, the submitter will get "ignored" later with good advice and it's bad for him and the team itself. You don't need to pass code review to get merged. There's always a DoD for it. What to do in this case ? Just answer it the way you feel good for the team. Don't care much about style. In my case, i'm always grateful for being taught by te…

> Why care much about "style" if you already know intention clearly ?

Because it may have taken me 5 minutes to understand what the function did because its name misled me, and that was with the benefit of the context of the PR. Trying to make sense of it while debugging something broken 6 months later is going to take even longer, and getting the naming right may pay significant dividends later on.

Re: Don’t teach during code reviews

#183

Post by hrayn3 »

Ensorceled wrote: post #123
razemio wrote: post #110

Earlier quoted context omitted.

At least I think so because of your previous comment. Since I can only take this as context their might be a misunderstanding. Still I would like to point out a few thinks, I did not like about your comment. In my opinion, you are looking at the problem from the wrong angle. If code gets worse (non working) after a code review, the reviewer made some serious mistakes and/or was not guiding his colleague to the correc…

> 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 things they can use next time to do a better job faster.

Spending some time using a review as a teaching experience pays so many dividends later. People who don't touch the code don't understand that.

Of course, there's a level of 'good enough' a senior should be able to identify and approve. But the bar should be high.

Re: Don’t teach during code reviews

#184

Post by rerdavies »

moomoo11 wrote: post #63

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship ( I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leav…

I most recently worked on a 100% code-review team. Fabulous!

In my personal experience, the hump from wide-eyed novice junior to productive contributing member really doesn't take that long when every line that everyone writes is code-reviewed. The first couple of code reviews might take longer than usual. But it definitely takes much less time than it used to take to bring a junior up to speed. No net productivity in the first year used to be a rule, when bringing juniors onboard in the dark ages of computing. With 100% code reviews, that process seems to take no more than a couple of months -- maybe even as little as one month. The difference is dramatic.

And it's a fabulous way to transmit not only good culture, but also love of our craft.

Re: Don’t teach during code reviews

#185

Post by simplotek »

doutunlined wrote: post #122
moomoo11 wrote: post #63

Hm. So I just give my opinion on code reviews by just giving explanation, link to a documentation, along with my recommended change. My job is to ship software, and as an engineer/team lead it is my job to ensure the code we ship ( I think people take code reviews way too personally. Like, my priority is to ensure I don’t have headaches down the line and when I work with peers or more experience people, I barely leav…

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 someone's architectural tastes.

It makes no sense to criticize someone for focusing where it matters the most and actually deliver critical features without delay if the tradeoff is some legacy debt, and that by no mean implies they're junior.

One important skill of a senior is being able to keep their eyes on the prize and be smart about tradeoffs. Prioritizing subjective opinions over software architecture over delivering value is more in line with a junior way of thinking about software than not, and to me doesn't sound like a position to criticize others.

Re: Don’t teach during code reviews

#187

Post by logbiscuitswave »

Ensorceled wrote: post #152
makeitdouble wrote: post #134

Earlier quoted context omitted.

> it's especially gauling if the original PR was working, reasonably well written, no major flaws [...] wasn't "best practices". You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices. I'd guess you also see half of the time your team spends on coding as some useless nitpicking you're letting them get away with by sheer generousity or bcause you don't have…

> You see a clear dichotomy between well written code and what your senior engieers see as enforceable best practices. No, in general, I agree with code review comments my senior developers make. I'm talking about the difference between "best practices" and actual best practices or in the difference between "this works, but next time, a better architecture would be x, let's refactor next time we come back to this" an…

“this works, but next time, a better architecture would be x, let's refactor next time we come back to this”

In my experience there is rarely ever a “next time” and “fix it later” becomes “fix it never”. It’s always cheapest to fix worst practices up front rather than letting them metastasize into a huge pile of technical debt later. It’s also rare to find anybody interested in or willing to go back and fix old stuff when priorities often involve chasing the next shiny thing.

Re: Don’t teach during code reviews

#188

Post by Denzel »

rmk wrote: post #100

I've had instances of junior people who are simply not interested in learning and suffer from a massively inflated sense of ability and seniority, in practically every job. When reviewing code submitted by such people, "don't teach during code reviews" is actually good advice to the senior person. The senior person is saved the angst and futility of the effort. As a consequence, the other benefit of the code review,…

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

Re: Don’t teach during code reviews

#189

Post by prettyStandard »

justahuman74 wrote: post #111
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…

On the other hand, some people actually write stuff that isn't great but visually seems ok. You have the senior engineer to say 'no lets not create a mess'

> reasonably well written, no major flaws and they forced the junior to rewrite because it wasn't "best practices"

Re: Don’t teach during code reviews

#190

Post by strix_varius »

Ensorceled wrote: post #169
strix_varius wrote: post #138

Earlier quoted context omitted.

Not OP, but your final comment confuses me - Given a senior developer, a junior developer, and a non-technical manager, in the context of a code review, the vast majority of the time you should absolutely listen to the senior developer. If that's not true in your organization, then hiring, leveling, and leadership should all be questioned.

I'm a very hands on, technical manager. But thanks for assuming I'm non-technical.

Assumed? I responded to your comment:

> you assume the "senior developer" is ALWAYS in the right, and the junior and the "non-technical" manager...

Post reply on HN