# Refactor > Refactor Java code in this repo without changing behavior. Not for bug fixes or new features. - Skill: `membrane/refactor` (Agent Skill) - Install (CLI): `npx skillmds@latest add membrane/refactor` - Raw SKILL.md: https://api.skillmd.com/api/skills/membrane/refactor/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - Author: membrane (https://skillmd.com/u/membrane) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/membrane/refactor --- # Refactor Behavior-preserving restructuring, Fowler-style. The catalog below is the vocabulary; the workflow is what keeps it safe. ## Workflow 1. **Pin behavior first.** Find the existing test class (`/src/test/java//Test.java`). If the code you're about to move has no test covering it, write a characterization test against the *current* behavior before touching anything. If the code is untestable as-is, the first refactoring is the one that makes it testable (usually Extract Method / Extract Class), done in the smallest step you can verify by compiling. 2. **One refactoring per step.** Apply a single named transformation, then compile. Never mix two catalog entries in one edit, and never mix a refactoring with a behavior change — if you spot a bug mid-refactor, note it and finish the refactor first, then fix it as a separate change with its own failing test. 3. **Run the affected tests** after each step, not just at the end, and delegate the run to the `test-runner` agent (CLAUDE.md) instead of hand-rolling Maven — it picks the runner for what you touched (a `core` class or package, a distribution/tutorial IT, or a module-wide run for `annot`; `war` has no tests of its own) and knows the traps that make a hand-rolled command silently run the wrong scope. Green after every step is the safety net for behavior, but a sequential test run does not establish thread safety: a refactoring that changes what is shared between threads needs a targeted concurrent test (below). 4. **Report** at the end: which refactorings were applied to which methods, what is now testable that wasn't, whether anything you moved changed what is shared between threads, and anything you deliberately left alone. ## Scope discipline - Refactor only what the user named plus what that change strictly requires. Adjacent messy code stays messy — mention it, don't touch it (CLAUDE.md §3). - Public API of `@MCElement`-annotated config classes is a contract: attribute/child setter names and signatures are the config grammar. Do not rename or re-sign them as part of a refactor. Private helpers behind them are fair game. - Delete what your change orphaned (now-unused imports, fields, private methods). Leave pre-existing dead code alone. ## Thread safety One interceptor instance serves every request thread (`Interceptor` javadoc: "Interceptor implementations need to be thread safe"), and ``/internal routing re-enters that same instance on its own thread — so per-request state in a field is corrupted by nesting before it is ever raced on. A refactoring that moves state out of a local and onto the object therefore changes behavior invisibly: parallel execution is commented out in `core/src/test/resources/junit-platform.properties`, so every unit test runs single-threaded and an ordinary sequential test run does not establish thread safety. Catch it by reading the diff, and when a refactoring changes what is shared between threads, cover it with a targeted concurrent test (drive the refactored code from several threads — `ExecutorService` plus a `CountDownLatch` to start them together — and assert each thread's own result). **Preserve what is shared.** Locals, parameters and return values are per-request; fields on an interceptor are per-server. Never convert the first into the second to shorten a signature. Real per-request state goes on the `Exchange` (`ProtocolHandler` javadoc), never into a field — see the standing reminder at `REST2SOAPInterceptor.java:183`, "Determine SOAP version per-request; do not cache in instance state". Scan your own diff for these, each a race until you can argue otherwise: - **A new field** on an interceptor that a request path writes. - **Extract Class whose result becomes a field.** Anything holding per-message state is built per call — `XMLProtector` wraps one message's writer. Only a stateless or immutable helper may be held as a field. - **Lazy initialization or a cache** introduced by Replace Temp with Query. Config-derived values are computed in `init()`, which is guaranteed to run before any port opens — prefer that (the `config-error-handling` skill), and keep it idempotent: `RuleReinitializer` can re-run it. - **`static` mutable state.** `static final` is safe for primitives, `String` and `List.of(...)`; it is a race for `SimpleDateFormat`, `Matcher`, `MessageDigest`, `DocumentBuilder`, `Transformer` and the StAX/DOM/XPath factories. The repo's three sanctioned answers: build it per call, a `static final ThreadLocal` (`HardenedStaxInputFactory`, `XPathUtil`), or a pool (`XSLTTransformer`). - **Deleting `volatile`, `synchronized`, `Atomic*`, `ThreadLocal` or a concurrent collection** because it reads as redundant — assume it is load-bearing until `git log -S` says otherwise. - **Handing a body or an `Exchange` to another thread** — `AbstractBody`: "Accessing the body from multiple threads is illegal." `final` and CLAUDE.md's immutable-by-default rule are doing concurrency work here, not just style work: a `final` field is safely published, a `record` parameter object is shareable by construction. A race you *find* is a bug, not a refactoring — note it and fix it separately (step 2). ## Catalog ### Extract Method — the default move Pull out any block that has one responsibility and could be tested on its own. Signals: a comment explaining what the next few lines do, a blank-line-separated paragraph inside a method, a loop body doing real work, a nested conditional branch of more than ~3 lines. - Name the method after **what it answers or produces**, not how — `isExpiredToken`, `resolveSchemaFor`, not `doCheck2`. - If the block reads three fields and writes none, pass them as parameters; if it writes two or more locals, the block wants Extract Class instead (below), not a method with out-params. Either way the extracted method takes what it needs as parameters — a new field to dodge a parameter is shared state (see Thread safety). - Keep the extracted method small and cohesive; a helper that itself needs a section comment is not done being extracted. ### Replace Temp with Query / Inline Variable A local that is assigned once from an expression and read later is usually a name looking for a method. Replace it with a call to a small query method — that removes the temp *and* makes the computation reachable from a test. - **Only when the expression is pure.** A query is evaluated at every read, so this is not behavior-preserving if the expression has side effects, observes mutable state that changes in between, allocates an object whose identity is compared, or is expensive. In those cases keep the local (call the helper once and assign it) instead. On a shared interceptor "changes in between" includes another thread changing it, so a query reading a mutable field is not pure. - Inline a temp outright only when the expression is short - A variable that gets **reassigned** is the strongest extract signal in this repo: CLAUDE.md says reassignment means "extract a helper method instead". Convert accumulate-in-a-loop temps into a helper returning the value, and make the remaining locals `final`. - Split Temporary Variable when one local holds two different things at two points; never reuse a name for a second purpose. ### Remove Duplication Duplication first, abstraction second — extract the shared code only once you've seen it twice and the two copies mean the same thing (same reason to change), not merely look alike. - Identical code in **two methods of one class** → Extract Method, call it from both. - Identical code in **two sibling subclasses** → Pull Up Method to the shared supertype. - Same shape, different values → parameterize the extracted method. Same shape, different *step* → pass the varying step as a small functional interface or use Template Method — but only if there are ≥3 call sites, otherwise the indirection costs more than the duplication. - Similar-looking code that would need to change for different reasons is **not** duplication. Leave it and say so. ### Introduce Abstractions — only when they pay CLAUDE.md forbids speculative abstraction. Introduce one only when a concrete pressure exists now: - **Extract Class** — a class holding two clusters of fields that don't talk to each other, or a method needing 4+ locals to survive extraction. Move the cluster and the methods that use it. Decide the new object's lifetime explicitly: per call (the default for anything stateful) or a field (only if stateless or immutable). - **Replace Conditional with Polymorphism / sealed switch** — a `switch` or `if/else` chain on a type tag repeated in more than one method. This repo prefers pattern matching over sealed types and records to hand-rolled hierarchies (CLAUDE.md code style) — prefer a `switch` over a sealed interface before inventing an abstract base class. - **Introduce Parameter Object / Value Object** — see long parameter lists below. - **Replace Magic Literal with Constant** — a literal appearing twice, or once with non-obvious meaning. `static final` only for immutable values: a shared `SimpleDateFormat` or `Matcher` is a race, not a constant. - Do **not** introduce an interface with a single implementation, a factory for a constructor call, or a strategy for a two-branch conditional. When you think an abstraction is worth it, ask. ### Move Method — feature envy A method that calls more methods/fields of another object than of its own belongs on that other object. Move it, leaving a delegating method behind only if external callers need it. - Extract the envious *part* first if only a portion is envious, then move that. - Related smells: Move Field (a field used mainly by another class), Hide Delegate (callers doing `a.getB().getC().doIt()` — give `a` the method), Remove Middle Man (a class that only forwards). - If the target class is one you must not change (a JDK/library type or a generated/config class), don't move — extract a static helper in a `util`-style class instead and note why. ### Long Parameter Lists Threshold: **more than 3 parameters, or any two adjacent parameters of the same type** (call-site transposition bugs). - Parameters that always travel together and mean one thing → **Introduce Parameter Object**, as a `record` (immutable, matches the repo's immutable-by-default rule). - A parameter derivable from another parameter → **Replace Parameter with Query**, drop it. - A boolean flag that selects behavior → **Remove Flag Argument**: split into two clearly named methods. - Several parameters that are all fields of the caller → the method probably wants to move to the caller's class (feature envy, above). - Promoting parameters to fields is **not** a way to shorten a signature — on an interceptor that turns per-request state into per-server state (see Thread safety). - Don't abbreviate parameter names in public interfaces (`docs/CONVENTIONS.md`). ## Repo-specific constraints - Java 21. Prefer `record`s for parameter/value objects, pattern-matching `switch` over sealed types, `List.of`/`Collections.unmodifiableList` for extracted collection state, `getFirst()` over `.get(0)` — but note the swap changes the empty-list exception from `IndexOutOfBoundsException` to `NoSuchElementException`, so only do it where the list is guaranteed non-empty or the exception type isn't part of the contract (no test or caller catches it). - `final` on extracted fields, parameters, and locals wherever it compiles. - SLF4J only; never introduce `System.out` while moving code. - Don't reformat lines you didn't otherwise change — a diff full of whitespace hides the real refactoring. - Every newly extracted method that has real behavior needs at least one test; add it to the existing mirrored test class rather than creating a new one (CLAUDE.md testing rules). The exemption covers only *plain* accessors — a bare field read/write, `record` components, generated `equals`/`hashCode`/`toString`. An extracted accessor that validates, lazily initializes, computes, logs, or does I/O has behavior and needs a test. ## When to stop Stop when the named smell is gone. Resist the pull to keep going: a refactor that touches twice the files the user expected is a worse outcome than one that leaves a second smell for later. Name the leftovers in your report instead of fixing them.