Refactoring

​

Overview

​

Structure changes and behavior changes

// Fetch order history and shape it for display
export const useOrderHistory = () => {
  const { data } = useOrders();

  // Hide cancelled orders and sort newest first
  const orders = (data ?? [])
    .filter((order) => order.status !== "cancelled")
    .sort((a, b) => compareDesc(a.orderedAt, b.orderedAt));

  return { orders };
};

const selectOrders = (data: Order[] | undefined) =>
  (data ?? [])
    .filter((order) => order.status !== "cancelled")
    .sort((a, b) => compareDesc(a.orderedAt, b.orderedAt));

export const useOrderHistory = () => {
  const { data } = useOrders();
  return { orders: selectOrders(data) };
};

export const useOrderHistory = () => {
  const { data } = useOrders();

  // Drop the filter and keep only the newest-first sort
  const orders = (data ?? [])
    .sort((a, b) => compareDesc(a.orderedAt, b.orderedAt));

  return { orders };
};

​

Do not mix the two in one pull request

const selectOrders = (data: Order[] | undefined) =>
  (data ?? [])
    .filter((order) => order.status !== "cancelled")
    .sort((a, b) => compareDesc(a.orderedAt, b.orderedAt));

export const useOrderHistory = () => {
  const { data } = useOrders();
  // Calling the same logic as before keeps the list identical
  return { orders: selectOrders(data) };
};

// The tests are untouched — passing unchanged is the evidence
// that the observable behavior is preserved
test("returns order history, newest first", () => {
  const { result } = renderHook(() => useOrderHistory());

  // cancelled orders are excluded from the list
  expect(result.current.orders).toEqual([delivered2, delivered1]);
});

const selectOrders = (data: Order[] | undefined) =>
  (data ?? [])
    // Alongside the extraction, the filter is dropped so
    // cancelled orders are now shown
    .sort((a, b) => compareDesc(a.orderedAt, b.orderedAt));

export const useOrderHistory = () => {
  const { data } = useOrders();
  return { orders: selectOrders(data) };
};

  test("returns order history, newest first", () => {
    const { result } = renderHook(() => useOrderHistory());

-   // cancelled orders are excluded from the list
-   expect(result.current.orders).toEqual([delivered2, delivered1]);
+   // cancelled orders now appear in the list
+   expect(result.current.orders).toEqual([delivered2, cancelled1, delivered1]);
  });

​

The timing is a choice, not a rule

• Structure first — when the current structure makes the coming behavior change awkward, reshape the code first. The behavior change that follows becomes smaller and easier to review.
• Structure after — when the behavior change is urgent or the structural problem only becomes visible while working, ship the behavior first and clean up immediately after, leaving the area ready for the next change.
• Not now — when the code is rarely touched, the cost of restructuring may never pay back. Leaving it alone is a legitimate decision.

​

Tests define “behavior unchanged”

• A test had to change. Either the change was not actually structure-only, or the test asserts implementation details rather than observable behavior. Both cases need fixing — the first in the diff, the second in the test. See Test Code.
• No test failed while the logic was temporarily broken. If an intermediate mistake produced no failing test, the safety net has a hole in the area the refactoring touches.

​

Small, reversible steps

// The existing function is untouched
export const formatPrice = (price: number) =>
  `¥${price.toLocaleString()}`;

// Add the new function; nothing uses it yet
export const formatMoney = ({ amount, currency }: Money) =>
  `${amount.toLocaleString()} ${currency}`;

- {formatPrice(order.total)}
+ {formatMoney({ amount: order.total, currency: order.currency })}

- export const formatPrice = (price: number) =>
-   `¥${price.toLocaleString()}`;

​

Designing for deletion

• Keep dependencies explicit and one-directional. The question “what depends on this?” must have a mechanical answer — from types, imports, or module boundaries. Code whose dependents cannot be listed cannot be deleted with confidence.
• Group code by concept, not by kind. When everything a feature needs lives in one subtree, deleting the feature is deleting the subtree. When it is scattered across shared layers, every deletion is an investigation. Places grouped only by kind — utils, common — are the typical form of logical or coincidental cohesion.
• Treat shared abstractions as deletion risks. Every new caller of a shared helper makes that helper harder to remove or change. Extract shared code when duplication has proven itself a burden, not in anticipation.
• Delete dead code instead of keeping it. Commented-out blocks, unused exports, and flags that never flip are not free: they mislead readers and dilute change detection. Version control already preserves everything — restoring deleted code is one revert away.

# ❌ Grouped by kind: removing order history means
# investigating every layer
src/
  components/OrderList.tsx
  hooks/useOrderHistory.ts
  utils/orderFormatters.ts

# ✅ Grouped by concept: removing order history is
# deleting one subtree
src/
  features/order-history/
    OrderList.tsx
    useOrderHistory.ts
    formatters.ts

​

Related pages