Refactor Guide

refactor-guide

Purpose

Identify code smells by name and describe their impact on the codebase — never show the refactored version, never write the replacement code, never prescribe which refactoring to apply.

Hard Refusals

• Never show refactored code — not even "it could look something like this." The human must write the improvement.
• Never name a specific refactoring technique and tell the human to apply it — "extract this into a method" is a prescription. Name the smell instead and let the human decide.
• Never say the code is clean — code always has tradeoffs; approval without full context is not useful.
• Never prioritize the refactoring backlog for the human — ordering which smells to fix first is a judgment call that belongs to the human.
• Never refactor in service of aesthetics — only engage with smells that have a named, concrete cost.

Triggers

• "This code needs refactoring / cleaning up"
• "This feels wrong but I don't know why"
• "How do I make this better?"
• "This is getting hard to work with"
• Code pasted with a request for structural improvement

Workflow

1. Get the context before reading the code

Before examining code, ask the human for context.

Gate 1: Human has described intent, pain, change frequency, and recent context.

Memory note: Record the pain description in SKILL_MEMORY.md.

2. Identify and name code smells

Read the code and produce a list of named code smells. Each entry must follow this format:

Smell: [name of the smell]
Location: [where in the code — function name, line range, pattern]
Impact: [what becomes harder because of this smell — reading, testing, changing, debugging]

Code smell reference:

Limit to the 5 most impactful smells per session. More than 5 at once is not useful.

Gate 2: At least one smell has been named with location and impact.

3. Ask the human to assess each smell

For each smell, ask one question that makes the human engage with its cost:

Gate 3: Human has responded to the assessment question for each named smell — engaging with the cost, not just acknowledging the label.

4. Let the human decide

After Gate 3, ask the human to decide the disposition of each smell:

"For each smell you've assessed, what's your decision:
 - Fix now
 - Defer (with a reason)
 - Accept (because the cost is justified by the context)"

Do not suggest which to fix first. Do not suggest which to accept. The human owns the refactoring backlog.

Gate 4: Human has stated a decision for every named smell.

Deviation Protocol

If the human says "just show me what the refactored version should look like":

1. Acknowledge: "I understand — seeing the destination makes the path clearer." 2. Assess: Ask "Which smell feels most unclear to you — what the problem is, or what fixing it would involve?" — the request for a refactored example usually means the smell's impact isn't clear yet. 3. Guide forward: Deepen the impact question for that specific smell (step 3). The goal is for the human to understand the smell well enough to write the fix themselves.

Related skills

• skills/core-inversions/code-review-challenger — when refactoring assessment happens in the context of a code review
• skills/cognitive-forcing/complexity-cop — when the smells are primarily about over-engineering
• skills/cognitive-forcing/first-principles-mode — when the smells suggest the design assumptions need revisiting, not just the code

Refactor Guide — Extended Code Smell Reference

This file extends the 19-smell table in the refactor-guide workflow with fuller detection signals, concrete cost descriptions, and the single most effective interrogation question for each smell. The SKILL.md covers the core set; use this reference when the smell you're seeing isn't captured there or when you need more depth to help the human engage with a specific pattern.

---

Method-Level Smells

Long Method

What it looks like: A method that does more than one conceptual thing — setup, validation, transformation, persistence, and notification all in one function. Reading it requires holding the entire state machine in your head simultaneously.

Detection signals:

• Method longer than can be read in one screen without scrolling
• Multiple levels of indentation (nested loops inside conditionals inside try/catch)
• Comments that say "now do X" — comments that narrate steps are a sign steps should be functions
• Variables declared at the top that aren't used until the bottom
• The method name is a verb that describes only one of the three things it does

Cost: Any change to one step risks breaking another. Testing requires setting up state for all steps even when testing one. The name lies — the method does more than it says.

Interrogation question: "If you had to explain what this method does to a new engineer, how many sentences would it take? Each sentence is a candidate for a separate function."

---

Long Parameter List

What it looks like: A function that takes more parameters than can be held in working memory — typically more than 4, often more than 7. Callers can't remember the order. Readers can't understand the call site without looking at the signature.

Detection signals:

• Boolean parameters that control which of several behaviors the function performs
• Parameters that are always passed together (they should be a type)
• null passed for parameters that don't apply to this call
• Parameters that are only used in one branch of the function

Cost: Every call site is a puzzle. Parameter order errors are silent — wrong order, same type. Adding a new parameter requires changing every call site. Boolean parameters make the function a disguised switch statement.

Interrogation question: "When a caller invokes this function, can they tell what each argument means at the call site without looking at the signature? What does processOrder(true, false, null, 3) mean to a reader?"

---

Feature Envy

What it looks like: A method that spends most of its time using data from another class — calling getters, accessing fields, or invoking methods of another object rather than working with its own data.

Detection signals:

• A method that takes an object as a parameter and calls five methods on it
• A method that duplicates logic already present in another class
• A method whose name would make more sense as a method on the object it's most interested in

Cost: Logic about a type lives somewhere other than the type. When the type changes, you have to find and update all the methods in other classes that are envious of it.

Interrogation question: "Which object does this method know the most about — the one it belongs to, or one of its parameters? What would it look like if this method lived on the object it's most interested in?"

---

Inappropriate Intimacy

What it looks like: Two classes that know too much about each other's internals — accessing private fields through reflection or package-level access, depending on specific implementation details rather than contracts.

Detection signals:

• Class A accesses fields of Class B that are only intended for internal use
• Two classes that always change together — every commit to one touches the other
• Tests for Class A that require setting up the internals of Class B
• Circular imports or circular dependencies between two modules

Cost: Neither class can be changed independently. They are effectively one class pretending to be two. Testing one requires understanding both.

Interrogation question: "When was the last time you changed Class A without also changing Class B? What does that tell you about whether they're actually separate?"

---

Data Clumps

What it looks like: Groups of data that always appear together but haven't been given a type — three parameters that are always passed together, four fields that are always set at the same time, a struct that gets built and then immediately deconstructed.

Detection signals:

• Three or more fields that always appear together in function signatures
• Code that checks if x != null && y != null && z != null repeatedly
• A pattern where you extract three values from one object and pass all three to another function
• The same group of fields duplicated across multiple classes

Cost: The relationship between the data items is implicit. Validating the group requires repeating the validation everywhere the group appears. Adding a field to the group requires changing every place it's used.

Interrogation question: "Do these three values always travel together? Could you give that group a name? What would it mean to have a valid X without a Y?"

---

Primitive Obsession

What it looks like: Domain concepts represented as primitive types — a user ID as a plain integer, a currency amount as a float, a status as a string constant — when a dedicated type would carry the concept's rules with it.

Detection signals:

• An int that represents a user ID mixed up with an int that represents a product ID — both the same type
• A string that must match a specific format but the format isn't enforced at the type level
• A float used for money — with all the floating-point precision issues that implies
• Constants like STATUS_ACTIVE = "active" spread across files

Cost: The constraint on the primitive must be validated everywhere the primitive is used. Type safety doesn't protect against passing a user ID where a product ID is expected. Business rules about the domain concept are scattered rather than encapsulated.

Interrogation question: "If the format of this value changes — say, user IDs go from integers to UUIDs — how many places in the codebase would you need to update? What would you miss?"

---

Class-Level Smells

Large Class

What it looks like: A class that has accumulated responsibilities across multiple unrelated domains — originally had one job, then became the convenient place to add the next thing, and the next.

Detection signals:

• Class with more than 20 methods
• Class whose fields can be split into two groups that don't interact with each other
• Class name that includes "Manager", "Handler", "Processor", or "Service" with no further qualification
• Class that imports from more than a third of the codebase
• Tests for this class that require setting up state irrelevant to the thing being tested

Cost: Any change risks unintended interaction with one of the other responsibilities. The class is a merge conflict magnet. Understanding it requires reading all of it, even to make a small change.

Interrogation question: "Can you describe what this class does in a single sentence using a specific noun — not 'manages things' or 'handles requests', but something concrete? If not, how many sentences does it take?"

---

Divergent Change

What it looks like: A class that gets modified for multiple unrelated reasons — you change it when adding a new report format, and also when changing the database schema, and also when adding a new payment provider.

Detection signals:

• A class that appears in commits touching three different feature areas
• Methods in the class that are grouped into unrelated clusters
• "We always end up touching this class no matter what we're building"

Cost: Every change to the class for Reason A risks breaking the parts of the class that serve Reason B. The class couples unrelated concerns.

Interrogation question: "In the last ten commits, what were the three most different reasons you changed this class? Do those reasons have anything to do with each other?"

---

Shotgun Surgery

What it looks like: The inverse of Divergent Change — one conceptual change requires small edits scattered across many different classes.

Detection signals:

• "When I add a new [X], I have to update the factory, the registry, the router, the validator, and the test fixture"
• A feature flag that requires touching five files to add a new value
• A pattern where grep -r 'old-string' returns matches in a dozen files that all need updating together

Cost: It's easy to miss one of the required locations. Each location is a place where a merge conflict can block the change. The change is fragile — the next person to make a similar change may not know all the places.

Interrogation question: "The last time you added a new [type/variant/feature], how many files did you touch? Were they all related, or did they just happen to all need the same change?"

---

Lazy Class

What it looks like: A class that does so little it barely justifies its existence — a thin wrapper around one other class, or a class with a single method that could just be a function.

Detection signals:

• Class with one or two methods, both of which call through to another class
• Class that adds no behavior — just passes arguments through unchanged
• Class that exists "for future expansion" but hasn't expanded in a year

Cost: Every reader must learn that this class exists and what it does, even though it does almost nothing. It adds a level of indirection for no benefit.

Interrogation question: "What does this class do that its callers couldn't do by calling [the thing it wraps] directly? If the answer is nothing, what's the reason it exists as a separate class?"

---

Parallel Inheritance Hierarchies

What it looks like: Two class hierarchies that must be kept in sync — every time you add a subclass to one, you must add a corresponding subclass to the other.

Detection signals:

• Two hierarchies where every class in Hierarchy A has a corresponding class in Hierarchy B
• Names that mirror each other: UserValidator, AdminValidator, GuestValidator and UserSerializer, AdminSerializer, GuestSerializer
• A factory that switches on the same condition to create objects from two different hierarchies

Cost: Adding a new type requires changes in two places that may be maintained by different people or teams. The parallelism is a hidden constraint that isn't enforced by the type system.

Interrogation question: "When you add a new type to [Hierarchy A], what do you have to add to [Hierarchy B]? Is that enforced anywhere, or is it convention that could be forgotten?"

---

Code Quality Smells

Duplicate Code

What it looks like: The same or nearly-identical logic appearing in two or more places — copied and pasted, then slightly modified, then modified again when the first copy was updated but the second was forgotten.

Detection signals:

• Two methods that look almost identical except for one variable name or one conditional
• Bug fixes that require the same change in multiple files
• "I updated it in [place A], did you update it in [place B]?"
• Utility functions that were written twice because the second author didn't know the first existed

Cost: Every bug in the logic exists in all copies. Every fix must be applied to all copies. The copies diverge over time — they start as the same thing and become subtly different.

Interrogation question: "If a bug was found in this logic, how many places would you need to fix it? How confident are you that you'd find all of them?"

---

Dead Code

What it looks like: Code that is never called — functions that have no callers, conditional branches that can never be true, parameters that are always passed the same value.

Detection signals:

• Functions that don't appear in any grep of the codebase
• if (false) or if (DEBUG && false) blocks
• Parameters whose value is always null or always the same constant at every call site
• Commented-out code that has been commented out for more than a month

Cost: Dead code must be read and understood by every new team member even though it does nothing. It creates false leads during debugging. It grows stale — the dead code doesn't keep up with the live code around it.

Interrogation question: "When was this last called? Is there a test that exercises this path? If you deleted it today, what would break?"

---

Speculative Generality

What it looks like: Abstractions, parameters, or extension points built to accommodate use cases that don't exist yet — making the code more complex in anticipation of requirements that may never come.

Detection signals:

• An interface with one implementation "because we might need more"
• A configuration parameter that is never set to a non-default value
• A plugin system with no plugins
• "We built it this way so it would be easy to extend later"

Cost: Readers must understand the abstraction and all the flexibility it provides, even though only one path is ever taken. The flexibility constrains future implementations — they must fit the shape of an abstraction designed without their requirements.

Interrogation question: "What is the concrete second use case this generalization was built for? When will it exist? What's the cost of adding the generalization then, when you have the actual requirements?"

---

Comments That Explain What Instead of Why

What it looks like: Comments that re-narrate what the code does — restating in English what is already clear from reading the code — instead of explaining why the code is the way it is.

Detection signals:

• // increment i above i++
• // get the user from the database above user = db.getUser(id)
• Long comments explaining an algorithm that's straightforward, without explaining why that algorithm was chosen over alternatives
• No comment on code that is genuinely non-obvious — a magic number, a workaround for a library bug, a business rule that isn't derivable from the code

Cost: The comments lie as soon as the code changes but the comment isn't updated. They add noise that makes the real explanatory comments harder to find.

Interrogation question: "For each comment in this code — does it tell the reader something they couldn't get by reading the code itself? If not, what does it say that the code doesn't? If yes, why is that context in a comment rather than in the code structure?"

---

Temporary Field

What it looks like: Fields on a class that are only valid in some execution states — set in one method, used in another, and undefined (null, zero, or empty) in all other states.

Detection signals:

• Fields that are null in some states and populated in others
• Fields with names like _tempResult, _cachedValue, _currentRequest
• Methods that begin by setting several fields and end by clearing them
• Comments like // only valid after calling initialize()

Cost: Any code that accesses the field must first verify it's in a valid state. The class's invariants change depending on execution state. Testing requires knowing which state the object must be in for each test.

Interrogation question: "When is this field null or invalid? What code can safely access it? Is that constraint documented and enforced, or is it just convention?"

---

Message Chains

What it looks like: Code that navigates a chain of objects to get to the data it needs — order.getCustomer().getAddress().getCity() — violating the Law of Demeter and coupling the code to the entire chain of relationships.

Detection signals:

• Method calls chained three or more levels deep
• Code that breaks when any intermediate object in the chain changes its structure
• "I need to get to [deeply nested thing]" as a recurring pattern

Cost: The caller is coupled to the entire chain structure. If any intermediate relationship changes, the caller breaks. The chain must be navigated every time — there's no caching of intermediate results.

Interrogation question: "If the relationship between [middle object] and [end object] changed, how many call sites would break? Should the object at the start of the chain provide a more direct path to what you need?"

---

Middle Man

What it looks like: A class that delegates everything it does to another class — every public method is just a pass-through to a collaborator, adding a layer of indirection with no behavior of its own.

Detection signals:

• A class where every method calls the same method on the same collaborator with the same arguments
• A facade that hasn't added any simplification — it exposes the full complexity of the thing it wraps
• A proxy class that adds no cross-cutting concern (logging, caching, auth) — just passes through

Cost: Callers must know this class exists even though it adds nothing. Every new method on the collaborator requires a corresponding pass-through on the middle man. The layer makes stack traces longer without adding value.

Interrogation question: "What would break if callers called [the collaborator] directly instead of going through this class? If the answer is nothing, why does this class exist?"