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

291–300 of 304 posts

Re: Don’t teach during code reviews

#291

Post by coffeebeqn »

nonethewiser wrote: post #240
coffeebeqn wrote: post #21

Earlier quoted context omitted.

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.

That is a straight question and definitely valid.

Re: Don’t teach during code reviews

#292

Post by tremon »

throwaway892238 wrote: post #272
tremon wrote: post #142

Earlier quoted context omitted.

Yes, and not every kid wants to be told what to do by their parents. But it still happens, because that is part of the parents' role. As it is with senior developers.

You just described Paternalism...

You write that as if every -ism contains contains an automatic and universal value judgement. For those of us who don't subscribe to that model, do you think you can rephrase your argument without isms?

Re: Don’t teach during code reviews

#293

Post by blep_ »

lmm wrote: post #260
blep_ wrote: post #214

Earlier quoted context omitted.

That doesn't really seem like the sort of thing that needs to be standardized at all, though. They're both valid, they're both readable to anyone competent, just do whatever feels right.

Mixing and matching them makes the codebase harder to maintain, especially for those juniors who we're supposedly concerned about.

I don't think it does. This isn't some big architectural thing where the choice actually has implications that reach beyond the function you're in. It's not even on the level of naming conventions, where you might have to go look elsewhere to see how something is named. If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one you pick), you can read both of them just fine.

Re: Don’t teach during code reviews

#294

Post by dilyevsky »

Ensorceled wrote: post #85
sidlls wrote: post #68

Earlier quoted context omitted.

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

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

Like what? And dont tell me it’s some kind of lecture/lab-style group meeting for 1-2hrs

Re: Don’t teach during code reviews

#295

Post by dilyevsky »

astrange wrote: post #180
lelandfe wrote: post #119

Earlier quoted context omitted.

Code review is the most common arena for getting feedback on my code. My team doesn't pair very often, though I think that's more effective. I don't believe I have other opportunities for it...?

If you're getting complete rearchitecture feedback in a code review, something's wrong because that's too late for it. Especially if it means rewriting later changes they haven't put up yet.

The wrong part happened when sender went to work on something alone for a long time then sent a giant PR all at ones instead of progressive set of changes. Don’t blame the reviewer for having to either compromise their integrity by letting it slide into the codebase or getting onto the likes of TP’s shitlist by putting their foot down.

Re: Don’t teach during code reviews

#296

Post by Denzel »

rmk wrote: post #213
Denzel wrote: 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.

> almost always out of date

Perfection need not be the enemy of good enough. Usually, whether the documentation is out of date or not, _some_ documentation is better than none. I've rarely come across an instance where documentation was actively harmful -- not to say those cases don't exist on the margins.

> And I do not even know what 'code labs' even means.

Code labs are self-guided lessons, like a tutorial, about the systems and technologies that your company is built on. They're called code labs because producing code/artifacts as part of your learning is one of the goals. For example, your company may have a custom enterprise installation process that's built upon Ansible, Terraform, AWS tooling, and a little proprietary work sprinkled on top. Some code labs may take you step-by-step through how to add a new service; configure, migrate, and wire up databases; etc. such that you'd have working code at the end of the code lab and new knowledge around the company's best practices.

Similarly you can do this for the standard language at your company. Most companies accumulate some amount of proprietary helpers, libraries, or frameworks in their standard language. A code lab can walk you through how to write code in the way your company expects using the helpers, libraries, and frameworks that are common across your teams.

> My experience is fairly typical SV stuff, so I am wondering where these companies that still do all these things are.

I have over a decade of similar experience across startups and large companies in NYC, Seattle, and SV. In the last 5 companies I've been at, all of them have had some combination of the above.

Re: Don’t teach during code reviews

#297

Post by bcrosby95 »

simplotek wrote: post #283
bcrosby95 wrote: post #209

Earlier quoted context omitted.

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…

> 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 time tax is paid by not delivering a feature. Architecture is supposed to be there to help you, not fail deadlines. Also, it seems you completely failed to read or get what I wrote. My point was that if a PR delivers a feature without defects then your team is far better…

> The time tax is paid by not delivering a feature. Architecture is supposed to be there to help you, not fail deadlines.

Most deadlines are artificial. If it's not artificial, yeah, you're going to have to sacrifice something for the sake of the real deadline. But you are sacrificing something, and it's not just subjective fluff.

> Also, it seems you completely failed to read or get what I wrote. My point was that if a PR delivers a feature without defects then your team is far better off if you accept the PR and refactor later when you can spare the time

You don't ever have spare time. When you finish a feature there's another waiting for you. At least, that's how it works literally everywhere I've worked. Working this way is just another form of over-promising and under-delivering.

> then it is if you throw a hissy fit and refuse to get the feature done if you don't get your way

Rejecting a change doesn't mean you're throwing a hissy fit.

> You are paid to create value which is in delivering features and fixing bugs. Architecture is supposed to be there to help you.

It does help you. In the medium to longer term. In the short term, literally any standard out there will slow you down - unit tests, CI/CD, shit - testing at all. It all slows you down. It doesn't mean we should just code like its 90s era PHP - the fact that we don't should be a strong signal about the value of this whole hypothesis you have going here.

> You have to pass interfaces throughout a bunch of components, which require updating all sorts of tests, and result in a large code footprint.

Funny. Your tests are slowing you down, why don't you just comment them out to get your feature in?

> Look at what you're trying to claim. It's supposedly easy and trouble-free to put together a PR that respects your personal notion of what the software architecture should be.

It's not actually just personal. We have unit tests that enforce certain architectural rules.

> Yet, the work to refactor code that breaks it is so insurmountable that you're no longer able to refactor it back to shape?

If you don't do these things for the sake of feature velocity, as a senior software engineer, why should anyone else? If no one is, how large of a problem do you think you're going to get? And do you really think its just as easy to fix something after the fact?

Of course, if you don't know how something should be, that's one thing. But knowing how it should be and just skipping it altogether is quite another.

Why are you even refactoring for reasons you view as subjective? I don't do subjective refactorings. Despite what you claim, at the point of a change, much of this stuff is very not subjective. If you show any developer a function call before a feature was added, and what it looks like after its been hacked it in some fucked way, no one is going to say the fucked version is better.

> It looks like the FANG engineer you tried to badmouth had a firmer grasp on things and on what it matters the most than you do.

Well maybe you're just so smart its trivial for you to navigate this stuff. If so, continue by all means. Until then, us mere mortals will have to adopt standards so our codebases don't outpace our ability to understand it.

Re: Don’t teach during code reviews

#298

Post by lmm »

blep_ wrote: post #293
lmm wrote: post #260

Earlier quoted context omitted.

Mixing and matching them makes the codebase harder to maintain, especially for those juniors who we're supposedly concerned about.

I don't think it does. This isn't some big architectural thing where the choice actually has implications that reach beyond the function you're in. It's not even on the level of naming conventions, where you might have to go look elsewhere to see how something is named. If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one y…

> It's not even on the level of naming conventions, where you might have to go look elsewhere to see how something is named.

It's similar to naming conventions - you wouldn't want to mix snake_case and camelCase in the same code - only more so, because it's a bigger leap from one to another. Things that are the same should look the same and things that are different should look different. If each loop in your program is represented differently, it'll be hard to understand.

> If you can understand both of them, which you should be able to even as a junior (as you even imply yourself, by saying it doesn't matter which one you pick)

Juniors can learn anything but they can't learn everything. The more trivia they have to deal with, the longer it will take to learn the big things. It doesn't matter in the same way that, say, it doesn't matter whether you pick Rails/Ruby or Python/Django for your webapp; it's still a bad idea to do both at the same time.

Re: Don’t teach during code reviews

#299

Post by sidlls »

Ensorceled wrote: post #157
sidlls wrote: post #151

Earlier quoted context omitted.

I just strongly disagree with this. The context of a code review is the perfect opportunity to teach, much like a technical design review is for system designs, and having them sit as an observer during an incident response is for triage/debugging a live system under stress. There are no better settings for teaching these skills to juniors. And you should really let your senior engineers take advantage of these times…

> The context of a code review is the perfect opportunity to teach I think we are just talking about different scales and different senior developers. Every code review is either teaching something, finding missed bugs, find better ways to do things that a senior developer makes full use of. If not, your code review is just a preformative waste of time. I'm talking about simple PRs, the kind I would assign to a junio…

But "simple PRs" with poor implementations compound over time and make for outsized tech debt in the future. I agree that addressing specific major issues within the PR (e.g. comments) is not a good approach. If there are "enough" (for some relatively subjective definition of that) "teachable" things, it's likely an in-person (or at least chat thread) conversation is warranted. Under normal circumstances (and I'd hope a junior engineer isn't making a PR to patch something for an emergency/incident resolution), a PR should be fine to hold up, as the release timeline should account for the fact that junior engineers will have longer PR cycles.

Re: Don’t teach during code reviews

#300

Post by Aeolun »

cramjabsyn wrote: post #264
Aeolun wrote: post #228

Earlier quoted context omitted.

To be fair, some people are just pedantic. Just because you are the reviewer doesn’t mean things have to be done exactly the way you say. If I never accept anything from more junior people because it isn’t perfect, how are they ever going to stay motivated? I aim for one or at most two rounds of review, because anything else is just depressing.

Ehhh far from perfection, more like “please use the native feature that handles this in 2 LOC instead of shelling out to an external 100 line script”

There’s limits and degrees to anything. I guess I never get PR’s that are quite that boneheaded.
Post reply on HN