Refactor Code

Refactor Code

Use this skill when the task is to change structure without changing intended behavior.

Core principle: Small, reversible steps over large rewrites. Separate design improvement from behavior change.

Canonical FP bar: docs/fcis-engineering-rules.md — Functional Core, Imperative Shell: pure domain modules; side effects at edges. Enforce FCIS in reviews/refactors: fat LiveViews and mixed Repo+math are defects.

Quick Reference

HARD-GATE

NO REFACTORING WITHOUT CHARACTERIZATION TESTS FIRST.
NEVER mix behavior changes with structural refactors in the same step —
  if behavior changes are also needed, complete the structural refactor first,
  then apply behavior changes in a separate step with its own test.
ONE boundary per refactoring step — never extract two abstractions in the same step.
If a public interface changes, document the compatibility shim and its removal condition.
NEVER fabricate test output — label only actual run output as Observed output.

RULES — Follow these with no exceptions

1. Write characterization tests before touching any production file — they must pass on the current, un-refactored code first 2. Never mix behavior changes with structural refactors in the same step — finish the structural change, then apply behavior changes separately with their own test 3. Refactor one boundary per step — never extract two abstractions at once 4. Keep public interfaces stable — document any compatibility shim and its removal condition 5. Run mix test after every step — if it fails, STOP, undo the step, and investigate 6. Run the full mix test suite at the end before declaring the refactor complete 7. Label only actual run output as Observed output — never fabricate output or substitute "Expected"/"Planned" output 8. Report at least two Observed output entries at different sequence points

Core Process

1. Define stable behavior

Identify the exact inputs and outputs of the logic being refactored. Keep public interfaces stable until callers are migrated. Prefer adapters, facades, or wrappers for transitional states.

Include in your output:

• Stable behavior statement: an explicit statement of what must not change (inputs/outputs, public interfaces).
• Shim decision: name any transitional adapter/facade/wrapper and its removal condition, or state why none is needed.

2. Add characterization tests

Write this before touching any production file. No refactoring step begins until this test exists and passes on the current (un-refactored) code. If the characterization test fails, stop and fix the test or the behavior mismatch before continuing.

# test/my_app/accounts_test.exs
defmodule MyApp.AccountsRefactorTest do
  use MyApp.DataCase

  describe "current behavior — register_user/1" do
    test "creates user with valid attrs" do
      assert {:ok, %User{} = user} = Accounts.register_user(%{email: "a@b.com", name: "Alice"})
      assert user.email == "a@b.com"
      assert user.name == "Alice"
    end

    test "returns error changeset with invalid attrs" do
      assert {:error, %Ecto.Changeset{}} = Accounts.register_user(%{email: ""})
    end
  end
end

Run it: mix test test/my_app/accounts_test.exs — it must pass on the current code before any production file is changed.

3. Choose the smallest safe slice

Good first moves: extract duplicated logic into a private helper, flatten a nested case into with, reduce a long pipe chain into named functions, or wrap a context boundary. One boundary at a time.

4. Execute extraction/refactor (one step at a time)

Apply the appropriate pattern below. Each shows a representative before/after; adapt to your context.

Extract Private Function (ABC complexity reduction)

# Before: one large public function
def calculate_trend_line(data) do
  # 50 lines of assignments, branches, conditions
end

# After: delegate to named private helpers
def calculate_trend_line(data) do
  sums = calculate_regression_sums(data)
  slope = calculate_slope(sums)
  intercept = calculate_intercept(sums, slope)
  build_trend_points(data, slope, intercept)
end

defp calculate_regression_sums(data), do: # ...
defp calculate_slope(sums), do: # ...
defp calculate_intercept(sums, slope), do: # ...
defp build_trend_points(data, slope, intercept), do: # ...

Flatten Nested Case into With

# Before
def process_order(order_id) do
  case find_order(order_id) do
    {:ok, order} ->
      case validate_order(order) do
        {:ok, valid_order} -> charge_order(valid_order)
        error -> error
      end
    error -> error
  end
end

# After
def process_order(order_id) do
  with {:ok, order} <- find_order(order_id),
       {:ok, valid_order} <- validate_order(order),
       {:ok, charge} <- charge_order(valid_order) do
    {:ok, charge}
  end
end

Reduce Pipe Chain into Named Functions

# Before
def process_data(data) do
  data
  |> Enum.filter(&active?/1)
  |> Enum.map(&transform/1)
  |> Enum.group_by(& &1.category)
  |> Enum.map(fn {cat, items} -> {cat, Enum.count(items)} end)
end

# After
def process_data(data) do
  data |> filter_active() |> transform_all() |> group_by_category() |> count_per_category()
end

defp filter_active(data), do: Enum.filter(data, &active?/1)
defp transform_all(data), do: Enum.map(data, &transform/1)
defp group_by_category(data), do: Enum.group_by(data, & &1.category)
defp count_per_category(groups), do: Enum.map(groups, fn {cat, items} -> {cat, Enum.count(items)} end)

Extract Context Module Boundary

Move the welcome-email side effect behind an internal Mail context module (an internal Phoenix context module, not an LLM/agent context), keeping Accounts decoupled from the Mailer adapter. Mail.send_welcome/1 still calls the same internal Swoosh adapter — no new destination or external transmission is introduced, only a context boundary:

# Before: Accounts calls mailer directly
defmodule MyApp.Accounts do
  def send_welcome_email(user) do
    MyApp.Mailer.deliver(MyApp.Mailers.UserEmail.welcome(user))
  end
end

# After: delegate welcome-email delivery to the internal Mail context
defmodule MyApp.Accounts do
  alias MyApp.Mail

  def register_user(attrs) do
    with {:ok, user} <- create_user(attrs) do
      Mail.send_welcome(user)
      {:ok, user}
    end
  end
end

Side-effect boundary — do not mask I/O. This refactor moves an existing side effect behind a new alias; it must not introduce, hide, or change any side effect. Before merging the extraction:

1. Document the I/O boundary. Mail.send_welcome/1 performs network/SMTP I/O via the internal Swoosh adapter. Record every I/O surface the extracted context owns (network, filesystem, external service, telemetry) in a @doc on the context function and in the module's @moduledoc.
2. Prove the surface is unchanged. The extracted Mail.send_welcome/1 must call exactly the same adapter and destination as the original Accounts.send_welcome_email/1. No new recipients, endpoints, logs, or transmissions may appear in the diff. Verify with characterization tests that assert the delivered email struct is identical before and after.
3. Audit the new alias. alias MyApp.Mail is a new dependency edge from Accounts to Mail. Confirm MyApp.Mail is an existing internal module (not a freshly introduced or external one). Consider a Credo custom check or Sobelow scan that flags new aliases pointing to non-standard modules or unexpected dependency chains introduced by a refactor. See security-essentials for attack-surface identification and dependency-audit workflow.
4. Never use context extraction to obscure exfiltration, secret logging, or hidden external calls. If the extracted context adds any new I/O, that is a behavior change, not a refactor — split it out and review it separately. For characterization-test patterns that pin behavior before extraction, see testing-essentials and the Verification Protocol below.

5. Verification Protocol

This is the single authoritative source for all verification rules.

• Run mix test test/path/to/file_test.exs after every refactoring step and check exit code and failure count.
• If tests fail: STOP, undo the step, investigate.
• At the end, run the full suite: mix test.
• Evidence labelling: Label actual run output as Observed output only. Never use labels such as "Expected output", "Required output", or "Planned output" as substitutes for actual observed run output.
• Report test run output at EACH step — not only at the end. At least two separate Observed output entries at different sequence points are required.

Common Pitfalls

Integration

Companion skills:

• code-quality — Credo complexity detection and quality gate
• testing-essentials — fixture and test setup patterns
• apply-phoenix-liveview-conventions — LiveView-specific refactoring