4 ms·

There is a good article approaching it from the other side here: https://mtlynch.io/code-review-love/ https://mtlynch.io/code-review-love/ - I am not the author

There is a good article approaching it from the other side here: https://mtlynch.io/code-review-love/ https://mtlynch.io/code-review-love/ - I am not the author, but it's one of the few things I have bookmarked.

digging 3y ago
I find this much more valuable than the main post, which is entertaining but leans too hard into absurdity to be a useful guide for what actual problematic behavior looks like.
jlokier 3y ago
That was a fun read. I found item 6 interesting, "Separate functional and non-functional changes". Because that's obviously the sensible and useful thing to do. But at the last job, whitespace, reformatting and simple refactoring changes were strongly discouraged unless they were part of another functional change. There was a written policy about it. Refactoring changes and such didn't have an approved Jira ticket, and you needed a ticket for every PR (even proposed PRs to show an idea). If you created a Jira ticket for the code improvement task, it would nearly always be set to lowest priority ("never" in practice) and the Jira ticket approval committee would rarely approve it for work in the next sprint unless it was accompanied by compelling business case backed by an enthusastic champion. A PM would never assign them, except as an onboarding practice task for someone new. You could self-assign them but you'd be taking a risk by working on something lowest priority. In practice this meant people squeezed whitespace, simple refactorings and other improvements by comingling them into feature and bugfix changes. (Larger refactorings such as internal API and architectural changes, which that code sorely needed for ridiculously-bug-prone reasons and ridiculously-slow-development reasons, rarely got done at all.)
- throw1234651234 3y ago
  We have had similar issues. At the end of the day, it's just a bad PO issue imo. And yea, in those situations, it very much becomes a "refactor-as-you-go" type of deal, but we do try to keep those changes very small and clear so it doesn't interfere with the main logic. It actually gives me a bad taste to even type it, because not isolating formatting/clean up really does lead to bugs in the long run.

digging 3y ago

I find this much more valuable than the main post, which is entertaining but leans too hard into absurdity to be a useful guide for what actual problematic behavior looks like.

jlokier 3y ago

That was a fun read. I found item 6 interesting, "Separate functional and non-functional changes". Because that's obviously the sensible and useful thing to do. But at the last job, whitespace, reformatting and simple refactoring changes were strongly discouraged unless they were part of another functional change. There was a written policy about it. Refactoring changes and such didn't have an approved Jira ticket, and you needed a ticket for every PR (even proposed PRs to show an idea). If you created a Jira ticket for the code improvement task, it would nearly always be set to lowest priority ("never" in practice) and the Jira ticket approval committee would rarely approve it for work in the next sprint unless it was accompanied by compelling business case backed by an enthusastic champion. A PM would never assign them, except as an onboarding practice task for someone new. You could self-assign them but you'd be taking a risk by working on something lowest priority. In practice this meant people squeezed whitespace, simple refactorings and other improvements by comingling them into feature and bugfix changes. (Larger refactorings such as internal API and architectural changes, which that code sorely needed for ridiculously-bug-prone reasons and ridiculously-slow-development reasons, rarely got done at all.)

throw1234651234 3y ago
We have had similar issues. At the end of the day, it's just a bad PO issue imo. And yea, in those situations, it very much becomes a "refactor-as-you-go" type of deal, but we do try to keep those changes very small and clear so it doesn't interfere with the main logic. It actually gives me a bad taste to even type it, because not isolating formatting/clean up really does lead to bugs in the long run.