5 ms·

> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. No, it’s “this refactor looks very different t

> This is planning issue, if you correctly size tickets you aren’t going to end up in messy situations as often. No, it’s “this refactor looks very different to the original code because the original code thought it was doing two different things and it’s only by stepping through it with real customer data that you realized with the right inputs (not documented) it could do a third thing (not documented) that had very important “side effects” and was a no-op in the original code flow. Yea, it touches a lot of files. Ok, yea, I can break it up step by step, and wait a few days between approval for each of them so that you never have to actually understand what just happened”.

grey-area 2y ago
The way I normally approach this is one big pr for context and then break it into lots of small ones for review.
- F-W-M 2y ago
  A PR with self-contained smaller commits would be possible as well.
  - t-writescode 2y ago
    Yes, though it does depend on how good the commenting system is; and, for something like that, you're still probably going to want a meeting to walk people through such a huge change. And you'd better hope you're not squashing that monstrous thing when you're done.
- jaredsohn 2y ago
  I've found processes like this to work better, too. Basically, the one big pr is like building a prototype to throw away. And the benefit is it has to get thrown away because the PR will never pass review.
deleted 2y ago
[deleted]
withinboredom 2y ago
so, it's not just a refactoring then; it's also bug fixes + refactoring. In my experience, those are the worst PRs to review. Either just fix the bugs, or just refactor it. Don't do both because now I have to spend more time checking the bugs you claim to fix AND your refactoring for new bugs.
- rcxdude 2y ago
  There are certainly classes of bugs for which refactoring is the path of lowest resistance
  - t-writescode 2y ago
    And there are multi-PR processes that can be followed to most successfully convert those changes in a comprehensible way. It'll often include extra scaffolding and / or extra classes and then renaming those classes to match the old classes' name after you're done, to reduce future cognitive load.
    - rcxdude 2y ago
      I'm unconvinced that adding extra code churn in order to split up a refactor that fixes bugs into a bugfix and a refactor is worthwhile
      - withinboredom 2y ago
        One metric I like to give my team is to have any new PR start a review in less than 15 minutes and be completed within 15 minutes. So, the longest you should wait is about 30 minutes for a review. That means teams either go "fuck it" and rubber stamp massive PRs -- which is a whole different issue -- or they take it seriously and keep PRs small to get their PRs reviewed in less than 30 minutes. In most cases where I see responses like this, they're not surprised to wait hours or days for a PR review. In that case, it makes sense to go big, otherwise you'll never get anything done. If you only have to wait half an hour, max, for a PR review; the extra code churn is 1000% worth it.
justatdotin 2y ago
> only by stepping through it with real customer data that you realized with the right inputs (not documented) it could do a third thing (not documented) that had very important “side effects” and was a no-op in the original code flow sounds like the 'nightmare' was already there, not in the refactor. First step should be some tests to confirm the undocumented behaviour. Some of your complaints seem to be about peer review ('approval'). I found my work life improved a lot once I embraced async review as a feature, not a bug. As for 'break it up step by step' - I know how much I appreciate reviewing a feature that is well presented in this way, and so I've got good at rearranging my work (when necessary) to facilitate smooth reviews.
- lazyasciiart 2y ago
  > sounds like the 'nightmare' was already there, not in the refactor I admit that I am pretty allergic to people who avoid working with imperfect code.

grey-area 2y ago

The way I normally approach this is one big pr for context and then break it into lots of small ones for review.

F-W-M 2y ago
A PR with self-contained smaller commits would be possible as well.
- t-writescode 2y ago
  Yes, though it does depend on how good the commenting system is; and, for something like that, you're still probably going to want a meeting to walk people through such a huge change. And you'd better hope you're not squashing that monstrous thing when you're done.
jaredsohn 2y ago
I've found processes like this to work better, too. Basically, the one big pr is like building a prototype to throw away. And the benefit is it has to get thrown away because the PR will never pass review.

deleted 2y ago

[deleted]

withinboredom 2y ago

so, it's not just a refactoring then; it's also bug fixes + refactoring. In my experience, those are the worst PRs to review. Either just fix the bugs, or just refactor it. Don't do both because now I have to spend more time checking the bugs you claim to fix AND your refactoring for new bugs.

rcxdude 2y ago
There are certainly classes of bugs for which refactoring is the path of lowest resistance
- t-writescode 2y ago
  And there are multi-PR processes that can be followed to most successfully convert those changes in a comprehensible way. It'll often include extra scaffolding and / or extra classes and then renaming those classes to match the old classes' name after you're done, to reduce future cognitive load.
  - rcxdude 2y ago
    I'm unconvinced that adding extra code churn in order to split up a refactor that fixes bugs into a bugfix and a refactor is worthwhile
    - withinboredom 2y ago
      One metric I like to give my team is to have any new PR start a review in less than 15 minutes and be completed within 15 minutes. So, the longest you should wait is about 30 minutes for a review. That means teams either go "fuck it" and rubber stamp massive PRs -- which is a whole different issue -- or they take it seriously and keep PRs small to get their PRs reviewed in less than 30 minutes. In most cases where I see responses like this, they're not surprised to wait hours or days for a PR review. In that case, it makes sense to go big, otherwise you'll never get anything done. If you only have to wait half an hour, max, for a PR review; the extra code churn is 1000% worth it.

justatdotin 2y ago

> only by stepping through it with real customer data that you realized with the right inputs (not documented) it could do a third thing (not documented) that had very important “side effects” and was a no-op in the original code flow sounds like the 'nightmare' was already there, not in the refactor. First step should be some tests to confirm the undocumented behaviour. Some of your complaints seem to be about peer review ('approval'). I found my work life improved a lot once I embraced async review as a feature, not a bug. As for 'break it up step by step' - I know how much I appreciate reviewing a feature that is well presented in this way, and so I've got good at rearranging my work (when necessary) to facilitate smooth reviews.

lazyasciiart 2y ago
> sounds like the 'nightmare' was already there, not in the refactor I admit that I am pretty allergic to people who avoid working with imperfect code.