https://webup.org/blog/the-high-risk-refactoring/

-
Miroslav Nikolov

Miroslav Nikolov

The High-Risk Refactoring

February 13, 2024

The challenging code refactoring by AI

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 #

 1. 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.

 2. 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.

 3. 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".

---------------------------------------------------------------------
Follow on Linkedin

about * rss * hi@webup.org