4 ms·

> Do code reviews. Don't just skim PRs (sorry, reader!), but really dig into them: run the branch locally, test it, think critically about the design and the im

> Do code reviews. Don't just skim PRs (sorry, reader!), but really dig into them: run the branch locally, test it, think critically about the design and the implementation, and provide feedback. Record a video of your review to highlight things that could be better. Please no! Most managers want to increase output and engineers are aware of that. It is exceptionally frustrating when your manager tells you during your 1:1 that they want to help move things along and then does quite literally the opposite in a PR. If you must dive deep into a PR, get the PR unblocked and then follow up with the change. Or stop telling your direct reports that you want to help unblock them.

deleted 2y ago
[deleted]
alkonaut 2y ago
Anything that can be done in a follow-up shouldn't have to block a PR. But if the architecture is wrong, it's better to fix before than after. You are speeding your teams throughput by pointing out the problem earlier rather than later. But I don't think a manager necessarily needs to be at this level of detail.
- roenxi 2y ago
  > But if the architecture is wrong, it's better to fix before than after. Although this is true; if the manager is thinking about and getting involved in architecture after the PR is written it does suggest something has gone wrong. If there are architectural considerations then it is good to discuss them with the coder before they start developing. PR review is a great time to pick up subtle bugs, do last-line sanity checks or get used to someone's style but if they are a bad arena for combating most code issues. If they are picking up design problems there is probably a process flaw to be corrected.
  - alkonaut 2y ago
    > if the manager is thinking about and getting involved in architecture after the PR is written it does suggest something has gone wrong. If there are architectural considerations then it is good to discuss them with the coder before they start developing. That's the ideal yes. The problem with poor design/architecture is that it's never actually architected and designed. It just happens as part of a process where someone codes something without actually considering this to be a "design" (something that will affect future code, and solidify over time). So the job of whoever it is (senior developer, manager, colleague, ...) is to point out the poor design. The hope then is that it can be fixed before it is merged AND that next time there will be no "accidental design".
    - roenxi 2y ago
      This is one of the situations where the ideal is in pretty easy reach. 5 minute conversation when handing out tickets. "Hey [insert employee number here] - how are you thinking about doing this ticket?". Or check in with them during standup. That one question and a few minutes will both save hours of time and get better quality work. Letting the dev put time into work that gets redesigned in the PR is questionable management. Not the end of the world if it happens every so often but it suggests a lack of context in the job. If there is time to redesign during the PR there was time to think through what was acceptable quality before the work started.
ein0p 2y ago
Modulo the video, I did exactly this when I ran a team, only for what I thought were "important" or "gnarly" PRs. It works. I rarely had to spend more than an hour on a PR, because small, atomic PRs were all but mandatory on my team, with "atomic" taking precedence over "small". I would also review some of the PRs post-facto, after they are committed, as I was almost never a "formal"/blocking reviewer on them. Recording a video seems excessive to me. No one has the time or desire to watch me bloviate about something that I could say in a few PR comments that can be quickly skimmed.
- peterldowns 2y ago
  Can I get in touch with you to ask more about your time leading teams? No contact info in your HN profile but if you’re interested, my email is in mine.
Cthulhu_ 2y ago
If you as a manager have the time to do an in-depth code review like that... you're doing micromanagement and you're showing that you don't trust your subordinates. And clearly, you don't have anything more important to do.

deleted 2y ago

[deleted]

alkonaut 2y ago

Anything that can be done in a follow-up shouldn't have to block a PR. But if the architecture is wrong, it's better to fix before than after. You are speeding your teams throughput by pointing out the problem earlier rather than later. But I don't think a manager necessarily needs to be at this level of detail.

roenxi 2y ago
> But if the architecture is wrong, it's better to fix before than after. Although this is true; if the manager is thinking about and getting involved in architecture after the PR is written it does suggest something has gone wrong. If there are architectural considerations then it is good to discuss them with the coder before they start developing. PR review is a great time to pick up subtle bugs, do last-line sanity checks or get used to someone's style but if they are a bad arena for combating most code issues. If they are picking up design problems there is probably a process flaw to be corrected.
- alkonaut 2y ago
  > if the manager is thinking about and getting involved in architecture after the PR is written it does suggest something has gone wrong. If there are architectural considerations then it is good to discuss them with the coder before they start developing. That's the ideal yes. The problem with poor design/architecture is that it's never actually architected and designed. It just happens as part of a process where someone codes something without actually considering this to be a "design" (something that will affect future code, and solidify over time). So the job of whoever it is (senior developer, manager, colleague, ...) is to point out the poor design. The hope then is that it can be fixed before it is merged AND that next time there will be no "accidental design".
  - roenxi 2y ago
    This is one of the situations where the ideal is in pretty easy reach. 5 minute conversation when handing out tickets. "Hey [insert employee number here] - how are you thinking about doing this ticket?". Or check in with them during standup. That one question and a few minutes will both save hours of time and get better quality work. Letting the dev put time into work that gets redesigned in the PR is questionable management. Not the end of the world if it happens every so often but it suggests a lack of context in the job. If there is time to redesign during the PR there was time to think through what was acceptable quality before the work started.

ein0p 2y ago

Modulo the video, I did exactly this when I ran a team, only for what I thought were "important" or "gnarly" PRs. It works. I rarely had to spend more than an hour on a PR, because small, atomic PRs were all but mandatory on my team, with "atomic" taking precedence over "small". I would also review some of the PRs post-facto, after they are committed, as I was almost never a "formal"/blocking reviewer on them. Recording a video seems excessive to me. No one has the time or desire to watch me bloviate about something that I could say in a few PR comments that can be quickly skimmed.

peterldowns 2y ago
Can I get in touch with you to ask more about your time leading teams? No contact info in your HN profile but if you’re interested, my email is in mine.

Cthulhu_ 2y ago

If you as a manager have the time to do an in-depth code review like that... you're doing micromanagement and you're showing that you don't trust your subordinates. And clearly, you don't have anything more important to do.