6 ms·

The High-Risk Refactoring

71 points by ben_s 3y ago

jimbokun 3y ago
One of the subtler but quite dangerous risks of refactoring, is making the output more “correct” but different from what it was. Clients of the application likely have adapted to the less correct output in ways that will break in unanticipated ways if it changes. I’ve seen this lead to production issues, and the explanation that the new behavior was more correct did not go over well.
Ferret7446 3y ago
Refactoring code is the same as refactoring a math equation. Make sure you show every step so you don't screw up. If you skip steps you will screw it up. If it's risky, you're doing it wrong. (Or you're using an unchecked language like Python (which is incidentally why Python is not suited for large projects IMO, because you can't safely refactor them).) P.S. the steps are covered in Martin Fowler's book, but they are equivalent to refactoring functions in math, naturally.
- computerfriend 3y ago
  I think you mean "untyped", which is only as true as you make it. Type annotations have been mainstream for many years now.
Scubabear68 3y ago
I am surprised at the tone here as well as that if the article. If you view refactoring as this risky, you are I imagine using a dynamically typed language with automated tests few and far between. In a statically typed language and with a decent automated test suite, refactoring is just another coding activity. It sounds like people out there are terrified of going near their code with a 10’ pole. That is heart breaking to see.
- mlhpdx 3y ago
  Maybe don’t assume other people are “wrong”? A strongly typed codebase in a larger system almost certainly has untyped boundaries (HTTP, JSON, SQL, etc.) and likely to data driven with combinatorial complexity. Going into such rewrites with a great deal of respect for any existing code that “works” is very wise.
  - Scubabear68 3y ago
    I guess we should all stop fixing bugs and creating new feature and enhancements. My God - if we touch the code, something might break! Sorry for the sarcasm, but this attitude of fear is so pervasive and unnecessary. Seriously, isn’t this our job?
    - mlhpdx 3y ago
      Respect, not fear. I chose the word intentionally. Respecting the working system means investing a little before going in to redo. That’s all. No everyone is right for such work, which is fine because they are often well suited for other equally important things.
sweezyjeezy 3y ago
One approach I've made work a couple of times is to pull all of the data going in and out of a system for a time period, and rerun it after the refactor. This gave me a lot more confidence than unit tests.
wruza 3y ago
Just a reminder that you won't have to refactor if you don't factor it too much in the first place. It ought to be easier to conceive, understand, program, and maintain. If you have to refactor, you probably have to defactor instead.
- dimgl 3y ago
  I like this idea. What does it mean though? Does it mean proper enforcement of isolation of concerns and increased modularity?
gopal_virtual 3y ago
I start with questions like - - What are the engineering and business benefits of refactoring the current code base? - What is the cost of not refactoring code base? Does it affect user experience, performance of the app? - Does it significantly improves onboarding time for new engineers? - Does it enable the team to incorporate future updates or feature development? etc. Any engineering tech-debt has to be justified with outcomes, and needs to be weighed against current priorities. Else it will be a waste of critical resource.
liampulles 3y ago
Good reminder to try and improve things before one gets to the point where a large refactor is needed. Not always avoidable of course.
simonw 3y ago
A friend once told me that any time he takes on a big refactoring project at work he works with the assumption that the project could get cancelled at any moment, so his goal is to ensure that if it DOES get cancelled he'll still be leaving the code in a better place than it was when he started. I think this is a really smart strategy.
- Fire-Dragon-DoL 3y ago
  Your friend is doing an actual refactoring (kudos), according to the definition. Otherwise it's more like restructuring
- bigkm 3y ago
  how does that assumption work? I would have thought that assumption would lead one to care less.
  - Jtsummers 3y ago
    It works if simonw means the refactoring/rearchitecting/redesign project and not the broader encompassing project. In the former case, it means making sure your changes are mergeable (ideally merged) all along the way so that if that effort is canceled the broader project can still benefit from it. In the latter case, then you're correct it doesn't matter. If the encompassing project is canceled who cares what state the change effort is in since everything is getting tossed.
- seadan83 3y ago
  That strategy is something the heart of "agile development." Ensure you can do a little bit and then walk away and work on something else while still having delivered some value. A crux though for "big refactoring project" - first, that is kinda misnomer. It's not refactoring, it's re-architecture. Second, a number of refactorings can make things worse before they are better. For example, fixing bad abstractions, sometimes the first step is to inline stuff and copy/paste the abstractions to first "flatten the code" before restructuring it. That re-flattening step can lead to worse code, but it's in a place where it can be then made better (one step back to enable three steps forward)
sinuhe69 3y ago
For me, refactoring means simple, no-risk code re-organization only. It should be executed often during the production and it should serve foremost code-reuse and better readability. Anything going further than that is for me rewriting and should be considered appropriately.
- ck45 3y ago
  You are absolutely right, see the definition on https://refactoring.com/ https://refactoring.com/ Unfortunately, it has become common to refer to any kind of code changes as refactorings (not backed by any proof, just an observation)
- begueradj 3y ago
  Indeed. Re-writing is often dangerous: https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/ https://www.joelonsoftware.com/2000/04/06/things-you-should-...
IshKebab 3y ago
I have yet to work anywhere where people refactor anywhere near as much as they should, so I think this is really sending the wrong message. People massively overestimate the risks of refactoring and massively underestimate the risks of not refactoring - which many people incorrectly assume don't exist.
- dimgl 3y ago
  This is my viewpoint too, but I'm always wary of cargo cult refactors. I've seen multiple cases where a refactor moves the needle laterally (or even down). I also feel similarly about rewrites. I think people are way too scared of them, but unfortunately I've also seen and heard of cases of bad rewrites. This makes it much harder to convince someone that a rewrite is worth it. In essence, both refactors and rewrites can have great outcomes if the reasoning for it is sound.
mjr00 3y ago
Refactoring and related maintenance activities, like library/framework upgrades, are high risk and low immediate reward. In my experience, they're also one of the hardest overall software development tasks; large-scale refactors will end up touching a large percentage of existing code, and will require a ton of reading/understanding existing code to figure out what it does, as well as the domain knowledge to know what it should do. Often during refactoring I've found hairy bits of code which seem like they'll be intractable, until I stepped up a level and realized it's for a half-implemented feature that never got released, and can be completely removed. If you try to measure it on an incremental level, refactoring is never worth it. Your code changes can and will cause unexpected bugs; customers get no perceivable value; it's engineering time not spent on revenue-generating features, and per the previous paragraph, you should be getting your best engineers doing large refactors. But if you look at it across a longer horizon, it makes a massive difference in the maintainability of your codebase. It's like cutting out 300 calories/day from your diet; you won't notice a difference if you weigh yourself on two consecutive days, but if you do it for 6 months it has a noticeable impact.
- BurningFrog 3y ago
  The way to make refactoring safe is to have a solid test suite. With a good enough test suite, if all tests still pass, you can be pretty confident. To me, making refactoring easier/safer is probably the biggest benefit of having tests!
  - zenogantner 3y ago
    I'd say making adding new features that deliver value safer is an even bigger benefit of having tests.
keybored 3y ago
What I did last time for a moderate refactoring was to do everything step by step in the commits. Intermediate broken state was fine. Them purposefully commit in such a way that the tools could verify that I did the intended change. Like line moves: verify with `git diff --color-moved`. Then I told the reviewers about my breadcrumbs. Then finally for the merge I could rewrite the history. Ideally I want to not just post a 500-line diff as a pull request. Ideally I want to really write a program/script which does as much of the changes as I did (intermediate commits for the changes that need to be done manually). Refactorings are often done through and IDE so hopefully the IDE can output a script of the refactoring steps. Then the reviewers don’t even have to look at the diff (really): they can look at the script, see if it is reasonable, then run it themselves and verify that it produces the same output (the same tree). Coccinelle is a tool for C (and C++?) which lets you write “semantic patches” like “replace these boolean expressions with this one”. Maybe replace some considered-bad C standard library calls with what you consider to be better. Then you can leave those patches in and check that people don’t check in new code with the old pattern. All of this is just for refactoring though. Once you start talking about rewriting I feel like you have moved beyond that point.

71 points by ben_s 3y ago

One of the subtler but quite dangerous risks of refactoring, is making the output more “correct” but different from what it was. Clients of the application likely have adapted to the less correct output in ways that will break in unanticipated ways if it changes. I’ve seen this lead to production issues, and the explanation that the new behavior was more correct did not go over well.

Ferret7446 3y ago

Refactoring code is the same as refactoring a math equation. Make sure you show every step so you don't screw up. If you skip steps you will screw it up. If it's risky, you're doing it wrong. (Or you're using an unchecked language like Python (which is incidentally why Python is not suited for large projects IMO, because you can't safely refactor them).) P.S. the steps are covered in Martin Fowler's book, but they are equivalent to refactoring functions in math, naturally.

computerfriend 3y ago
I think you mean "untyped", which is only as true as you make it. Type annotations have been mainstream for many years now.

Scubabear68 3y ago

I am surprised at the tone here as well as that if the article. If you view refactoring as this risky, you are I imagine using a dynamically typed language with automated tests few and far between. In a statically typed language and with a decent automated test suite, refactoring is just another coding activity. It sounds like people out there are terrified of going near their code with a 10’ pole. That is heart breaking to see.

mlhpdx 3y ago
Maybe don’t assume other people are “wrong”? A strongly typed codebase in a larger system almost certainly has untyped boundaries (HTTP, JSON, SQL, etc.) and likely to data driven with combinatorial complexity. Going into such rewrites with a great deal of respect for any existing code that “works” is very wise.
- Scubabear68 3y ago
  I guess we should all stop fixing bugs and creating new feature and enhancements. My God - if we touch the code, something might break! Sorry for the sarcasm, but this attitude of fear is so pervasive and unnecessary. Seriously, isn’t this our job?
  - mlhpdx 3y ago
    Respect, not fear. I chose the word intentionally. Respecting the working system means investing a little before going in to redo. That’s all. No everyone is right for such work, which is fine because they are often well suited for other equally important things.

sweezyjeezy 3y ago

One approach I've made work a couple of times is to pull all of the data going in and out of a system for a time period, and rerun it after the refactor. This gave me a lot more confidence than unit tests.

wruza 3y ago

Just a reminder that you won't have to refactor if you don't factor it too much in the first place. It ought to be easier to conceive, understand, program, and maintain. If you have to refactor, you probably have to defactor instead.

dimgl 3y ago
I like this idea. What does it mean though? Does it mean proper enforcement of isolation of concerns and increased modularity?

gopal_virtual 3y ago

I start with questions like - - What are the engineering and business benefits of refactoring the current code base? - What is the cost of not refactoring code base? Does it affect user experience, performance of the app? - Does it significantly improves onboarding time for new engineers? - Does it enable the team to incorporate future updates or feature development? etc. Any engineering tech-debt has to be justified with outcomes, and needs to be weighed against current priorities. Else it will be a waste of critical resource.

liampulles 3y ago

Good reminder to try and improve things before one gets to the point where a large refactor is needed. Not always avoidable of course.

simonw 3y ago

A friend once told me that any time he takes on a big refactoring project at work he works with the assumption that the project could get cancelled at any moment, so his goal is to ensure that if it DOES get cancelled he'll still be leaving the code in a better place than it was when he started. I think this is a really smart strategy.

Fire-Dragon-DoL 3y ago
Your friend is doing an actual refactoring (kudos), according to the definition. Otherwise it's more like restructuring
bigkm 3y ago
how does that assumption work? I would have thought that assumption would lead one to care less.
- Jtsummers 3y ago
  It works if simonw means the refactoring/rearchitecting/redesign project and not the broader encompassing project. In the former case, it means making sure your changes are mergeable (ideally merged) all along the way so that if that effort is canceled the broader project can still benefit from it. In the latter case, then you're correct it doesn't matter. If the encompassing project is canceled who cares what state the change effort is in since everything is getting tossed.
seadan83 3y ago
That strategy is something the heart of "agile development." Ensure you can do a little bit and then walk away and work on something else while still having delivered some value. A crux though for "big refactoring project" - first, that is kinda misnomer. It's not refactoring, it's re-architecture. Second, a number of refactorings can make things worse before they are better. For example, fixing bad abstractions, sometimes the first step is to inline stuff and copy/paste the abstractions to first "flatten the code" before restructuring it. That re-flattening step can lead to worse code, but it's in a place where it can be then made better (one step back to enable three steps forward)

sinuhe69 3y ago

For me, refactoring means simple, no-risk code re-organization only. It should be executed often during the production and it should serve foremost code-reuse and better readability. Anything going further than that is for me rewriting and should be considered appropriately.

ck45 3y ago
You are absolutely right, see the definition on https://refactoring.com/ https://refactoring.com/ Unfortunately, it has become common to refer to any kind of code changes as refactorings (not backed by any proof, just an observation)
begueradj 3y ago
Indeed. Re-writing is often dangerous: https://www.joelonsoftware.com/2000/04/06/things-you-should-never-do-part-i/ https://www.joelonsoftware.com/2000/04/06/things-you-should-...

IshKebab 3y ago

I have yet to work anywhere where people refactor anywhere near as much as they should, so I think this is really sending the wrong message. People massively overestimate the risks of refactoring and massively underestimate the risks of not refactoring - which many people incorrectly assume don't exist.

dimgl 3y ago
This is my viewpoint too, but I'm always wary of cargo cult refactors. I've seen multiple cases where a refactor moves the needle laterally (or even down). I also feel similarly about rewrites. I think people are way too scared of them, but unfortunately I've also seen and heard of cases of bad rewrites. This makes it much harder to convince someone that a rewrite is worth it. In essence, both refactors and rewrites can have great outcomes if the reasoning for it is sound.

mjr00 3y ago

Refactoring and related maintenance activities, like library/framework upgrades, are high risk and low immediate reward. In my experience, they're also one of the hardest overall software development tasks; large-scale refactors will end up touching a large percentage of existing code, and will require a ton of reading/understanding existing code to figure out what it does, as well as the domain knowledge to know what it should do. Often during refactoring I've found hairy bits of code which seem like they'll be intractable, until I stepped up a level and realized it's for a half-implemented feature that never got released, and can be completely removed. If you try to measure it on an incremental level, refactoring is never worth it. Your code changes can and will cause unexpected bugs; customers get no perceivable value; it's engineering time not spent on revenue-generating features, and per the previous paragraph, you should be getting your best engineers doing large refactors. But if you look at it across a longer horizon, it makes a massive difference in the maintainability of your codebase. It's like cutting out 300 calories/day from your diet; you won't notice a difference if you weigh yourself on two consecutive days, but if you do it for 6 months it has a noticeable impact.

BurningFrog 3y ago
The way to make refactoring safe is to have a solid test suite. With a good enough test suite, if all tests still pass, you can be pretty confident. To me, making refactoring easier/safer is probably the biggest benefit of having tests!
- zenogantner 3y ago
  I'd say making adding new features that deliver value safer is an even bigger benefit of having tests.

keybored 3y ago

What I did last time for a moderate refactoring was to do everything step by step in the commits. Intermediate broken state was fine. Them purposefully commit in such a way that the tools could verify that I did the intended change. Like line moves: verify with `git diff --color-moved`. Then I told the reviewers about my breadcrumbs. Then finally for the merge I could rewrite the history. Ideally I want to not just post a 500-line diff as a pull request. Ideally I want to really write a program/script which does as much of the changes as I did (intermediate commits for the changes that need to be done manually). Refactorings are often done through and IDE so hopefully the IDE can output a script of the refactoring steps. Then the reviewers don’t even have to look at the diff (really): they can look at the script, see if it is reasonable, then run it themselves and verify that it produces the same output (the same tree). Coccinelle is a tool for C (and C++?) which lets you write “semantic patches” like “replace these boolean expressions with this one”. Maybe replace some considered-bad C standard library calls with what you consider to be better. Then you can leave those patches in and check that people don’t check in new code with the old pattern. All of this is just for refactoring though. Once you start talking about rewriting I feel like you have moved beyond that point.