feat(cstack): add refactoring-best-practices

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)

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.