The High-Risk Refactoring
February 13, 2024
Code refactoring may cost a fortune if not done right. A dysfunctional revamped system or new features coupled with incorrect rewrite is, with no doubt, damaging. One can argue to what extent.
Every Refactoring Hides Risks #
Code improvements are risky as they aim to change a working system. That's true. However, in many situations, developers can mitigate the risk via different activities and make it bearable.
Some refactoring tasks are larger and affect many subsystems. Others, in contrast, are constrained within a single component but may unforeseeably affect other parts of the whole system and cause breakdown in vital business operations. E.g. an existing purchasing flow of a core product. A third category is for improvements, "making room" for new features—e.g., changing a single product purchase flow to support more items and adding a new one after.
The commonality between all three cases is that they imply high risk. (1) If done wrong, such an improvement will hurt the business (revenue loss, customer attrition), the team (trust, motivation), or any related features (development blocked). (2) On the other hand, executing it is costly as it requires extra care, effort, and time. Experienced developers with domain knowledge are also preferable for that kind of tasks.
Addressing Risk (Checklist): #
✅ Define constraints. How far should I go.
✅ Isolate improvements from features. Do not apply them simultaneously.
✅ Write extensive tests. Higher level (integration) with fewer implementation details. They should run alongside changes.
✅ Have a visual confirmation. Open the browser.
❌ Do not skip tests. Don't be lazy.
❌ Do not rely too much on code reviews and QA. Humans make mistakes.
❌ Do not mix expensive cleanups with other changes. But do that for small improvements.
Sometimes is obvious, what improvements are required and where in the codebase. Moving some code out of a component or additional cleanup so new features are not walking on broken glass. For a developer, this may feel like something to address sooner than later. But the challenging refactoring (especially), hides pitfalls—verifying the change is not straightforward due to dev env impediments, dependencies, DB/APIs, flaky tests, or lack of time. Things will also break often throughout the improvement process. How does one catch the failures early?
It's tempting to simply start on the rewrite, but...
✋ Hold On #
- Evaluate how risky (expensive) the change is upfront—from a dev and business perspective.
What will happen if buggy code gets shipped? Broken business goals, user churn, blaming, or loss of trust are serious issues to consider. Thinking about the possible negative outcome, is a good first stopper. Maybe don't refactor in the first place.
- Consider if the improvement is a standalone task or part of another feature.
Fx. a new feature implementation reveals an obvious candidate for system refactoring where the feature itself will benefit directly. In this case, it's practical to do-it-now while being in the context. Usually “Later” never comes.
But. Separating the cleanup from the main feature work—so they can be reviewed and QA-ed individually—is a good approach in general. Especially if the improvement is expected to cause side effects or is difficult to verify.
- Prove the system is working beforehand.
Before starting any work, it's crucial to validate that all related parts are functioning. Having all the app tests pass is not enough. These tests cover specific user flows, but may miss others, so the mission here is to fill the gaps with appropriate tests.
🏋️ This Refactoring is a Big Deal #
As such it's highly important to ensure the system works the same way after the swap with the new code. In that regard, immediately spotting when something breaks throughout the whole refactoring process is very helpful. No one wants to find that out in production. Therefore. Write comprehensive tests. This should be a core activity from beginning to end.
Use breadcrumbs in the form of tests:
- unit and high-level integration.
- no implementation details.
- test as much as possible to build confidence (aka test everything).
Integration tests are very practical here and can catch leaking component side effects.
// React component test.
it("should show all addons", () => {
// Mocking the server response.
// Util that mocks to the lowest possible level—window.fetch()
server.get(endpoints.loadAddonsData, { addons: ["addon1", "addon2"] });
// A prop controlling the component's button.
props.shouldShowMoreAddons = true;
// Render.
render(<Addons {...props} />);
// Clicking the button to show all addons.
fireEvent.click(screen.getByRole("button"));
// Assert the addons list is shown.
await waitFor(() => {
expect(screen.queryByRole("list")).toBeInTheDocument();
});
})
Once everything is green ✅, hand it over to QA for next-level verification. We are human.
🪡 Refactoring Coupled with a Feature #
If time is pressing, release the feature separately and handle the refactoring later. It will require QA to retest parts of the feature, but this is better than releasing too many things, at once.
Evaluate and choose not to refactor alongside new code if that hides too many unknowns, costs, or increased risks that can't be afforded. Otherwise, ensure the system is working on the I/O. Little improvements are just fine.
{} Examples (Code) #
The example below illustrates a Dashboard React component that renders some widgets and an upsell box (<UpsellBox1 />) which data dependents on a backend call (loadUpsell1Data()).
// The example is greatly simplified.
export function Dashboard({ data }) {
// Other business logic goes here
const [shouldShowUpsell1, setShouldShowUpsell1] = useState(false);
const [upsell1Data, setUpsell1Data] = useState(null);
// Other business logic goes here
// and here
// and here
// ...
useEffect(() => {
// Complex logic to determine if UpsellBox1 should show.
if (condition1 && condition2 && !condition3) {
setShouldShowUpsell1(true);
loadUpsell1Data().then((bannerData) => {
setUpsell1Data(bannerData);
});
}
}, [...]);
return (
<div>
<Widget1 />
<Widget2 />
...
{shouldShowUpsell1 && <UpsellBox1 data={upsell1Data} />}
</div>
);
}
Imagine the new task is to render a second upsell (<UpsellBox2 />), and if both are present, put them in a carousel.
return (
<div>
<Widget1 />
<Widget2 />
...
<!-- Display both upsell boxes -->
{shouldShowUpsell1 && shouldShowUpsell2 && (
<UpsellSlider>
<UpsellBox1 data={upsell1Data} />
<UpsellBox2 data={upsell2Data} />
</UpsellSlider>
)}
<!-- OR show only <UpsellBox1 /> -->
{shouldShowUpsell1 && <UpsellBox1 data={upsell1Data} />}
<!-- OR <UpsellBox2 /> -->
{shouldShowUpsell2 && <UpsellBox2 data={upsell2Data} />}
</div>
);
Since there are two upsells (probably more in future), the logic in <Dashboard /> is growing. It makes sense to move the upsell code out—in a custom hook useUpsellData (not in <UpsellSlider />) as to not sacrifice explicitness—but this is tricky as widgets and upsells data mix together.
Here is how the component may look like after the change:
// The example is greatly simplified.
export function Dashboard({ data }) {
// ✅ Tests prior refactoring
// ❓ The logic sets "conditions" which is needed in useUpsellData
// Other business logic goes here
// ✅ New tests
// ❓ To ensure existing code doesn't break
// ❓ Proving the new code works
// 🧨 High risk—can break both upsells causing a heavy load on support and missed sales.
// The hook-specific tests will overlap with the
// Dashboard integration ones. That's OK.
const {
upsell1: { shouldShowUpsell1, upsell1Data },
upsell2: { shouldShowUpsell2, upsell2Data },
} = useUpsellData(conditions);
// ✅ Tests prior refactoring especially if the logic
// below is twisted with the upsells
// ❓ To ensure existing code doesn't break
// 🧨 High risk—can break important features that are hard to track.
// Other business logic goes here
// and here
// and here
// ...
// ✅ This useEffect is gone now, but its implementation
// should be tested as part of useUpsellData() 👆
// ❓ To ensure existing code doesn't break
// 🧨 High risk—can break the first upsell leading to no purchases.
// useEffect(() => {
// if (condition1 && condition2 && !condition3) {
// setShouldShowUpsell1(true);
//
// loadUpsell1Data().then((bannerData) => {
// setUpsell1Data(bannerData);
// });
// }
// }, [...]);
return (
<div>
<!--
✅ Tests prior refactoring for the widgets
❓ To ensure existing code doesn't break
🧨 High|Medium risk—can break something important.
-->
<Widget1 />
<Widget2 />
...
<!--
✅ New tests for the carousel with two slides
❓ To verify the change
🧨 High|Medium risk—can break existing functionality.
-->
{shouldShowUpsell1 && shouldShowUpsell2 && (
<UpsellSlider>
<UpsellBox1 data={upsell1Data} />
<UpsellBox2 data={upsell2Data} />
</UpsellSlider>
)}
<!--
✅ Tests prior refactoring for this specific case
❓ To ensure existing code doesn't break
🧨 High|Medium risk—can break existing functionality.
-->
{shouldShowUpsell1 && <UpsellBox1 data={upsell1Data} />}
<!--
✅ New tests
❓ To verify the change
🧨 Medium risk.
-->
{shouldShowUpsell2 && <UpsellBox2 data={upsell2Data} />}
</div>
);
}
The purpose with these examples is to illustrate how shaky refactoring can be if combined with new features within a busy component. The challenges mainly arise from the tightly coupled nature of <Dashboard />'s business logic that handles many widgets and sections as its direct children.
👍 Should I Refactor #
👍 Refactor if things are getting too complicated, but 👎 stop if can't prove it works.
👍 Accompany new features with refactoring for areas you foresee to be subject to a change, but 👎 copy-pasting is ok until patterns arise.
👍 Be proactive in finding new ways to ensure refactoring predictability, but 👎 be conservative about the assumption QA will find all the bugs.
👍 Move business logic out of busy components, but 👎 be brave enough to keep the legacy code intact if the only argument is "this code looks wrong".