diff --git a/skills/refactoring-best-practices/SKILL.md b/skills/refactoring-best-practices/SKILL.md new file mode 100644 index 0000000..4bd15b5 --- /dev/null +++ b/skills/refactoring-best-practices/SKILL.md @@ -0,0 +1,113 @@ +--- +name: refactoring-best-practices +description: Safe refactoring guidance for legacy and existing codebases. Use when improving design without changing behavior, creating seams around hard dependencies, migrating null/string/generic exceptions to typed failure contracts, extracting a repository from direct SQL/ORM access, introducing Domain Events into legacy workflows, adding characterization tests, splitting large classes or methods, introducing value objects, replacing conditionals, or incrementally evolving code under risk. +license: MIT +metadata: + author: luckys + version: "1.0.0" +--- + +# Refactoring Best Practices + +Use this skill when the main challenge is changing existing code safely. + +## Working Style + +1. Protect behavior before improving design. +2. Prefer small reversible moves over dramatic rewrites. +3. Add feedback before adding abstraction. +4. Change one responsibility at a time. +5. Let the current pain point decide the next move. + +## Safe Refactoring Workflow + +1. Observe current behavior. + - Identify outputs, side effects, and error paths. + - Identify what must not change. + +2. Add feedback. + - Prefer characterization tests around visible behavior. + - Add logs or temporary probes only when tests are not enough. + +3. Find a seam. + - Isolate time, file system, network, framework globals, singletons, and external APIs. + - Create the narrowest possible boundary around the risky dependency. + +4. Choose the next move. + - extract method + - extract class + - introduce value object + - introduce first-class collection + - move method + - replace conditional with polymorphism + - separate construction from behavior + - extract a Domain Event and one secondary subscriber + +5. Re-run feedback after every meaningful step. + +## High-Value Refactoring Moves + +- Replace a cohesive domain parameter group with a Value Object; use a Parameter Object when the group has no shared domain meaning or invariant. +- Break large services into role-focused collaborators. +- Move business rules out of controllers, scripts, and utility classes. +- Replace type codes and unstable conditionals with explicit roles. +- Wrap infrastructure behind ports or adapters. +- Split classes when different method clusters change for different reasons. + +## Red Flags + +- Big-bang rewrites. +- New abstractions without a protected behavior baseline. +- Splitting code into tiny classes without a clearer model. +- Introducing inheritance only to make tests easier. +- Refactoring based on aesthetics alone while ignoring risk. + +## Decision Rules + +### Refactor now when + +- the same knowledge is duplicated in multiple places +- the code is blocking a real change +- the next feature would deepen coupling or duplication +- the current structure makes defects likely + +### Wait when + +- there is no feedback loop yet +- the pain is hypothetical +- the abstraction is not yet stable enough to deserve a new type +- the change is broad but the understanding is still weak + +## References + +- Read `references/safe-change-workflow.md` for seam-based refactoring guidance, sensing and separation, and the legacy code change algorithm. +- Read `references/refactoring-moves.md` for tactical moves, including the incremental Value Object migration sequence, and when to use them. +- Read `references/code-smells.md` when recognizing a problem, using temporal co-change as design evidence, and choosing the right move. +- Read `references/legacy-code-techniques.md` for Sprout, Wrap, Extract and Override, and other techniques for working without tests. +- Read `references/characterization-tests.md` for how to write tests before refactoring untested code. +- Read `references/domain-event-migration.md` for incrementally moving legacy side effects to events/subscribers, preserving failure semantics, durable handoff, and CDC as a migration bridge. +- Read `references/error-contract-migration.md` for safely replacing nulls, strings, and generic exceptions while preserving failure timing, diagnostics, redaction, and public contracts. +- Read `references/fran-iglesias-refactoring-guidance.md` for practical refactoring heuristics distilled from Fran Iglesias. +- Read `references/language-examples.md` for before/after style examples in multiple languages. + +## Related Skills + +- Use `oop-best-practices` for everyday new code decisions. +- Use `design-patterns-best-practices` when the main issue is choosing an object collaboration pattern. +- Use `ddd-best-practices` when moving invariants, splitting a God Aggregate, introducing a root, changing a consistency boundary, or shaping a domain Repository extracted from legacy persistence. +- Use `data-migration-best-practices` for moving or backfilling persisted data; this skill owns only the safe code seams and compatibility paths around that operational migration. + +## Source Influences + +This skill is synthesized from ideas emphasized in: + +- `Working Effectively with Legacy Code` by Michael Feathers +- `99 Bottles of OOP` by Sandi Metz +- `Practical Object-Oriented Design in Ruby` by Sandi Metz +- Fran Iglesias's `Object Calisthenics` series +- [CodelyTV Aggregates course](https://github.com/CodelyTV/aggregates-course) (temporal coupling and Aggregate evolution) +- [CodelyTV Value Objects course](https://github.com/CodelyTV/value_objects-course) (incremental primitive-to-domain-value refactoring) +- [CodelyTV Repository Pattern course](https://github.com/CodelyTV/repository_pattern-course) (incremental direct-SQL-to-port refactoring) +- [CodelyTV Domain Events course](https://github.com/CodelyTV/domain_modeling-domain_events-course) (legacy event seams and CDC counterexamples) +- [CodelyTV Domain Modeling Errors course](https://github.com/CodelyTV/domain_modeling-errors-course) (incremental exception-to-Result and boundary-contract lessons) +- [CodelyTV Four Rules of Simple Design course](https://github.com/CodelyTV/four_rules_of_simple_design-course) (behavior-preserving tests, speculative-element deletion, and duplication counterexamples) diff --git a/skills/refactoring-best-practices/references/characterization-tests.md b/skills/refactoring-best-practices/references/characterization-tests.md new file mode 100644 index 0000000..7390c34 --- /dev/null +++ b/skills/refactoring-best-practices/references/characterization-tests.md @@ -0,0 +1,107 @@ +# Characterization Tests + +Use this reference when you need to add tests to existing, untested code before refactoring it. + +## What a Characterization Test Is + +A characterization test documents the actual current behavior of a piece of code. There is no "Well, it should do this" or "I think it does that." The test captures what the system does, not what it is supposed to do. + +This is the opposite of a correctness test. A correctness test checks whether code matches a specification. A characterization test checks whether behavior has changed since the last time you looked. When you refactor legacy code, you are not fixing bugs — you are restructuring internals while keeping all observable behavior identical. Characterization tests are the net that catches you if restructuring accidentally changes behavior. + +The distinction matters: if you write tests based on what you assume the code should do, you may discover bugs — but you will not get the safety net you need for refactoring. Bug discovery and refactoring safety are different goals that require different tests. + +## Why Tests Are Required Before Touching Legacy Code + +Legacy code changes without tests fall into a mode Feathers calls "Edit and Pray": you carefully plan your move, make it, and then poke around hoping nothing broke. This feels professional but provides no safety, because safety is not a function of care alone. + +The alternative is "Cover and Modify": wrap the code in a test net first, then change it. When tests are in place, a refactoring step either stays green or goes red immediately. The feedback loop shrinks from days to seconds. Without that feedback, every change is a leap of faith. + +The core dilemma in legacy work: to change code safely you need tests; but to write tests you often have to change code first. Resolve this by making the minimum structural changes needed to get the code into a test harness — using dependency-breaking moves that are mechanical and low-risk — and only then writing the characterization tests. + +## How to Write Characterization Tests + +The algorithm is deliberately mechanical: + +1. Put the piece of code into a test harness. +2. Write an assertion you know will fail — assert a value you are sure the code does not return. +3. Run the test and let it fail. The failure message tells you what the code actually returns. +4. Change the assertion to expect the value the code produced. +5. Run again to confirm the test is now green. +6. Repeat for other inputs, branches, and edge cases. + +This observe-then-assert loop is the key insight: you do not need to understand the code to write these tests. The code itself tells you what it does. You are a reporter, not a specifier. + +Focus your tests on the areas you plan to change. Write as many cases as needed to feel confident that any unintended change in that area will show up as a failure. Concentrate especially on branches and paths that the refactoring will touch — extract, move, or inline operations are the riskiest. + +Many characterization tests look like "sunny day" tests. They do not explore special conditions or edge cases exhaustively. Their purpose is to verify that particular behaviors are present and connected correctly after the refactoring, not to probe the full contract of the code. + +## Finding Where to Test: Pinch Points + +Before writing tests, identify a pinch point: a place in the code where a small number of assertions can detect a wide range of changes. A pinch point is a natural encapsulation boundary — a method or interface through which all the effects of a cluster of changes are visible. + +Prefer interception points close to the change point. Every step between where you change code and where you observe the effect is a gap in which silent errors can hide. The fewer steps in that chain, the more confident you can be that a failing test actually points to your change. + +If a class is hard to instantiate directly (because it pulls in databases, services, or framework globals), test at a higher-level interception point that is easier to reach. Once the refactoring stabilizes those inner classes, you can add narrower tests and eventually remove the broader ones. + +## Golden Master / Approval Testing + +When the code produces large or complex output — a report, a rendered document, a serialized data structure — writing field-by-field assertions is impractical. Golden master testing (also called approval testing) handles this at scale. + +The process: + +- Run the code and capture its full output as a stored snapshot file (the "golden master" or "approved" file). +- The test passes by comparing the current output against the stored snapshot. Any difference fails the test. +- When a deliberate change in behavior is correct, you update the snapshot to the new output and commit it. + +Golden master testing is a characterization strategy, not a specification strategy. The snapshot records what the code did, not what it should do. It is especially useful for legacy report generators, template engines, serializers, and any code whose output is too large or variable to assert inline. + +The risk of snapshot tests is that they can encode bugs alongside correct behavior. If the original output was wrong, your test protects the wrong behavior. Treat golden master tests as a refactoring scaffold, not as a permanent specification. + +## Handling Side Effects and External Dependencies + +Code that writes to files, sends email, calls databases, or invokes external services cannot be tested directly without setting up or mocking those systems. Two approaches: + +**Sensing and separation.** Find a seam — a place where you can substitute the real collaborator with a fake one without editing the code under test. Inject the dependency through a constructor parameter, a method argument, or an interface. The fake records what the production code tried to do, so you can assert on that record rather than on the real side effect. + +**Higher-level interception.** If breaking the dependency is too invasive for now, test through a higher-level interface that you can observe. A class that writes to a file might also return a status object or emit a log entry that is easier to check. Use whatever surface is available. + +When sensing is genuinely impossible — the dependency is hard-coded, final, or sealed — write a thin wrapper around it, test through the wrapper, and break the hard-coded connection at the wrapper boundary. This is a mechanical move that does not change behavior. + +The important constraint: any dependency-breaking change you make before writing characterization tests must itself be low-risk and mechanical. Keep those preliminary moves minimal. Their only purpose is to get the code into the test harness; do not redesign at this stage. + +## When Characterization Tests Are Enough vs. When to Invest in Unit Tests + +Characterization tests at a pinch point give you a broad safety net but coarse-grained feedback. They tell you that something changed inside a cluster of classes, not which class or which line. That is often sufficient for a refactoring that extracts or moves code without changing logic. + +Invest in narrower unit tests when: + +- You are about to change logic, not just structure. +- You need to understand what each individual class is responsible for. +- The characterization tests run slowly and would break the feedback loop. +- The refactoring involves splitting a class — at that point, tests at the old pinch point become useless and tests at each new class are needed. + +The decision rule: characterization tests at the highest reachable pinch point are the cheapest way to start. Add narrower unit tests as you carve out and stabilize individual classes. Over time, the broad pinch-point tests become redundant and can be deleted. + +## When to Delete Characterization Tests + +Characterization tests are scaffolding, not permanent documentation. They exist to protect a specific refactoring and become a liability once that protection is no longer needed. + +Delete a characterization test when: + +- The class it was written for now has its own focused unit tests that cover the same paths. +- The refactoring is complete and the broad pinch-point test no longer covers any code path that a narrower test misses. +- The test is coupled to implementation details that you have since changed, causing it to break on unrelated future work. + +Tests that are too tightly coupled to code create exactly the problem they were meant to prevent: every improvement breaks the test, making change painful instead of safe. A characterization test that outlives its usefulness becomes a burden to every developer who touches that code. + +The signal to delete is not "the refactoring is done" but rather "the behavior this test captured is now fully covered by tests I trust more." Replace broad coverage with narrow, intention-revealing unit tests as the design improves. + +## Decision Rule Summary + +- Code with no tests and no imminent change: add no tests, make no structural changes. +- Code you need to refactor: write characterization tests at the nearest observable pinch point before touching anything. +- Side effects block you: make the minimum dependency-breaking move, then write characterization tests. +- Output is too large for inline assertions: use golden master / approval testing. +- Refactoring is structural only (extract, move, rename): characterization tests at a pinch point are sufficient. +- Refactoring changes logic: invest in narrower unit tests before and during the change. +- Refactoring is stable and unit tests are in place: delete the characterization tests. diff --git a/skills/refactoring-best-practices/references/code-smells.md b/skills/refactoring-best-practices/references/code-smells.md new file mode 100644 index 0000000..2af1d48 --- /dev/null +++ b/skills/refactoring-best-practices/references/code-smells.md @@ -0,0 +1,215 @@ +# Code Smells + +Use this reference when you need to name what is wrong with a piece of code and decide which refactoring move to apply first. + +## Long Method + +Recognition signals: +- The method body does not fit on a single screen +- You need to read the whole body before understanding what it does +- The method mixes several levels of abstraction in the same block +- Blank lines separate internally distinct responsibilities within the same method +- The method is a "monster": it has complex conditional logic, nested loops, and no extracted helpers (Working Effectively with Legacy Code, Ch. 22) + +What it means: +- The method is doing more than one thing +- Understanding and testing it in isolation becomes increasingly difficult + +Move to apply: Extract Method; Split Phase when the method has two sequential concerns + +--- + +## Large Class + +Recognition signals: +- The class has many instance variables that are not all used by the same methods +- Methods cluster into groups that are largely independent of each other +- You struggle to give the class a single, precise name +- Private methods accumulate that would make more sense as public methods on a smaller collaborator +- Testing a part of the class requires instantiating the whole thing (Working Effectively with Legacy Code, Ch. 20) + +What it means: +- The class carries more than one responsibility +- It will have multiple unrelated reasons to change + +Move to apply: Extract Class; Extract Interface / Protocol when callers only use a subset of the class + +--- + +## Feature Envy + +Recognition signals: +- A method calls several methods on another object, or accesses several of its fields, more than its own +- A method needs to pull data out of a collaborator before it can do its work +- A Service class contains all the logic while model classes hold only raw data (Codigo Sostenible, Ch. 6 — "Service se dedica a saquearles, porque no posee ningún dato propio — les envidia") + +What it means: +- The behavior belongs to the object whose data it uses, not to the class where it lives +- The current placement creates unnecessary coupling and weakens the collaborating object + +Move to apply: Move Method to the class whose data the method most uses + +--- + +## Data Clumps + +Recognition signals: +- The same group of two or more fields appears together in multiple class definitions +- The same set of parameters is repeated across several method signatures +- Removing one item from the group makes the remaining items meaningless on their own (refactorcotidiano, Ch. "Deja atrás lo primitivo") + +What it means: +- The group of data represents a concept that does not yet have a name in the code +- Validation and formatting rules for that concept are scattered + +Move to apply: Introduce Value Object; Introduce Parameter Object when the clump appears in method signatures + +--- + +## Primitive Obsession + +Recognition signals: +- Domain concepts are represented as raw strings, integers, or booleans +- Validation of a value is repeated wherever the value is used +- A parameter named `email`, `currency`, or `status` is typed as `string` or `int` +- Many methods take the same primitive argument and share the same conditional logic on it (99 Bottles of OOP, Ch. 5 — "Primitive Obsession is when you use one of these data types to represent a concept in your domain") +- The refactorcotidiano book calls this out explicitly: encapsulating a primitive is "Replace Data with Object" and yields consistency across the domain (refactorcotidiano, Ch. "Deja atrás lo primitivo") + +What it means: +- Domain rules live outside the domain concept they belong to +- The type system cannot enforce invariants that belong to the value + +Move to apply: Introduce Value Object + +--- + +## Shotgun Surgery + +Recognition signals: +- A single conceptual change requires edits in many unrelated classes +- Accessing a shared data structure directly from multiple call sites means every structural change ripples outward (refactorcotidiano, "si cambiamos la estructura de datos, tendremos que cambiar el código que la usa — un caso de Shotgun Surgery") +- Every new requirement means touching five files instead of one + +What it means: +- A single responsibility is spread across the codebase instead of being owned by one place +- The inverse of Divergent Change: one change, many classes affected + +Move to apply: Move Method and Move Field to consolidate scattered logic; Extract Class to create the missing owner + +--- + +## Divergent Change + +Recognition signals: +- The same class is modified for different, unrelated reasons across releases +- You can point to two distinct groups of methods in the class, each driven by a different external force +- Adding a new payment type requires changes in the same class as adding a new report format + +What it means: +- The class has more than one axis of variation, which means more than one responsibility +- The symmetric opposite of Shotgun Surgery: one class, many change reasons + +Move to apply: Extract Class so that each resulting class has a single reason to change + +--- + +## Middle Man + +Recognition signals: +- Most of the class's methods delegate directly to another object without adding logic +- Removing the class and calling the delegate directly would change nothing observable +- The class exists because it used to do more but was progressively emptied out + +What it means: +- The abstraction no longer earns its place +- Callers pay indirection cost without receiving encapsulation benefit + +Move to apply: Inline Method to remove the delegation layer; if the class still plays a role, consider whether it should absorb the delegate's behavior instead + +--- + +## Inappropriate Intimacy + +Recognition signals: +- A class accesses the internal data structure of another class directly rather than through its interface +- Two classes navigate each other's private fields or reach through each other's internals +- Accessing a collaborator's concrete structure creates coupling that propagates: "si accedemos a la estructura de datos directamente, estamos acoplando el código que la usa a la estructura de datos concreta — es un caso de Inappropriate Intimacy" (refactorcotidiano) +- One class reconstructs or replicates logic that belongs to another + +What it means: +- Encapsulation is broken between the two classes +- Changes to the internal representation of one class force changes in the other + +Move to apply: Move Method to migrate the behavior to where the data lives; Extract Interface / Protocol to hide the internal representation behind a stable boundary + +--- + +## Comments (as a symptom of unclear code) + +Recognition signals: +- A comment restates what the code already says in plain English +- A comment explains what a block does rather than why +- A blank line with no comment separates two blocks inside a method that each have their own purpose +- A comment was written to compensate for a poor name (refactorcotidiano, Ch. "Cuando los comentarios confunden" — comments become unnecessary when a method is given an expressive name) +- A comment is out of date and contradicts the code — a "lying comment" + +What it means: +- The code itself is not communicating its intent +- The comment is a workaround for a naming or structure problem, not a solution + +Move to apply: Rename the variable, method, or class so the comment becomes redundant; Extract Method to give the block a name that replaces the comment + +--- + +## Duplicate Code + +Recognition signals: +- Two methods contain the same block of logic, perhaps with minor variation +- The same conditional appears in several places and must be updated together +- A bug fixed in one location is found again in another because the fix was not propagated + +What it means: +- Knowledge is represented more than once; the second copy will drift from the first +- Every future change to the shared logic requires finding all copies + +Move to apply: Extract Method to create a single named version; Move Method when the duplicated logic belongs to a specific object; Replace Conditional with Polymorphism when the duplication is variation by type or role + +--- + +## Data Class + +Recognition signals: +- The class contains only private fields with public getters and setters for each +- No method on the class transforms, validates, or makes a decision using its own data +- Other classes reach into this class to retrieve data and perform operations with it elsewhere +- Codigo Sostenible calls this the "anemic model": "una clase que tiene una serie de campos privados, setters y getters para todos ellos, y nada más, es lo que se denomina modelo anémico" (Codigo Sostenible, Ch. 6) + +What it means: +- Behavior that belongs to the object is living outside it, in service or utility classes +- The class acts as a passive data container rather than an active participant in the domain +- Feature Envy in other classes is often a consequence of Data Class + +Move to apply: Move Method to transfer behavior into the class; Introduce Value Object if the class represents an immutable domain concept; remove setters where mutation is not needed to enforce invariants + +--- + +## Temporal Coupling in Version History + +Recognition signals: +- The same files repeatedly change together for one business rule +- A Value Object, validator/ensurer, exception, and use case move in lockstep +- Fixing one concept requires remembering several distant representations of the same knowledge + +What it means: +- Co-change can reveal duplicated knowledge or a responsibility split across the wrong boundaries +- The files may represent one concept that deserves a single owner +- The module or Aggregate boundary may not match the actual change boundary + +How to investigate: +- Inspect version history or a co-change matrix over representative feature commits +- Exclude generated files, formatting commits, bulk migrations, and mechanical renames +- Confirm the signal against domain language, invariants, ownership, and runtime consistency + +Move to apply: Move validation into the Value Object that owns intrinsic validity; Move Method behind the Aggregate Root for stateful rules; Extract Class or first-class collection when one concept is scattered. Treat temporal coupling as evidence, never as proof. + +Source: [CodelyTV/aggregates-course](https://github.com/CodelyTV/aggregates-course), temporal-coupling lesson and history examples. diff --git a/skills/refactoring-best-practices/references/domain-event-migration.md b/skills/refactoring-best-practices/references/domain-event-migration.md new file mode 100644 index 0000000..8f7f833 --- /dev/null +++ b/skills/refactoring-best-practices/references/domain-event-migration.md @@ -0,0 +1,46 @@ +# Introducing Domain Events into Legacy Code + +Source: migration lessons and counterexamples from [CodelyTV/domain_modeling-domain_events-course](https://github.com/CodelyTV/domain_modeling-domain_events-course). + +Introducing events changes coupling, timing, failure behavior, and often consistency. Treat it as an incremental behavior change, not a mechanical class extraction. + +## Migration Sequence + +1. Characterize the primary state change and every existing side effect. +2. Identify the business fact and give it a semantic past-tense name. +3. Introduce a narrow message port at an application seam with a recording fake or no-op adapter. +4. Record the fact at the Aggregate transition, or at the narrowest honest application seam when the model cannot yet change. +5. Keep primary persistence synchronous and unchanged. +6. Move one secondary reaction at a time into a directly tested subscriber. +7. Compare old and new paths before removing the direct call; dual-run only idempotent or safely suppressed effects. +8. Add transactional Outbox delivery before relying on subscribers for consistency. +9. Make consumers idempotent and prove retry/replay behavior. +10. Remove the old side-effect path only after parity and recovery are observable. + +Use a recording Event Bus as a sensing seam: drive the legacy entry point, observe database/message/email outputs, and assert the new fact without changing its delivery timing prematurely. + +## Preserve Semantics Deliberately + +Moving a direct call to asynchronous delivery changes when errors reach the caller. Document whether the command can succeed while the reaction is pending or failed, and add monitoring/recovery before making that change. + +Do not move the original Aggregate save into an ordinary subscriber. That changes the source-of-truth transaction and can acknowledge success before state is durable. + +Do not dual-run payments, emails, inventory decrements, or other non-idempotent effects without a deduplication key and suppression strategy. + +## Change Data Capture + +Use CDC when the writer cannot be changed or as a migration/anti-corruption bridge. Map a specific table plus mutation action to a stable integration message. + +CDC observes rows, not domain intent. A generic update may not reveal whether a user was archived, corrected, imported, or migrated. Compare old/new values when available, version mappings with schema evolution, preserve stable message identity across retries, and require idempotent consumers. + +Prefer an application-recorded Outbox once the writer can be modified. Do not rename raw row mutations as Domain Events indefinitely. + +## Red Flags + +- Big-bang event-driven rewrite. +- Event Bus injected into Entities or static publication from constructors. +- Event created manually in every use case after the transition. +- Primary persistence implemented as a derived subscriber action. +- Synchronous failures silently becoming eventual consistency. +- Old and new non-idempotent side effects running together. +- CDC contracts coupled directly to unstable table schemas. diff --git a/skills/refactoring-best-practices/references/error-contract-migration.md b/skills/refactoring-best-practices/references/error-contract-migration.md new file mode 100644 index 0000000..1ff4144 --- /dev/null +++ b/skills/refactoring-best-practices/references/error-contract-migration.md @@ -0,0 +1,33 @@ +# Migrating Error Contracts Safely + +Changing an exception, null, Result variant, status code, message, or throw timing can be externally observable. Treat error modernization as a behavior migration, not automatically as a refactoring. + +## Migration Sequence + +1. Characterize current success and every failure path: type, message, timing, side effects, logs, status, and response body. +2. Separate absence from outage and business rejection from validation/transport failure. +3. Introduce a typed internal failure alongside the old boundary contract. +4. Translate it at one adapter so external behavior remains stable. +5. Migrate callers from null/string/message parsing to type/code matching. +6. Introduce Result/Either only where failures are expected and callers compose/recover. +7. Add exhaustive mapping, redaction, and unknown-failure tests. +8. Change the public contract only as an explicit versioned behavior change. +9. Remove the compatibility adapter after all callers and contract tests migrate. + +## Preserve Failure Timing + +Moving validation earlier can prevent side effects; moving it later can leave partial state. Prove rejected operations remain atomic. A Result does not roll back writes performed before `error` is returned. + +## Preserve Diagnostics + +When translating vendor exceptions, preserve the original cause for logs/traces while exposing only a stable internal category. Do not replace every exception with a generic domain error and lose operational evidence. + +## Red Flags + +- Parsing exception messages during migration. +- Mapping database outage to not-found. +- Publishing class names as permanent API codes. +- Reflectively serializing all error properties. +- Big-bang conversion of every throw to a home-grown Result. +- Unchecked catch casts presented as exhaustive. +- Changing status/body/message without acceptance-contract tests. diff --git a/skills/refactoring-best-practices/references/fran-iglesias-refactoring-guidance.md b/skills/refactoring-best-practices/references/fran-iglesias-refactoring-guidance.md new file mode 100644 index 0000000..4a5a44f --- /dev/null +++ b/skills/refactoring-best-practices/references/fran-iglesias-refactoring-guidance.md @@ -0,0 +1,152 @@ +# Fran Iglesias Refactoring Guidance + +This reference distills practical ideas from Fran Iglesias's articles tagged "refactoring" into heuristics for everyday code improvement. +It focuses on safe incremental steps, recognizing when to act, using tests as a safety net, and moving behavior to the right place. +OOP design principles, TDD workflows as primary focus, and DDD strategic modeling belong in separate references. + +## Main Themes + +Across these articles, several ideas repeat consistently: + +- use code smells and metrics as objective signals, not subjective taste, to decide where to refactor +- extract behavior into value objects when primitives carry domain rules +- group data that travels together into a single concept +- decompose long methods by responsibility before looking for new classes +- shrink parameter lists by introducing parameter objects or value objects +- lift repeated conditions out of nested branches so each flow path is expressed once +- get tests in place before touching complex or legacy code — combinatory and snapshot techniques help reach coverage quickly +- refactoring immediately before or while making a change gives the best return on investment + +## Metrics-Driven Refactoring + +From `metric-driven-refactoring`: + +- refactoring is a cost-control tool, not an aesthetic exercise — maintainable code costs less to change regardless of whether a human or an AI agent does the work +- quality is measurable through structural and cognitive complexity, cohesion, and coupling — these are concrete, not subjective +- code smells and object-calisthenics rules serve as cheap proxies for precise metrics: they let you spot problem areas without running a measurement suite +- cognitive complexity (Campbell 2018) is more sensitive than cyclomatic complexity because it penalizes nesting depth in addition to branching count; a sawtooth left margin is the visual signal +- the Maintainability Index combines Halstead volume, cyclomatic complexity, and lines of code into a single score; very low scores flag units that deserve immediate attention +- OO-specific CK metrics (coupling between objects, response for a class, depth of inheritance) add a relational dimension that per-method metrics miss +- a practical workflow: run metrics, identify three high-impact refactors ordered by their effect on complexity and coupling, justify the time investment before starting + +Practical rule: + +- if you cannot articulate why a unit is hard to change beyond "it feels messy," run cognitive complexity and coupling metrics — the numbers will tell you which problem is real and which refactor to prioritize + +## Primitive Obsession + +From `primitive-obsession`: + +- primitive obsession occurs when domain concepts are modeled with raw language types, forcing validation, formatting, and business rules to scatter across the entire codebase +- scattered validation creates inconsistency: the same rule is written multiple times, drifts, and eventually produces bugs that are hard to trace +- the characteristic refactor is "Introduce Value Object": wrap the primitive, enforce invariants in the constructor or factory method, and let the object carry its own behavior +- a private constructor combined with a named factory (`Amount.valid(...)`) makes it impossible to create an invalid instance; callers cannot bypass the rule +- once the value object exists, domain-specific behavior (formatting, comparison, conversion) migrates into it naturally instead of living in services or utilities +- primitive obsession and data clump often appear together; recognize the difference — primitive obsession is about a single value that carries rules, data clump is about several values that always travel together + +Practical rule: + +- if you have to write the same validation for a field in more than one place, or if formatting logic branches on what "kind" a value is, wrap the value in an object and put the rule there + +## Data Clump + +From `data-clump`: + +- a data clump is a group of fields that travel together through constructors, method signatures, and class fields — they are signaling an unnamed concept +- the smell does not cause bugs immediately, but every future change that affects those fields must be applied in multiple places, creating drift and inconsistency +- the refactor is "Introduce Value Object": identify the fields that belong together, create a class for them, give it a meaningful name, and let it carry any behavior that touches only those fields +- once the value object exists, adding new fields or changing formatting is a one-place change instead of a cascade — this is the concrete payoff +- value objects should attract behavior: if a method only uses fields from the value object, it belongs inside the value object + +Practical rule: + +- if the same three fields appear together in two or more constructors or method signatures, name the concept they represent and introduce a class for it + +## Long Method + +From `long-method`: + +- a long method is doing several things at once; the first symptom is mixed levels of abstraction within the same method body +- the visual signal: comment blocks that group related lines are already candidate method extractions waiting to happen +- the refactoring sequence is: first "Extract Method" to isolate each responsibility into a private helper with a meaningful name; then look at what those helpers need and whether some naturally belong on separate collaborator classes ("Extract Class") +- extracting private methods clarifies the main method by hiding detail and making the high-level flow readable at a glance; only after that does "Extract Class" become safe because responsibilities are already named and bounded +- before extracting from complex legacy code, establish a safety net — even a single characterization test that exercises the main path is enough to start + +Practical rule: + +- if reading a method requires mentally tracking what "phase" you are in, extract each phase into a named private method; if extracted methods share more context with each other than with the parent class, extract a class + +## Long Parameter List + +From `long-parameter-list`: + +- more than three or four parameters overloads working memory; positional parameters of the same type are also fragile because swapping two silently produces wrong results +- adding optional parameters with default values is a short-term fix that compounds future cost — every new optional parameter makes the list harder to document and test +- three refactors address this smell: + - "Introduce Value Object" when a subset of the parameters represents a domain concept with its own rules (the tightest fix) + - "Introduce Parameter Object" when the parameters lack that conceptual bond but the signature still needs to be stable and manageable + - "Builder" when construction is complex and the object under construction needs a readable creation language +- parameter objects and value objects act as a shield: when the method's inputs change, you update the object rather than hunting for every call site + +Practical rule: + +- if adding a new parameter would push the list beyond four, first ask whether any existing parameters belong together as a concept; if yes, introduce a value object; if no, introduce a parameter object to keep the signature stable + +## Uplift Conditional + +From `a_case_for_uplift_conditional`: + +- when the same condition appears in multiple nested or sequential branches, the code is expressing two tangled flows in one block instead of separating them +- "Uplift Conditional" pulls the dominating condition to the top level, producing two independent and flat code paths — one per branch of the lifted condition +- the technique is safe even without comprehensive test coverage: each small step (extract method, lift condition, merge duplicated branches) is individually obvious and preserves behavior +- the payoff is that each flow has a single location to modify; meaningful names can be applied per context; the behavior of each path is easy to reason about independently +- before applying, extract the tangled block into its own method to create a clear boundary; then lift the condition inside that method + +Practical rule: + +- if the same boolean condition appears in three or more branches of a single function, lift it to the top so the function has two flat sections; the duplication inside each section will then be easy to see and eliminate + +## Golden Master and Approval Testing for Legacy Code + +From `golden-cookbook-master-approval`: + +- refactoring legacy code without tests is unsafe; the Golden Master technique provides a fast path to meaningful coverage even when business rules are opaque and scattered +- the approach: run the system under test with a large number of parameter combinations, capture all outputs as a snapshot, and treat any future deviation as a failing test +- "approval mode" keeps the snapshot always regenerating so you can examine output and tune the input combinations until you reach 100% code coverage; then approve and switch to normal snapshot mode +- once 100% branch coverage is confirmed, any refactor that changes observable behavior breaks at least one test — this is the safety net +- combinatory input generation replaces hand-crafted test cases: enumerate the distinct values for each input dimension and let the framework produce all combinations + +Practical rule: + +- before touching any legacy code that has no tests and complex conditional logic, use combinatory snapshot testing to get to full branch coverage first; only then start extracting and simplifying + +## Economics of Refactoring and the 3P Pattern + +From `Some-economics-of-refactoring` and `Breaking-out-of-legacy-with-3P`: + +- the return on investment from refactoring is highest when it happens immediately before the change that benefits from it; delaying it to a "cleanup sprint" defers the benefit and makes it harder to justify +- the 3P pattern (Protect → Prepare → Produce) makes opportunistic refactoring part of the normal feature workflow: + - Protect: add tests that capture existing behavior before any change + - Prepare: refactor so the new feature can be introduced cleanly + - Produce: implement the feature with TDD +- the developer who writes the protection tests is the first beneficiary of those tests; they understand the code best at that moment and can spot ill-conceived tests immediately +- this pattern keeps refactoring small and focused — only the code directly relevant to the current story is improved, preventing the scope from becoming a rewrite + +Practical rule: + +- do not start a feature in messy code without first protecting existing behavior with tests and preparing the structure to receive the change; the protection step is not overhead — it is how you avoid breaking things while delivering + +## Everyday Refactoring Heuristics + +When improving code, try this order: + +1. make the problem objective — run cognitive complexity and coupling metrics or apply a code smell checklist before deciding where to act +2. get tests in place first — use characterization tests, Golden Master, or combinatory snapshot testing to create a safety net before touching the code +3. name the concept precisely — rename until the code explains itself without comments +4. eliminate primitive obsession — wrap any value that carries its own rules or formatting in a value object +5. collapse data clumps — identify fields that always travel together, give the concept a name, introduce a class +6. decompose long methods by responsibility — extract named private methods first, then extract classes from clusters of methods that belong together +7. shorten parameter lists — replace related parameters with value objects; replace unrelated ones with a parameter object or builder +8. lift repeated conditions — pull the dominant boolean to the top so each code path is flat and expressed once +9. refactor opportunistically, not in bulk — apply the 3P pattern so each story cleans only what it touches +10. use tests as a wall at your back — run the full suite after every small step; roll back immediately if anything turns red diff --git a/skills/refactoring-best-practices/references/language-examples.md b/skills/refactoring-best-practices/references/language-examples.md new file mode 100644 index 0000000..c10b62b --- /dev/null +++ b/skills/refactoring-best-practices/references/language-examples.md @@ -0,0 +1,1224 @@ +# Language Examples + +These examples show a refactoring move from branching logic toward role-based collaboration. + +## Before + +```typescript +function discountAmount(customerType: string, subtotalInCents: number): number { + if (customerType === 'premium') { + return Math.round(subtotalInCents * 0.8) + } + + if (customerType === 'partner') { + return Math.round(subtotalInCents * 0.85) + } + + return subtotalInCents +} +``` + +## After in TypeScript + +```typescript +interface PricingPolicy { + apply(subtotalInCents: number): number +} + +class StandardPricing implements PricingPolicy { + apply(subtotalInCents: number): number { + return subtotalInCents + } +} + +class PremiumPricing implements PricingPolicy { + apply(subtotalInCents: number): number { + return Math.round(subtotalInCents * 0.8) + } +} + +class PartnerPricing implements PricingPolicy { + apply(subtotalInCents: number): number { + return Math.round(subtotalInCents * 0.85) + } +} +``` + +## After in Go + +```go +type PricingPolicy interface { + Apply(subtotalInCents int) int +} + +type StandardPricing struct{} +type PremiumPricing struct{} +type PartnerPricing struct{} + +func (StandardPricing) Apply(s int) int { return s } +func (PremiumPricing) Apply(s int) int { return s * 8 / 10 } +func (PartnerPricing) Apply(s int) int { return s * 85 / 100 } +``` + +## After in Rust + +```rust +pub trait PricingPolicy { fn apply(&self, subtotal_in_cents: i64) -> i64; } + +pub struct StandardPricing; +pub struct PremiumPricing; +pub struct PartnerPricing; + +impl PricingPolicy for StandardPricing { fn apply(&self, s: i64) -> i64 { s } } +impl PricingPolicy for PremiumPricing { fn apply(&self, s: i64) -> i64 { s * 8 / 10 } } +impl PricingPolicy for PartnerPricing { fn apply(&self, s: i64) -> i64 { s * 85 / 100 } } +``` + +## After in Java + +```java +public interface PricingPolicy { + int apply(int subtotalInCents); +} + +public final class StandardPricing implements PricingPolicy { + public int apply(int subtotalInCents) { + return subtotalInCents; + } +} + +public final class PremiumPricing implements PricingPolicy { + public int apply(int subtotalInCents) { + return Math.round(subtotalInCents * 0.8f); + } +} +``` + +## After in Python + +```python +class PricingPolicy: + def apply(self, subtotal_in_cents: int) -> int: + raise NotImplementedError + + +class StandardPricing(PricingPolicy): + def apply(self, subtotal_in_cents: int) -> int: + return subtotal_in_cents + + +class PremiumPricing(PricingPolicy): + def apply(self, subtotal_in_cents: int) -> int: + return round(subtotal_in_cents * 0.8) +``` + +## After in C# + +```csharp +public interface IPricingPolicy +{ + int Apply(int subtotalInCents); +} + +public sealed class StandardPricing : IPricingPolicy +{ + public int Apply(int subtotalInCents) + { + return subtotalInCents; + } +} + +public sealed class PremiumPricing : IPricingPolicy +{ + public int Apply(int subtotalInCents) + { + return (int)Math.Round(subtotalInCents * 0.8); + } +} +``` + +## After in Ruby + +```ruby +class StandardPricing + def apply(subtotal_in_cents) + subtotal_in_cents + end +end + +class PremiumPricing + def apply(subtotal_in_cents) + (subtotal_in_cents * 0.8).round + end +end +``` + +## After in PHP + +```php +interface PricingPolicy +{ + public function apply(int $subtotalInCents): int; +} + +final class StandardPricing implements PricingPolicy +{ + public function apply(int $subtotalInCents): int + { + return $subtotalInCents; + } +} + +final class PremiumPricing implements PricingPolicy +{ + public function apply(int $subtotalInCents): int + { + return (int) round($subtotalInCents * 0.8); + } +} +``` + +## Extract Method + +Use this move when a method mixes several levels of abstraction or contains a block whose purpose can be captured in a single clear name. + +### Before + +```typescript +function printOrderSummary(order: Order): void { + console.log(`Order: ${order.id()}`) + console.log(`Date: ${order.date()}`) + + for (const item of order.items()) { + console.log(` ${item.name()}: ${item.price().value()}`) + } + + console.log(`Total: ${order.items().total().value()}`) +} +``` + +### After in TypeScript + +```typescript +function printOrderSummary(order: Order): void { + printHeader(order) + printItems(order) + printTotal(order) +} + +function printHeader(order: Order): void { + console.log(`Order: ${order.id()}`) + console.log(`Date: ${order.date()}`) +} + +function printItems(order: Order): void { + for (const item of order.items()) { + console.log(` ${item.name()}: ${item.price().value()}`) + } +} + +function printTotal(order: Order): void { + console.log(`Total: ${order.items().total().value()}`) +} +``` + +### After in Go + +```go +func printOrderSummary(order Order) { + printHeader(order) + printItems(order) + printTotal(order) +} + +func printHeader(order Order) { + fmt.Printf("Order: %s\n", order.ID()) + fmt.Printf("Date: %s\n", order.Date()) +} + +func printItems(order Order) { + for _, item := range order.Items() { + fmt.Printf(" %s: %d\n", item.Name(), item.Price().Value()) + } +} + +func printTotal(order Order) { + fmt.Printf("Total: %d\n", order.Items().Total().Value()) +} +``` + +### After in Rust + +```rust +fn print_order_summary(order: &Order) { + print_header(order); + print_items(order); + print_total(order); +} + +fn print_header(order: &Order) { + println!("Order: {}", order.id()); + println!("Date: {}", order.date()); +} + +fn print_items(order: &Order) { + for item in order.items() { + println!(" {}: {}", item.name(), item.price().value()); + } +} + +fn print_total(order: &Order) { + println!("Total: {}", order.items().total().value()); +} +``` + +### After in Java + +```java +void printOrderSummary(Order order) { + printHeader(order); + printItems(order); + printTotal(order); +} + +private void printHeader(Order order) { + System.out.printf("Order: %s%n", order.id()); + System.out.printf("Date: %s%n", order.date()); +} + +private void printItems(Order order) { + for (OrderItem item : order.items()) { + System.out.printf(" %s: %s%n", item.name(), item.price().value()); + } +} + +private void printTotal(Order order) { + System.out.printf("Total: %s%n", order.items().total().value()); +} +``` + +### After in Python + +```python +def print_order_summary(order: Order) -> None: + print_header(order) + print_items(order) + print_total(order) + + +def print_header(order: Order) -> None: + print(f"Order: {order.id()}") + print(f"Date: {order.date()}") + + +def print_items(order: Order) -> None: + for item in order.items(): + print(f" {item.name()}: {item.price().value()}") + + +def print_total(order: Order) -> None: + print(f"Total: {order.items().total().value()}") +``` + +### After in C# + +```csharp +void PrintOrderSummary(Order order) +{ + PrintHeader(order); + PrintItems(order); + PrintTotal(order); +} + +private void PrintHeader(Order order) +{ + Console.WriteLine($"Order: {order.Id()}"); + Console.WriteLine($"Date: {order.Date()}"); +} + +private void PrintItems(Order order) +{ + foreach (var item in order.Items()) + { + Console.WriteLine($" {item.Name()}: {item.Price().Value()}"); + } +} + +private void PrintTotal(Order order) +{ + Console.WriteLine($"Total: {order.Items().Total().Value()}"); +} +``` + +### After in Ruby + +```ruby +def print_order_summary(order) + print_header(order) + print_items(order) + print_total(order) +end + +def print_header(order) + puts "Order: #{order.id}" + puts "Date: #{order.date}" +end + +def print_items(order) + order.items.each do |item| + puts " #{item.name}: #{item.price.value}" + end +end + +def print_total(order) + puts "Total: #{order.items.total.value}" +end +``` + +### After in PHP + +```php +function printOrderSummary(Order $order): void +{ + printHeader($order); + printItems($order); + printTotal($order); +} + +function printHeader(Order $order): void +{ + echo "Order: {$order->id()}\n"; + echo "Date: {$order->date()}\n"; +} + +function printItems(Order $order): void +{ + foreach ($order->items() as $item) { + echo " {$item->name()}: {$item->price()->value()}\n"; + } +} + +function printTotal(Order $order): void +{ + echo "Total: {$order->items()->total()->value()}\n"; +} +``` + +--- + +## Extract Class + +Use this move when a class carries two distinct clusters of data and behavior that have different reasons to change. + +### Before + +```typescript +class Order { + constructor( + private readonly orderId: string, + private readonly customerName: string, + private readonly customerEmail: string, + private readonly items: OrderItem[], + ) {} + + id(): string { return this.orderId } + customerName(): string { return this.customerName } + customerEmail(): string { return this.customerEmail } + itemCount(): number { return this.items.length } +} +``` + +### After in TypeScript + +```typescript +class Customer { + constructor( + private readonly _name: string, + private readonly _email: string, + ) {} + + name(): string { return this._name } + email(): string { return this._email } +} + +class Order { + constructor( + private readonly orderId: string, + private readonly _customer: Customer, + private readonly items: OrderItem[], + ) {} + + id(): string { return this.orderId } + customer(): Customer { return this._customer } + itemCount(): number { return this.items.length } +} +``` + +### After in Go + +```go +type Customer struct { + name string + email string +} + +func (c Customer) Name() string { return c.name } +func (c Customer) Email() string { return c.email } + +type Order struct { + id string + customer Customer + items []OrderItem +} + +func (o Order) ID() string { return o.id } +func (o Order) Customer() Customer { return o.customer } +func (o Order) ItemCount() int { return len(o.items) } +``` + +### After in Rust + +```rust +pub struct Customer { name: String, email: String } + +impl Customer { + pub fn name(&self) -> &str { &self.name } + pub fn email(&self) -> &str { &self.email } +} + +pub struct Order { id: String, customer: Customer, items: Vec } + +impl Order { + pub fn id(&self) -> &str { &self.id } + pub fn customer(&self) -> &Customer { &self.customer } + pub fn item_count(&self) -> usize { self.items.len() } +} +``` + +### After in Java + +```java +public final class Customer { + private final String name; + private final String email; + + public Customer(String name, String email) { + this.name = name; + this.email = email; + } + + public String name() { return name; } + public String email() { return email; } +} + +public final class Order { + private final String orderId; + private final Customer customer; + private final List items; + + public Order(String orderId, Customer customer, List items) { + this.orderId = orderId; + this.customer = customer; + this.items = items; + } + + public String id() { return orderId; } + public Customer customer() { return customer; } + public int itemCount() { return items.size(); } +} +``` + +### After in Python + +```python +class Customer: + def __init__(self, name: str, email: str) -> None: + self._name = name + self._email = email + + def name(self) -> str: + return self._name + + def email(self) -> str: + return self._email + + +class Order: + def __init__(self, order_id: str, customer: Customer, items: list[OrderItem]) -> None: + self._order_id = order_id + self._customer = customer + self._items = items + + def id(self) -> str: + return self._order_id + + def customer(self) -> Customer: + return self._customer + + def item_count(self) -> int: + return len(self._items) +``` + +### After in C# + +```csharp +public sealed class Customer +{ + public Customer(string name, string email) + { + Name = name; + Email = email; + } + + public string Name { get; } + public string Email { get; } +} + +public sealed class Order +{ + public Order(string orderId, Customer customer, IReadOnlyList items) + { + Id = orderId; + Customer = customer; + Items = items; + } + + public string Id { get; } + public Customer Customer { get; } + public int ItemCount => Items.Count; + private IReadOnlyList Items { get; } +} +``` + +### After in Ruby + +```ruby +class Customer + attr_reader :name, :email + + def initialize(name, email) + @name = name + @email = email + end +end + +class Order + attr_reader :id, :customer + + def initialize(order_id, customer, items) + @id = order_id + @customer = customer + @items = items + end + + def item_count + @items.length + end +end +``` + +### After in PHP + +```php +final class Customer +{ + public function __construct( + private readonly string $name, + private readonly string $email, + ) {} + + public function name(): string { return $this->name; } + public function email(): string { return $this->email; } +} + +final class Order +{ + public function __construct( + private readonly string $orderId, + private readonly Customer $customer, + private readonly array $items, + ) {} + + public function id(): string { return $this->orderId; } + public function customer(): Customer { return $this->customer; } + public function itemCount(): int { return count($this->items); } +} +``` + +--- + +## Introduce Value Object + +Use this move when validation logic for a domain concept is duplicated across multiple callers. + +### Before + +```typescript +function registerUser(email: string): void { + if (!email.includes('@')) throw new Error('Invalid email') + userRepository.save(new User(email)) +} + +function sendNewsletter(email: string): void { + if (!email.includes('@')) throw new Error('Invalid email') + mailer.send(email, 'Newsletter content') +} +``` + +### After in TypeScript + +```typescript +class EmailAddress { + constructor(private readonly value: string) { + if (!value.includes('@')) throw new Error('Invalid email address') + } + + toString(): string { + return this.value + } +} + +function registerUser(email: EmailAddress): void { + userRepository.save(new User(email)) +} + +function sendNewsletter(email: EmailAddress): void { + mailer.send(email.toString(), 'Newsletter content') +} +``` + +### After in Go + +```go +type EmailAddress struct{ value string } + +func NewEmailAddress(value string) (EmailAddress, error) { + if !strings.Contains(value, "@") { + return EmailAddress{}, errors.New("invalid email address") + } + return EmailAddress{value: value}, nil +} + +func (e EmailAddress) String() string { return e.value } + +func registerUser(email EmailAddress) { userRepository.save(NewUser(email)) } +func sendNewsletter(email EmailAddress) { mailer.send(email.String(), "Newsletter content") } +``` + +### After in Rust + +```rust +pub struct EmailAddress(String); + +impl EmailAddress { + pub fn new(value: impl Into) -> Result { + let v = value.into(); + if !v.contains('@') { return Err("invalid email address"); } + Ok(Self(v)) + } +} + +impl std::fmt::Display for EmailAddress { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { write!(f, "{}", self.0) } +} + +fn register_user(email: EmailAddress) { user_repository.save(User::new(email)); } +fn send_newsletter(email: EmailAddress) { mailer.send(&email.to_string(), "Newsletter content"); } +``` + +### After in Java + +```java +public final class EmailAddress { + private final String value; + + public EmailAddress(String value) { + if (!value.contains("@")) throw new IllegalArgumentException("Invalid email address"); + this.value = value; + } + + @Override + public String toString() { return value; } +} + +void registerUser(EmailAddress email) { + userRepository.save(new User(email)); +} + +void sendNewsletter(EmailAddress email) { + mailer.send(email.toString(), "Newsletter content"); +} +``` + +### After in Python + +```python +class EmailAddress: + def __init__(self, value: str) -> None: + if '@' not in value: + raise ValueError("Invalid email address") + self._value = value + + def __str__(self) -> str: + return self._value + + +def register_user(email: EmailAddress) -> None: + user_repository.save(User(email)) + + +def send_newsletter(email: EmailAddress) -> None: + mailer.send(str(email), "Newsletter content") +``` + +### After in C# + +```csharp +public sealed class EmailAddress +{ + private readonly string _value; + + public EmailAddress(string value) + { + if (!value.Contains('@')) throw new ArgumentException("Invalid email address"); + _value = value; + } + + public override string ToString() => _value; +} + +void RegisterUser(EmailAddress email) +{ + userRepository.Save(new User(email)); +} + +void SendNewsletter(EmailAddress email) +{ + mailer.Send(email.ToString(), "Newsletter content"); +} +``` + +### After in Ruby + +```ruby +class EmailAddress + def initialize(value) + raise ArgumentError, 'Invalid email address' unless value.include?('@') + @value = value + end + + def to_s + @value + end +end + +def register_user(email) + user_repository.save(User.new(email)) +end + +def send_newsletter(email) + mailer.send(email.to_s, 'Newsletter content') +end +``` + +### After in PHP + +```php +final class EmailAddress +{ + private string $value; + + public function __construct(string $value) + { + if (!str_contains($value, '@')) { + throw new InvalidArgumentException('Invalid email address'); + } + $this->value = $value; + } + + public function __toString(): string + { + return $this->value; + } +} + +function registerUser(EmailAddress $email): void +{ + $userRepository->save(new User($email)); +} + +function sendNewsletter(EmailAddress $email): void +{ + $mailer->send((string) $email, 'Newsletter content'); +} +``` + +--- + +## Move Method + +Use this move when a method uses more data from another object than from its own class. + +### Before + +```typescript +class Rental { + constructor( + private readonly movie: Movie, + private readonly daysRented: number, + ) {} + + charge(): number { + if (this.movie.isPremium()) { + return this.daysRented * 3.5 + } + return this.daysRented * 2 + } +} +``` + +### After in TypeScript + +```typescript +class Movie { + constructor(private readonly premium: boolean) {} + + dailyRate(): number { + return this.premium ? 3.5 : 2 + } +} + +class Rental { + constructor( + private readonly movie: Movie, + private readonly daysRented: number, + ) {} + + charge(): number { + return this.movie.dailyRate() * this.daysRented + } +} +``` + +### After in Go + +```go +type Movie struct{ premium bool } +func (m Movie) DailyRate() float64 { if m.premium { return 3.5 }; return 2.0 } + +type Rental struct { movie Movie; daysRented int } +func (r Rental) Charge() float64 { return r.movie.DailyRate() * float64(r.daysRented) } +``` + +### After in Rust + +```rust +pub struct Movie { premium: bool } +impl Movie { pub fn daily_rate(&self) -> f64 { if self.premium { 3.5 } else { 2.0 } } } + +pub struct Rental { movie: Movie, days_rented: u32 } +impl Rental { pub fn charge(&self) -> f64 { self.movie.daily_rate() * self.days_rented as f64 } } +``` + +### After in Java + +```java +public final class Movie { + private final boolean premium; + + public Movie(boolean premium) { + this.premium = premium; + } + + public double dailyRate() { + return premium ? 3.5 : 2.0; + } +} + +public final class Rental { + private final Movie movie; + private final int daysRented; + + public Rental(Movie movie, int daysRented) { + this.movie = movie; + this.daysRented = daysRented; + } + + public double charge() { + return movie.dailyRate() * daysRented; + } +} +``` + +### After in Python + +```python +class Movie: + def __init__(self, premium: bool) -> None: + self._premium = premium + + def daily_rate(self) -> float: + return 3.5 if self._premium else 2.0 + + +class Rental: + def __init__(self, movie: Movie, days_rented: int) -> None: + self._movie = movie + self._days_rented = days_rented + + def charge(self) -> float: + return self._movie.daily_rate() * self._days_rented +``` + +### After in C# + +```csharp +public sealed class Movie +{ + private readonly bool _premium; + + public Movie(bool premium) => _premium = premium; + + public double DailyRate() => _premium ? 3.5 : 2.0; +} + +public sealed class Rental +{ + private readonly Movie _movie; + private readonly int _daysRented; + + public Rental(Movie movie, int daysRented) + { + _movie = movie; + _daysRented = daysRented; + } + + public double Charge() => _movie.DailyRate() * _daysRented; +} +``` + +### After in Ruby + +```ruby +class Movie + def initialize(premium) + @premium = premium + end + + def daily_rate + @premium ? 3.5 : 2.0 + end +end + +class Rental + def initialize(movie, days_rented) + @movie = movie + @days_rented = days_rented + end + + def charge + @movie.daily_rate * @days_rented + end +end +``` + +### After in PHP + +```php +final class Movie +{ + public function __construct(private readonly bool $premium) {} + + public function dailyRate(): float + { + return $this->premium ? 3.5 : 2.0; + } +} + +final class Rental +{ + public function __construct( + private readonly Movie $movie, + private readonly int $daysRented, + ) {} + + public function charge(): float + { + return $this->movie->dailyRate() * $this->daysRented; + } +} +``` + +--- + +## Replace Temp with Query + +Use this move when a temporary variable stores a computation that can be extracted into a named method, making the intent readable without comments. + +### Before + +```typescript +class Cart { + calculateTotal(): number { + const base = this.items.reduce((sum, item) => sum + item.price(), 0) + const discount = base > 100 ? base * 0.1 : 0 + return base - discount + } +} +``` + +### After in TypeScript + +```typescript +class Cart { + calculateTotal(): number { + return this.baseAmount() - this.discount() + } + + private baseAmount(): number { + return this.items.reduce((sum, item) => sum + item.price(), 0) + } + + private discount(): number { + return this.baseAmount() > 100 ? this.baseAmount() * 0.1 : 0 + } +} +``` + +### After in Go + +```go +type Cart struct{ items []CartItem } + +func (c Cart) CalculateTotal() float64 { return c.baseAmount() - c.discount() } +func (c Cart) baseAmount() float64 { s := 0.0; for _, i := range c.items { s += i.Price() }; return s } +func (c Cart) discount() float64 { if c.baseAmount() > 100 { return c.baseAmount() * 0.1 }; return 0 } +``` + +### After in Rust + +```rust +pub struct Cart { items: Vec } + +impl Cart { + pub fn calculate_total(&self) -> f64 { self.base_amount() - self.discount() } + fn base_amount(&self) -> f64 { self.items.iter().map(|i| i.price()).sum() } + fn discount(&self) -> f64 { if self.base_amount() > 100.0 { self.base_amount() * 0.1 } else { 0.0 } } +} +``` + +### After in Java + +```java +public class Cart { + private final List items; + + public Cart(List items) { + this.items = items; + } + + public double calculateTotal() { + return baseAmount() - discount(); + } + + private double baseAmount() { + return items.stream().mapToDouble(CartItem::price).sum(); + } + + private double discount() { + return baseAmount() > 100 ? baseAmount() * 0.1 : 0; + } +} +``` + +### After in Python + +```python +class Cart: + def __init__(self, items: list[CartItem]) -> None: + self._items = items + + def calculate_total(self) -> float: + return self._base_amount() - self._discount() + + def _base_amount(self) -> float: + return sum(item.price() for item in self._items) + + def _discount(self) -> float: + return self._base_amount() * 0.1 if self._base_amount() > 100 else 0 +``` + +### After in C# + +```csharp +public class Cart +{ + private readonly IReadOnlyList _items; + + public Cart(IReadOnlyList items) => _items = items; + + public double CalculateTotal() => BaseAmount() - Discount(); + + private double BaseAmount() => _items.Sum(item => item.Price()); + + private double Discount() => BaseAmount() > 100 ? BaseAmount() * 0.1 : 0; +} +``` + +### After in Ruby + +```ruby +class Cart + def initialize(items) + @items = items + end + + def calculate_total + base_amount - discount + end + + private + + def base_amount + @items.sum(&:price) + end + + def discount + base_amount > 100 ? base_amount * 0.1 : 0 + end +end +``` + +### After in PHP + +```php +class Cart +{ + public function __construct(private readonly array $items) {} + + public function calculateTotal(): float + { + return $this->baseAmount() - $this->discount(); + } + + private function baseAmount(): float + { + return array_sum(array_map(fn($item) => $item->price(), $this->items)); + } + + private function discount(): float + { + return $this->baseAmount() > 100 ? $this->baseAmount() * 0.1 : 0; + } +} +``` + +--- + +## What to Notice + +- The variation becomes explicit. +- Adding a new pricing rule no longer requires editing a central conditional. +- The refactor is safest when protected by tests that captured the old behavior first. +- The same refactoring move works across mainstream object-oriented languages. +- Each move addresses a specific structural problem — match the smell to the move before refactoring. diff --git a/skills/refactoring-best-practices/references/legacy-code-techniques.md b/skills/refactoring-best-practices/references/legacy-code-techniques.md new file mode 100644 index 0000000..6724ff5 --- /dev/null +++ b/skills/refactoring-best-practices/references/legacy-code-techniques.md @@ -0,0 +1,211 @@ +# Legacy Code Techniques + +Use this reference when making changes to code that has no tests and where dependencies are hard to break. + +## The Core Problem: Legacy Code + +Legacy code — code without tests — is risky to change because there is no safety net. The two obstacles that come up most often are: + +- Difficulty instantiating objects in a test harness due to tangled construction dependencies. +- Difficulty running methods in a test harness due to hidden side effects. + +The goal is not to produce ideal design immediately. The goal is to make the next safe step possible: get a small piece of code under test, change it with confidence, and leave the system slightly more testable than you found it. + +## Sensing and Separation + +Two distinct reasons to break dependencies: + +- **Sensing** — break a dependency so you can observe what the code actually computes. Use this when you need to verify that a value was set, a call was made, or a side effect occurred, but the code gives you no way to see it from the outside. +- **Separation** — break a dependency so you can get a piece of code into a test harness at all, even if you do not care about sensing its effects. Use this when the code cannot compile or run in isolation because it pulls in too much of the system. + +Often you need both: you separate to get the code into a harness, then sense to verify behavior. + +## The Seam Model + +A seam is a place in the code where you can substitute one behavior for another without editing the code at that exact location. Every seam has an enabling point — the place where you make the choice to use one behavior or another. + +Three types of seams: + +- **Preprocessing seam** — a macro or conditional compilation directive that can replace a call before the compiler sees it. The enabling point is the build flag or include directive. +- **Link seam** — a function or class that can be replaced by pointing the linker or classpath at a different implementation. The enabling point is the makefile, build script, or classpath setting. +- **Object seam** — a method call on an object where the actual method executed depends on the runtime type. The enabling point is the place where you decide which object to create or pass. This is the most useful seam in object-oriented languages. + +When a method call is made through a reference that can be varied — because it is passed as an argument, assigned from outside, or resolved through polymorphism — it is an object seam. When the object is constructed inside the same method that calls it, there is no enabling point and no seam. + +## Faking Collaborators + +To sense or separate, you often need to replace a real dependency with a fake. Fakes implement the same interface as the real collaborator but behave in a way that is controlled during the test. A fake has two sides: the side the class under test sees (the interface it expects), and the side the test sees (inspection methods that reveal what happened). + +Use fakes when: + +- A collaborator has side effects you cannot afford in a test (database writes, network calls, file system access). +- A collaborator produces values you need to control (clocks, random number generators, external APIs). +- You need to verify that a call was made with specific arguments. + +## The Legacy Code Change Algorithm + +When making any change in a legacy code base, follow this sequence: + +1. **Identify change points** — find exactly where the code needs to change. If the design is unclear, explore it before cutting. +2. **Find test points** — find the places where you can write tests that will detect whether the change was made correctly or broke existing behavior. Test points are often close to the change points but not always the same. +3. **Break dependencies** — use the techniques below to get the code into a harness. Accept that the first incisions may leave the code looking slightly worse. The goal is a safe path into the code, not an ideal design. +4. **Write tests** — write characterization tests that pin the existing behavior, and write new tests that specify the intended change. Characterization tests document what the code actually does, not what you wish it did. +5. **Make changes and refactor** — with tests in place, make the change using test-driven development. Once the new behavior is covered, look for small refactoring opportunities in the surrounding code. + +Breaking dependencies to get tests in place is different from refactoring. The dependency-breaking steps are done without tests protecting them; they must be done conservatively, with as few edits as possible, to minimize the chance of introducing new errors. + +## Sprout Method + +Use Sprout Method when you need to add new behavior and the new behavior can be expressed as a self-contained sequence of statements that does not need to be woven into the existing logic. + +How it works: write the new behavior in a new, separately testable method. Call that method from the existing legacy method. Do not edit the legacy logic itself. + +Steps: +1. Identify where the new behavior needs to be triggered in the existing method. +2. Write a call to a new method at that point and comment it out. +3. Identify what data the new method needs from the existing method and pass those values as arguments. +4. Determine whether the new method needs to return a value back to the existing method; if so, assign the return value to a variable. +5. Develop the new method using test-driven development. +6. Uncomment the call. + +When to use it: +- The new behavior is a distinct piece of work with a clear boundary. +- You cannot yet get tests around the existing method, but you can test new code in isolation. +- Adding code inline would mix two unrelated concerns in the same method. + +Tradeoffs: +- Advantage: new code is cleanly separated from old code. The interface between them is explicit and visible through the method signature. +- Advantage: you can write tests for the new behavior without touching or testing the legacy method. +- Disadvantage: you are deferring cleanup of the legacy method. The source method remains in a limbo state — untested, with a single call to the new method grafted onto it. +- If the legacy class itself cannot be instantiated, consider making the sprouted method a public static method that takes the required data as arguments. + +## Sprout Class + +Use Sprout Class when Sprout Method is not enough — when the legacy class itself cannot be instantiated in a test harness within a reasonable time, or when the new behavior represents a responsibility large enough to belong in a separate class. + +Two situations that lead to Sprout Class: +- The new behavior would violate the existing class's responsibility, suggesting it belongs elsewhere by design. +- The existing class has so many creational or hidden dependencies that instantiating it in a test harness is not feasible now. + +Steps: +1. Identify where the change needs to happen. +2. Name a new class that would own the new behavior. Write the code to instantiate it and call it at the change point, then comment it out. +3. Determine what data the new class needs and pass those values through the constructor. +4. Determine whether the new class needs to return values to the source method; if so, add a method for that. +5. Develop the new class test-first. +6. Uncomment the instantiation and call. + +When to use it: +- Sprout Method is blocked because the source class cannot be instantiated. +- The new behavior requires its own data structures or has significant complexity that would clutter the source class. +- The new behavior has a distinct enough responsibility that it justifies a new concept. + +Tradeoffs: +- Advantage: move forward with confidence without touching the source class. +- Advantage: the new class can be fully tested in isolation. +- Disadvantage: increases conceptual complexity. A new class that is clearly just a workaround for a hard-to-test parent class is harder to understand for someone learning the codebase. Over time, some sprouted classes absorb new related behavior and become genuine concepts; others remain awkward relics. + +## Wrap Method + +Use Wrap Method when you need to add behavior that must happen at the same time as an existing method call, but should not be tangled with that method's logic. + +Temporal coupling — grouping code together only because it has to execute at the same time — produces methods that are hard to change independently later. Wrap Method explicitly separates the concerns. + +Two forms: + +**Form 1 — same public name:** Rename the existing method to something descriptive of what it actually does. Create a new method with the old name. The new method calls both the renamed original and the new behavior. Clients see no change in the interface. + +Steps: +1. Identify the method to change. +2. Rename the existing method, preserving its signature exactly. +3. Create a new method with the original name that calls the renamed method. +4. Develop the new behavior using test-driven development and call it from the new method. + +**Form 2 — new explicit name:** Keep the original method unchanged. Write the new behavior as a separate method. Create a third method that calls both. Expose this third method to callers who need the combined behavior. + +When to use it: +- The new behavior is cleanly before or after the existing logic, not interleaved with it. +- You want to introduce a seam between two concerns that have always executed together. +- The existing method should not grow any larger. + +Tradeoffs: +- Advantage: does not increase the size of existing methods. +- Advantage: makes the independence of the new behavior explicit. +- Disadvantage: renaming the original method to make room for the wrapper can produce a poor name. The renamed method often ends up describing only part of what it does. + +## Wrap Class + +Use Wrap Class — the Decorator pattern — when you need to add behavior around an entire class rather than a single method, or when the class cannot be instantiated in a test harness. + +How it works: create a new class that holds a reference to the original class and implements the same interface. The new class adds the new behavior and delegates the original calls to the wrapped object. + +When to use it: +- The new behavior needs to apply across many or all methods of the class. +- The original class cannot be changed (third-party, generated, or locked down). +- The behavior is a cross-cutting concern — logging, auditing, validation — that does not belong in the original class. +- You want to add tested behavior without touching untested production code. + +Tradeoffs: +- Advantage: no modification to the original class. All new behavior is isolated in the wrapper. +- Advantage: the wrapper can be fully tested without the original class's dependencies. +- Disadvantage: introduces an extra layer that callers must be aware of. If every method must be delegated, it increases the amount of boilerplate. + +## Extract and Override + +Use Extract and Override when a dependency is hardcoded inside a method and there is no seam to replace it. + +How it works: extract the dependency call into a new, overridable method on the class. In tests, subclass the class under test and override that method to substitute a fake or a controlled behavior. + +Variants: +- **Extract and Override Call** — extract a single hardcoded call into a virtual method. +- **Extract and Override Factory Method** — extract object creation into a virtual factory method so tests can substitute a different object. +- **Extract and Override Getter** — extract access to a field or singleton into a getter method, then override the getter in a test subclass. + +When to use it: +- A dependency cannot be injected through the constructor or a parameter. +- The dependency is created or accessed directly inside the method body. +- Subclassing is feasible and does not introduce other complications. + +Decision rule: if the dependency is accessed in one place, extract that one call. If it is accessed through construction, extract a factory method. If it is a field or global accessed repeatedly, extract a getter. + +## Parameterize Constructor + +Use Parameterize Constructor when a class creates a hard dependency inside its constructor, making it impossible to substitute the dependency in a test. + +How it works: add a new parameter to the constructor that accepts the dependency from outside. Provide a convenience constructor (or default argument) that supplies the production default, so existing callers do not break. + +When to use it: +- The constructor calls `new` on a concrete class that is expensive, has side effects, or cannot be instantiated in a test harness. +- The dependency needs to vary between production and test. + +Decision rule: prefer Parameterize Constructor over Introduce Static Setter when the dependency is per-instance and the class will be instantiated multiple times with different collaborators. + +## Introduce Static Setter + +Use Introduce Static Setter when the dependency is a global or singleton and there is no other way to replace it in a test. + +How it works: add a static setter method on the singleton or global holder that allows a test to install a substitute before the code under test runs, and reset it afterward. + +When to use it: +- The code accesses a global or singleton directly, with no parameter or constructor through which to inject a replacement. +- Parameterizing the constructor or method is too invasive given the time available. +- The singleton is used across many places and changing all call sites is not practical right now. + +Tradeoffs: +- This technique makes the global nature of the dependency explicit rather than hiding it. +- Tests that use a static setter must restore the original state afterward, or they will interfere with each other. +- Prefer it only when cleaner injection is not feasible in the current context. + +## Decision Rules: Choosing a Technique + +Use this guidance when deciding which technique to apply: + +- **Can you formulate the new behavior as a distinct, self-contained method?** Use Sprout Method. +- **Can you formulate the behavior but cannot instantiate the class?** Use Sprout Class. +- **Does the new behavior need to run at the same time as an existing method, but independently of its logic?** Use Wrap Method. +- **Does the new behavior apply across the whole class, or is the class not modifiable?** Use Wrap Class. +- **Is a dependency hardcoded inside a method body with no way to substitute it?** Use Extract and Override. +- **Is the dependency created in the constructor?** Use Parameterize Constructor. +- **Is the dependency a global or singleton accessed throughout the codebase?** Use Introduce Static Setter. + +When in doubt, prefer the technique that requires the fewest edits to existing code. The first goal is a safe path to a test, not a clean design. Clean design follows once the code is under test. diff --git a/skills/refactoring-best-practices/references/refactoring-moves.md b/skills/refactoring-best-practices/references/refactoring-moves.md new file mode 100644 index 0000000..c0b43b3 --- /dev/null +++ b/skills/refactoring-best-practices/references/refactoring-moves.md @@ -0,0 +1,147 @@ +# Refactoring Moves + +## Extract Method + +Use when: +- a method mixes several levels of abstraction +- a block has a clear name +- a decision or calculation is hidden in noise + +## Move Method + +Use when: +- a method uses more data from another object than from its own class +- callers need to pull data out before a decision can happen +- feature envy is visible + +## Extract Class + +Use when: +- the class has several reasons to change +- methods form cohesive clusters +- the object is carrying too much state + +## Introduce Value Object + +Use when: +- validation is repeated +- a primitive carries domain meaning +- formatting, parsing, comparison, or invariants belong to the value +- same-typed primitives can be accidentally swapped + +Do not use when: +- the fields are unrelated transport data +- the rule depends on current time, tenant, repository state, or workflow +- a type alias, brand, enum, or Parameter Object already provides enough clarity +- the concept has no stable domain meaning yet + +Safe sequence: +1. Characterize current behavior, errors, and serialized output. +2. Name one cohesive concept and separate intrinsic rules from contextual policy. +3. Add the Value Object beside the primitive API. +4. Convert primitives at one boundary and migrate callers incrementally. +5. Move duplicated validation, normalization, comparison, and behavior. +6. Add semantic equality, matching hashing, and defensive copies. +7. Verify persistence and transport round trips. +8. Remove obsolete primitive validation only after all feedback is green. + +Use `oop-best-practices` for the target Value Object design contract. + +## Introduce First-Class Collection + +Use when: +- collection rules are duplicated +- filtering, ordering, uniqueness, or summary logic belongs to the collection +- the collection has domain language of its own + +## Replace Conditional with Polymorphism + +Use when: +- branching depends on role, type, or policy +- variation is stable enough to deserve explicit collaboration +- adding a new branch would keep spreading decision logic + +Do not use when: +- the branching is tiny and local +- the variation is not stable +- the introduced abstraction would obscure the behavior more than clarify it + +## Rename + +Use when: +- a name no longer matches the concept it represents +- reading the code requires mental translation between the name and what it actually does +- the current name is an abbreviation or a generic word that carries no domain meaning +- the domain has evolved and the old name reflects an outdated understanding + +Note: rename usually preserves runtime behavior, but can break reflection, serialization, DI conventions, database mappings, and public consumers. Search those boundaries before treating it as mechanical. + +## Inline Method + +Use when: +- a method body is as clear as its name and the name adds no abstraction +- the method is called only once and the indirection adds no value +- a method was extracted for a reason that no longer exists + +Do not use when: +- the method name adds meaningful abstraction that the body alone does not convey +- the method has multiple callers that benefit from the shared name + +## Extract Interface / Protocol + +Use when: +- a class is used as a collaborator but callers depend on more than they actually need +- you want to substitute the collaborator in tests or extend behavior without changing existing code +- two unrelated classes could play the same role for a caller + +Note: define the interface by what clients need, not by what the class exposes. + +Do not extract a mirror interface merely because a class exists. Keep a single-implementation interface when it establishes a real port, dependency direction, public contract, or substitution boundary; implementation count alone is not decisive. Remove interfaces that only repeat a record, factory, or use-case surface without isolating change. + +## Remove Speculative Elements + +Use when: +- an enum contains states unsupported by current behavior +- a field has no rule, output, or current use case +- a repository predicts many query/count/delete variants +- an abstraction exists only for a hypothetical future + +Safe sequence: +1. Search production, tests, serialization, reflection, DI, persistence mappings, and external consumers. +2. Protect any current observable contract. +3. Delete one element and run feedback. +4. Restore it only if a concrete consumer or boundary proves its job. + +YAGNI does not authorize breaking public compatibility or removing deliberate architecture boundaries. + +## Consolidate Duplicated Knowledge + +Use when the same business decision must remain synchronized across multiple expressions. Extract a named policy or move the rule to its canonical owner. + +Do not consolidate merely because code has the same shape. Invoice and order calculations may share one pricing policy, while email, SMS, and push workflows can evolve independently despite structural similarity. Prefer composition around a shared decision over a generic base class that couples unrelated concepts. + +## Replace Temp with Query + +Use when: +- a temporary variable stores the result of an expression that is computed once +- the expression can be given a meaningful name as a method +- callers would benefit from the named query instead of reading the raw expression + +Do not use when: +- the computation is expensive and caching the result in a variable matters for performance + +## Separate Query from Modifier + +Use when: +- a method both returns a value and changes state, mixing a side effect with a return value +- callers cannot ask a question without causing a change in the system +- testing becomes hard because observing a result also mutates state + +Note: prefer separation when it clarifies behavior, but treat this as a heuristic. Atomic operations such as `pop`, iterators, and fluent immutable APIs may legitimately return a result while changing or replacing state. + +## Split Phase + +Use when: +- a method or class mixes two sequential concerns, such as parsing input and then processing it +- the second phase depends only on the output of the first phase, not on the raw input +- each phase has its own vocabulary and its own reasons to change diff --git a/skills/refactoring-best-practices/references/safe-change-workflow.md b/skills/refactoring-best-practices/references/safe-change-workflow.md new file mode 100644 index 0000000..340d43c --- /dev/null +++ b/skills/refactoring-best-practices/references/safe-change-workflow.md @@ -0,0 +1,98 @@ +# Safe Change Workflow + +Use this workflow when refactoring existing code under uncertainty. + +## 1. Observe Before Editing + +- Identify the entry points. +- Identify externally visible outcomes. +- Identify failure modes and side effects. +- Write down what must remain stable. + +## 2. Add Characterization Tests + +- Test current behavior before redesigning internals. +- Focus on outcomes, not private implementation. +- Capture edge cases that are easy to break accidentally. + +## 3. Find Seams + +A seam is a place where you can change behavior without editing everything around it. + +Look for seams around: + +- databases +- clocks and random generators +- file system access +- external APIs +- framework globals +- static singletons + +## 4. Break Dependencies Narrowly + +- Introduce a small adapter instead of a wide rewrite. +- Separate object construction from domain behavior. +- Move only enough code to make the next safe step possible. + +## 5. Refactor in Small Moves + +Good move sequence: + +1. rename +2. extract method +3. move method +4. extract class +5. introduce value object +6. replace conditional with polymorphism + +## 6. Recheck Constantly + +After each meaningful step ask: + +- Did behavior stay the same? +- Did the code become easier to change? +- Did the public API get simpler or more honest? +- Did I introduce accidental complexity? + +Keep tests attached to observable behavior rather than private helpers. A private-method spy turns harmless rename, extract, inline, or reordering moves into test failures even when the contract remains unchanged. + +## 7. Sensing and Separation + +When breaking a dependency in legacy code, name which of two distinct problems you are solving: + +- **Sensing** — you need to observe what the code does. The values it computes, the side effects it produces, or the calls it makes are invisible from the outside. Breaking the dependency lets you access or record those values so a test can verify them. +- **Separation** — you cannot even get the piece of code into a test harness to run. Hard dependencies (real database connections, live hardware, external processes) prevent instantiation or execution. Breaking the dependency replaces those collaborators so the code can run at all. + +Most dependency-breaking techniques serve one or both purposes. Sensing problems are usually solved by substituting a fake collaborator that records calls or returns controlled values. Separation problems are solved by any technique that removes the obstacle to instantiation or execution. Knowing which problem you have helps you pick the right technique and avoid over-engineering the solution. + +A single dependency can block you on both fronts at once. Solve separation first — you cannot sense anything from code you cannot run. + +## 8. Finding Test Points + +In code that has no tests, the first task is locating where a test can attach. Look in three directions: + +- **Entry points** — public methods, event handlers, message handlers, and command dispatchers. These are the places where the system accepts input and begins executing the logic you want to cover. +- **Output points** — return values, written files, sent messages, database writes, and any other externally observable result. A test that drives an entry point and then checks an output point gives you direct behavioral coverage. +- **Effect points** — state that changes when the code runs: object fields, global variables, in-memory collections, and anything a collaborator records. When you cannot observe a return value or a file, a fake collaborator that records calls can expose the effect. + +When direct tests on the target code are impossible, find the nearest observable point upstream or downstream and write tests there first. A test at an indirect point still catches regressions and gives you enough coverage to begin dependency-breaking work safely. As dependencies are removed, move the tests closer to the code under change. + +For event migration, keep the legacy command as the entry point and database/message/email effects as output points. A recording message port can expose the new fact as a sensing seam before delivery timing changes. Move one secondary effect at a time and keep primary persistence unchanged; read `domain-event-migration.md` for the complete sequence. + +## 9. The Legacy Code Change Algorithm + +Feathers' algorithm from *Working Effectively with Legacy Code* structures every change in untested code as a five-step sequence: + +1. **Identify the change points** — find the exact locations where the required change must happen. Understanding the architecture well enough to place the change correctly is a prerequisite; without this, dependency-breaking work may happen in the wrong place. +2. **Find the test points** — determine where tests can be written to cover the change points. Test points are often not the same locations as change points. Look for entry points, output points, and effect points nearby. +3. **Break dependencies** — remove the obstacles that prevent getting the code into a test harness and sensing its behavior. Apply the minimum change needed: introduce a seam, extract a collaborator, or substitute a fake. These steps are done without full test coverage and should be as mechanical and safe as possible. +4. **Write tests** — write characterization or pinch-point tests that cover the change points through the test points found in step 2. These tests document current behavior and protect against accidental regression during the actual change. +5. **Make changes and refactor** — with test coverage in place, make the functional change and then improve the surrounding structure. This is the only step where behavior is intentionally altered. + +Steps 1 through 4 are setup. No functional change happens until step 5. The goal of each programming episode is to leave both new functionality and new tests behind, so that tested areas of the codebase grow over time. + +## 10. Reproduce Defects At Their Observable Boundary + +Before fixing a defect, add a failing test where the incorrect result can actually be observed. Assert final state or output, not only that a collaborator was called. + +An interaction test can verify `repository.save(updatedEntity)` while missing that the real repository refuses to replace an existing record. Start with the use-case plus a faithful fake or adapter contract test that proves a subsequent read returns the update. Add a narrower unit test only if it protects a separate decision. diff --git a/tests/inventory.test.mjs b/tests/inventory.test.mjs index 38956b3..c6ac067 100644 --- a/tests/inventory.test.mjs +++ b/tests/inventory.test.mjs @@ -31,11 +31,12 @@ test('live skills are unique and include the three original packages plus BotKit assert.ok(names.includes('setup-bot')); assert.ok(names.includes('retro')); assert.ok(names.includes('simple-as-writing')); + assert.ok(names.includes('refactoring-best-practices')); assert.ok(names.includes('tdd')); assert.ok(names.includes('matt-tdd')); assert.ok(names.includes('teach')); assert.ok(names.includes('matt-teach')); - assert.equal(names.length, 96); + assert.equal(names.length, 97); }); test('pinned original sources remain present', async () => {