undefined | Better HN

0 pointsEnsorceled3y ago0 comments

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.

0 comments

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

If I had to choose the best single way to grow as a software engineer, it would be through code reviews: both giving and receiving.

That said, "long, painful" code reviews is a red flag for your team. A code review should have one of four outcomes:

1. Lgtm.

2. Approved. Left some suggestions for your consideration.

3. Specific changes requested.

4. This whole approach needs to be re-evaluated, let's talk.

_None_ of those should be long or painful. It sounds like your team might be doing #4 in the PR itself when really that's an exceptional case that should make it to a usually-synchronous discussion.

sidlls3y ago

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 to impart their wisdom. It will help them grow junior engineers into high quality seniors that will help make your team’s output better and faster in the long run.

EnsorceledOP3y ago

> 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 junior developer, that are actually finished, getting held up while a "teachable moment" unfolds.

sidlls3y ago

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.

lelandfe3y ago

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

astrange3y ago

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.

dilyevsky3y ago

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.

fatjokes3y ago

Like what? What's that Confucius saying: "I hear and I forget, I see and I remember, I do and I understand"? Nothing beats teaching using a real example that the parties are hands on with.

I get what you're saying. Delaying a milestone to have a long convo in a CR is not good prioritization. But as the technical manager it's on you to budget more time for junior devs to land their changes.

penjelly3y ago

i disagree, it is the place. The work shouldnt be at risk of missing release to begin with, timelines should not assume a senior dev works the issue.

dilyevsky3y ago

> 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

j / k navigate · click thread line to collapse

0 comments

strix_varius3y ago

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

If I had to choose the best single way to grow as a software engineer, it would be through code reviews: both giving and receiving.

That said, "long, painful" code reviews is a red flag for your team. A code review should have one of four outcomes:

1. Lgtm.

2. Approved. Left some suggestions for your consideration.

3. Specific changes requested.

4. This whole approach needs to be re-evaluated, let's talk.

_None_ of those should be long or painful. It sounds like your team might be doing #4 in the PR itself when really that's an exceptional case that should make it to a usually-synchronous discussion.

sidlls3y ago

EnsorceledOP3y ago

> The context of a code review is the perfect opportunity to teach

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 junior developer, that are actually finished, getting held up while a "teachable moment" unfolds.

sidlls3y ago

lelandfe3y ago

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

astrange3y ago

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.

dilyevsky3y ago

fatjokes3y ago

Like what? What's that Confucius saying: "I hear and I forget, I see and I remember, I do and I understand"? Nothing beats teaching using a real example that the parties are hands on with.

penjelly3y ago

i disagree, it is the place. The work shouldnt be at risk of missing release to begin with, timelines should not assume a senior dev works the issue.

dilyevsky3y ago

> 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

j / k navigate · click thread line to collapse