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

201–210 of 304 posts

Re: Don’t teach during code reviews

#201

Post by simplotek »

rictic wrote: post #170
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, and it doesn't take much to convey that this is a conversation between peers. A couple simple changes I get a lot of mileage out of. Where once I'd have written: "Do [X]" Now I write: "I see [problem Y], consider making [change X] to improve it" If the reviewee agrees then the change is easy and straightforward to make, but if they're unconvinced then the phrasing invites a dialog. Or if I think I see a bug, I'…

> Clear, actionable, refutable

That's wisdom, and a clear way to make everyone around you better.

I'd add that being humble should also play a role. We have tastes and insights and preferences, and it's not productive to block PRs because of subjective, non-critical aspects. A working CICD pipeline lowers the cost of pushing a change, this we can always revisit things. It's far more important to have a team that trusts each other and feels confident to push changes fast than it is to have gatekeepers whose role ends up being one of needlessly putting breaks on a team for no justifiable reason.

Re: Don’t teach during code reviews

#202

Post by makeitdouble »

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…

> the difference between "best practices" and actual best practices

hmm...this would sound so much better if you felt your team was pushing for "actual" best practices.

Perhaps it's all working well, but from your words there's so much underlying tension and your clarification adds further distanciation between you and your team.

I'd hate to be on either side of this to be honest.

Re: Don’t teach during code reviews

#204

Post by [deleted] »

projectazorian wrote: post #172
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.

It’s usually very transparent when someone is pulling this tactic so I like to cut to the chase by providing some token explanation. At the end I then ask “did you have any changes you would like to propose?” Tends not to happen again after that.

[deleted]

Re: Don’t teach during code reviews

#205

Post by willio58 »

Zagill wrote: post #132
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've always had a pretty positive attitude towards code reviews (still a junior dev), because I know there's probably something I missed or a technique I'm not familiar with or some language quirk I didn't know about. If I submit a PR with a bunch of new functionality, I'm going to be way more concerned if I don't get a handful of comments on it than if I do.

As a more senior dev who reviews junior prs all the time I hope they have the same view. I work with a lot of great people who seem to always want to get better, so I think they do. I only wish I had the same level of scrutiny when I was a junior!

Re: Don’t teach during code reviews

#206

Post by schrodinger »

Ensorceled wrote: post #112
peteradio wrote: post #90

Earlier quoted context omitted.

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.

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, they don't mind), and (c) setting a goal for them to achieve (rather than a "task"). E.g. expose an internal REST endpoint providing these parameters with this latency and scale by two weeks, and letting them determine the tasks needed to get there. (obv this goal should be set together as well, not dictated).

Re: Don’t teach during code reviews

#207

Post by schrodinger »

Ensorceled wrote: post #130
doutunlined wrote: post #124

Earlier quoted context omitted.

Why would you rewrite them as loops?

"It's much easier to read a loop than a list comprehension, please rewrite all the list comprehensions as loops." I squashed that.

Sounds awfully micromanagey, making the decision as someone with way less context than the senior engineer… unless you have OCD and non-pragmatic engineers upholding unreasonable expectations.

Re: Don’t teach during code reviews

#208

Post by nonethewiser »

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…

This sounds like you and your peers don't really read and take the time to understand what changes are in the PR.

Re: Don’t teach during code reviews

#209

Post by bcrosby95 »

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…

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 while you have a giant festering pile of shit that you just keep making bigger and bigger and now people are talking about rewriting it.

In my experience you start actively losing velocity in this situation very quickly. If you're doing this when you still your have 3+ months on a project, you're likely making poor decisions.

I'm not saying you need to constantly gold plate your architecture. But it should be palatable.

The subjective part is what is palatable. But I don't know anyone who thinks steaming turdpiles are palatable.

Re: Don’t teach during code reviews

#210

Post by beirut_bootleg »

doutunlined wrote: post #125
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…

"It works" is perhaps the lowest bar you can possibly have for merging code.

"It works" is the most common metric I've seen juniors decide when to submit code (the rest being just trash). Add code review to that and expect it to get merged because "it works"? Not sure the problem is with the code review holding up the ship...
Post reply on HN