Safe refactoring

Safety checklist and atomic operations

Prefer IDE refactor tools over manual search-replace; use language servers or codemods for cross-file renames to avoid misses. Prefer several small PRs over a long-lived mega branch.

• Refactor PRs should not mix behavior changes or features unless explicitly split.
• Hot paths need before/after benchmarks or profiling when refactored.
• Database and API changes follow expand–contract—not “ship everything together.”

Pre-refactor safety checklist

Must verify before starting (do not refactor if any item fails):

Coverage gate:
  [ ] Current module unit coverage ≥ 70%
      (run: npm test -- --coverage --collectCoverageFrom='src/module/**')
  [ ] If coverage is insufficient: add characterization tests (black-box I/O) first, then refactor

Rollback capability:
  [ ] All changes on an isolated branch (git branch refactor/xxx)
  [ ] Atomic commits per step; any intermediate state is revertible via git revert
  [ ] If DB migration involved: confirm migration can roll back (run: make db-rollback)

Dependency scope:
  [ ] Count files that depend on this module
      (run: grep -r "from.*module-name" src/ | wc -l)
  [ ] Confirmed list of public API changes (function signatures, error codes)
  [ ] Check whether consumer services need simultaneous upgrade (notify via CODEOWNERS)

Four atomic refactoring operations and safety ratings

Operation    | Safety     | Commit granularity
-------------|------------|--------------------------------------------------
rename       | ★★★★★      | Change one symbol at a time; one commit per symbol
             |            | git commit -m "refactor: rename getUserData to fetchUserProfile"
extract      | ★★★★☆      | Extract first (keep original call), test green, then remove original
             |            | git commit -m "refactor: extract validateCoupon from checkout handler"
move         | ★★★☆☆      | Add re-export first to stabilize path, then update imports, then delete re-export
             |            | commit 1: "refactor: add re-export for backward compat"
             |            | commit 2: "refactor: update all imports to new path"
             |            | commit 3: "refactor: remove re-export after import migration"
inline       | ★★☆☆☆      | Confirm function has no side effects and is called from only one place
             |            | git commit -m "refactor: inline single-use helper formatDate"

Behavior-preserving verification: snapshot vs golden file

// Option 1: Jest/Vitest Snapshot tests (best for UI components and serialized output)
it('checkout summary renders correctly', () => {
  const { container } = render(<OrderSummary order={mockOrder} />);
  expect(container).toMatchSnapshot();  // generate snapshot before refactoring
});
// After refactoring: if snapshot changes, run jest --updateSnapshot and review the diff carefully

// Option 2: Golden file comparison (best for API responses, data transforms, report generation)
// golden-files/order-summary.json (generated before refactoring and committed to git)
{
  "orderId": "ORD-001",
  "total": 99.50,
  "items": [{"name": "Item A", "qty": 2, "price": 49.75}]
}

// Test code
import expectedOutput from './golden-files/order-summary.json';
it('transformOrder output matches golden file', () => {
  const result = transformOrder(rawApiResponse);
  expect(result).toEqual(expectedOutput);  // output must be identical before and after refactor
});

// When to use which:
// snapshot  → component trees, HTML output, object serialization (fast but diffs are hard to read)
// golden file → data transforms, reports, API responses (clearer diffs; better for code review)

Invariants and contracts

Invariants are statements that must remain true after structural edits. Write them as a checklist before coding; for agents, tie each item to code or tests—not vague “behavior unchanged.”

• Public API: signatures, error codes, idempotency; breaking changes need their own PR and release notes.
• Persistence: field meaning, serialization, migration reversibility; feature-flag readers and writers.
• Messages & integrations: topics, event schemas, gRPC/REST contracts; consumer gray readiness.
• Observability: key log fields, metric names, alert queries still work after refactor.

Mark unverifiable invariants (e.g. reliance on undocumented third-party behavior) as assumptions in the PR with a validation plan or human sign-off.

Test before you move

“Move” means files, module boundaries, dependency direction—easy to miss references. Order: behavior already covered by tests → change paths/imports → remove compatibility shims.

• Ideal: unit/integration tests cover current behavior; otherwise add the narrowest characterization or contract tests.
• Prefer stable symbol paths during moves: re-exports or adapters first, delete in a follow-up—don’t change ten imports in one blind step.
• Each “move-only” commit should stay green; don’t batch “move everything then test.”

Expand–contract (APIs and data)

For outward contracts or shared storage, use phased releases: expand (old and new coexist, backward compatible) then contract (remove old paths after callers are gone).

1. Expand: add fields/endpoints/columns with defaults old clients understand; legacy paths keep working.
2. Migrate: dual-write or backfill; monitor errors and consistency.
3. Switch: read path moves behind a flag; observe for a soak window.
4. Contract: retire old APIs/columns/dead code—this is where breaking commits are allowed.

Agents should state the current phase; avoid adding a new contract and deleting the old one in a single commit unless the team can atomically release.

Small-commit pipeline

Each step should be reviewable and revert-friendly; ideally each maps to a clear commit message (see the one-line helper below).

  [ List invariants + risk surface ]
                    │
                    ▼
            [ Add tests / characterization anchors ]
                    │
                    ▼
            [ Small edits: IDE refactor / single intent ]
                    │
                    ▼
         ┌──────────┴──────────┐
         ▼                    ▼
  [ Local: related tests + lint ]   [ Optional: types / format ]
         │                     │
         └──────────┬──────────┘
                    ▼
            [ Commit: single intent, revertible ]
                    │
                    ▼
            [ Push →CI green before next step ]

CI gate steps

Refactor branches should meet the same or stricter CI as feature work—no “green only on my laptop.” Trim for repo size, but keep this ordering logic.

1. Checkout & deps: lockfile integrity, reproducible installs.
2. Static analysis: lint, format check (or format bot commit).
3. Types: TypeScript, mypy, or your project’s type checker—at parity with build.
4. Unit & integration: suites tied to touched modules; monorepos may use affected-only runs.
5. Build: app or library builds cleanly; multi-package graphs stay valid.
6. (Optional) Contract / E2E: when APIs or critical paths move; dry-run migrations for schema changes.

When claiming “this step is done,” cite concrete commands or CI job names—not only “tests passed.”

Anti-patterns (5 unsafe refactoring before/after)

❌ Anti-pattern 1: Missing reflection/string references when renaming
Before: function getUserData(id) { ... }
After:  function fetchUserProfile(id) { ... }
Problem: routes.ts has app.get('/user', handlers['getUserData'])  ← string reference missed
Fix: use IDE global refactor instead of manual find & replace

❌ Anti-pattern 2: Silently changing default parameter values during extract
Before: function formatDate(date, format = 'YYYY-MM-DD') { ... }
After:  function formatDate(date, format = 'MM/DD/YYYY') { ... }  ← default quietly changed
Fix: never change default values during extraction; lock default behavior with a characterization test

❌ Anti-pattern 3: Updating some but not all imports after moving a file
Before: import { helper } from '../utils/helper'
After:  import { helper } from '../shared/helper'  ← only some files updated
Problem: old ../utils/helper deleted; other files that still import it fail to compile
Fix: add re-export first (utils/helper.ts → export * from '../shared/helper'),
     batch-update all imports, then delete the re-export

❌ Anti-pattern 4: Introducing duplicate side effects when inlining
Before:
  function logAndReturn(val) { metrics.count++; return val; }
  const a = logAndReturn(x);
  const b = logAndReturn(y);
After (inlined):
  const a = (metrics.count++, x);  // ← metrics.count incremented unexpectedly
  const b = (metrics.count++, y);
Fix: do not inline functions that have side effects

❌ Anti-pattern 5: Reversed refactoring order (delete old code before adding tests)
Before: delete the old implementation directly; new implementation has no tests
Problem: when new implementation has a bug, no tests act as a safety net; rollback is costly
Fix: the required order is: add tests → implement new logic → confirm tests green → delete old code

One-line commit (prefix + subject)

Prefixes clarify intent for logs and reverts: refactor: structure-only, test: tests only, chore: tooling/config. Choose a prefix and subject below; stored only in this page’s localStorage under a unique key.