6 ms·

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

by sidlls 4y ago

Ensorceled 4y ago
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.
- lelandfe 4y 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...?
  - astrange 4y 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.
    - dilyevsky 4y 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.
- penjelly 4y 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.
- fatjokes 4y 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.
- sidlls 4y 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.
  - Ensorceled 4y 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.
    - deleted 4y ago
      [deleted]
    - sidlls 4y 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.
- strix_varius 4y 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.
- dilyevsky 4y 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

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

by sidlls 4y ago

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

Ensorceled 4y ago

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.

lelandfe 4y 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...?
- astrange 4y 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.
  - dilyevsky 4y 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.
penjelly 4y 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.
fatjokes 4y 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.
sidlls 4y 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.
- Ensorceled 4y 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.
  - deleted 4y ago
    [deleted]
  - sidlls 4y 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.
strix_varius 4y 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.
dilyevsky 4y 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