You inherit a class named AccountService with 40 public methods, 14 injected dependencies, and several hundred lines of branching business rules that every screen and job in the system calls. Using GRASP reasoning, how do you diagnose and refactor it?
answer
- 4 bloat symptoms: all events, does the work, much state, low cohesion
- cure order: push down (Info Expert) THEN split (use cases)
- dependency-usage matrix + co-change history = seams
- strangle: old class becomes thin delegating facade
- split vertically by use case, never horizontally by layer
basics
~20 sThat's a bloated controller: it receives too many events, does the work itself, and holds too much. Fix it by splitting into one coordinator per use case and pushing the actual rules down into the domain objects that own the data.
solid answer
~60 sDiagnose with GRASP's named symptoms of a **bloated controller**: it receives most system events across unrelated features; it performs the work instead of delegating; it carries lots of state/dependencies; its cohesion is unfocused. The 14 dependencies with mostly disjoint usage per method is the hard evidence. Two complementary cures, in order: 1. **Push logic down (Information Expert).** For each rule, ask which class owns the data needed to decide, and move the rule there — an entity method, a value object invariant, or a domain service if the rule spans aggregates. The controller shrinks to load → delegate → save. 2. **Add more controllers (use-case split).** Group the remaining methods into cohesive scenarios and extract `OpenAccountHandler`, `CloseAccountHandler`, `TransferFundsHandler`, each holding only its own collaborators. Sequence it safely: characterization tests first, then strangle — keep the old class as a thin delegating facade so all callers keep compiling, migrate them incrementally, delete it last. Measure success by dependency count per class, test setup size, and change contention, not by line count moved.
go deeper
Recognize the symptom (one class doing everything) and name the direction: split by use case, move rules onto the objects that own the data.
Give the four bloated-controller symptoms and both cures, and mention keeping the old class as a delegating facade so callers keep working.
Add the evidence-gathering step (dependency-usage matrix, co-change analysis, caller map), the ordering argument (push down before splitting), characterization tests, incremental strangling, and vertical-vs-horizontal slicing.
Frame it as a program: sequence seams by upcoming roadmap value, define measurable exit criteria (deps per class, test setup, change contention), install executable guardrails against regrowth, and explicitly decide not to refactor stable code that no roadmap item touches.
## 1. Diagnosis, in GRASP vocabulary Larman lists four symptoms of a **bloated controller**; check them explicitly rather than saying "it's too big": 1. **Single class receives all/most system events** — here, every screen and job calls it. 2. **The controller performs many of the tasks itself** instead of delegating — the hundreds of lines of branching rules. 3. **The controller has many attributes / maintains significant system state** — 14 dependencies is the structural analogue. 4. **Low, unfocused cohesion** — you cannot describe the class in one sentence without the word "and". Corroborating evidence worth gathering before touching code: - **Dependency-usage matrix**: methods × injected collaborators. Clusters of methods using disjoint collaborator subsets are seam candidates — this is essentially manual cohesion analysis (related to the LCOM family of metrics). - **Change history**: which methods change together in the same commits? Co-change clusters are better seams than the class's current alphabetical grouping. - **Caller map**: which entry points call which methods? A method called by one screen and nothing else is easy to peel off first. - **Transaction/authorization shape**: methods with different policies almost never belong together. ## 2. Two cures, and the order matters ### Cure A — Push logic down (Information Expert) For each rule, ask: *which class already holds the data needed to decide this?* Move the rule there. - Rule about one entity's state → **entity method** (`account.close()` enforcing "balance must be zero"). - Rule about a value's validity → **value object** (`Money`, `Iban`) enforcing invariants at construction. - Rule spanning several aggregates with no natural owner → **domain service** (a Pure Fabrication) — but keep it in the domain layer, not in the coordinator. - Rule about *sequencing* (load A, call B, publish C, commit) → legitimately stays in the coordinator; that is what a controller is for. Do this **first**, because it shrinks the material you then have to partition, and because splitting a class whose methods still contain rules just produces several smaller bloated classes. ### Cure B — Add more controllers (use-case split) Partition the remaining coordination into one class per use case: ``` AccountService (40 methods, 14 deps) ├─ OpenAccountHandler (deps: accounts, kycGateway, clock) ├─ CloseAccountHandler (deps: accounts, ledger) ├─ TransferFundsHandler (deps: accounts, ledger, limits, events) └─ ... ``` Each handler gets a single `handle(command)` or a small set of scenario steps, and only the collaborators it actually uses. ## 3. A safe refactoring sequence 1. **Freeze behaviour with characterization tests.** Before understanding the rules, capture what the code currently *does* (including the wrong bits) at the public surface. Without this, you cannot distinguish a refactor from a rewrite. 2. **Pick the seam with the best ratio of value to risk** — usually a cluster with few callers, its own dependencies, and recent change activity. 3. **Extract the new handler**; have the old method body become a one-line delegation. Nothing else changes; all callers still compile. This is the **strangler** approach applied inside a class: the old class survives as a thin facade during migration. 4. **Migrate callers** to the new handler incrementally, one entry point per change, each independently shippable and revertible. 5. **Push rules into the domain** for that slice, replacing branching with polymorphism/entity methods where it genuinely reduces conditionals (GRASP Polymorphism) — but only where the conditional is on a *type*, not everywhere. 6. **Delete the delegating method** once its last caller is gone. Repeat. 7. **Add a guardrail** so the class cannot regrow: an architecture test / lint rule capping constructor arity or forbidding new methods on the legacy class, plus a code-review convention that new use cases get new handlers. ## 4. What *not* to do - **Mechanical splitting by size** ("AccountServiceA/B/C", or splitting alphabetically) — it moves the mess without improving cohesion and destroys navigability. - **Splitting by technical layer only** (`AccountValidationService`, `AccountPersistenceService`, `AccountCalculationService`) — this is horizontal slicing; a single business change then touches all three, which is the opposite of the goal. - **Big-bang rewrite** with no characterization tests — the rules encoded in those branches usually include undocumented business behaviour that customers depend on. - **Extracting private helpers only** — a 40-method class with tidy private methods still has 14 dependencies and the same change contention. - **Deleting the old class immediately**, breaking every caller in one commit. ## 5. Measuring success Don't claim victory on line count. Track: - **dependencies per class** (target: single digits, ideally ≤4); - **test setup size / test runtime** for a single use case; - **change contention**: number of PRs touching the same file per sprint; - **blast radius**: files changed for a typical feature (should go down for vertical features, up only if you sliced horizontally — a warning sign); - **cognitive load**: can a new joiner find where 'transfer funds' happens in under a minute? ## 6. When to leave it alone If the class is stable (rarely changed), well covered, and not on the path of upcoming work, refactoring it is inventory, not value. Refactor along the grain of the work you actually have — the seam you need next quarter is the seam worth cutting now.
- Why push logic into the domain before splitting the class?Splitting first just yields several smaller classes that still do the work themselves — the bloat symptom of 'performs the tasks itself' survives the split. Shrinking each method to coordination first makes the remaining partition obvious and the resulting handlers genuinely thin.
- How do you keep the class from growing back after the refactor?Make the convention executable: an architecture/lint rule capping constructor arity or method count, a check that forbids adding methods to the legacy class, and a review norm that every new use case gets its own handler. Conventions that aren't enforced decay.
- What is wrong with splitting into ValidationService, CalculationService, and PersistenceService?That is horizontal slicing by technical concern: a single business change then edits all three classes, so coupling per feature goes up rather than down. Slice vertically by use case so a feature change lands in one place.
It's a single overloaded manager who personally does every job in the department. You don't fix it by giving the manager three desks; you first hand each task to the specialist who already owns the relevant knowledge, then appoint one coordinator per workflow.
saying these in an interview costs you the question
- Proposing a big-bang rewrite without characterization tests first
- Splitting by size or alphabetically rather than by cohesion (shared dependencies, co-change)
- Slicing horizontally into validation/calculation/persistence services and calling it decomposition
- Extracting private methods only, leaving the dependency count and change contention unchanged
- Claiming success from reduced line count while dependency count and test setup stay the same
- Refactoring a stable, rarely-touched class that isn't on the path of upcoming work