# Phase 2 — System Designer Lessons 0015–0026. Flexible, composable code; complexity that stays managed as the system grows. Phase 1 protected the domain. Phase 2 is about everything around it: what is a class and what is a function, what gets injected, where infrastructure lives, and what happens to the eleven module-level globals this system currently depends on. --- ## 2.1 Decide what deserves to be a class **Lessons:** 0015 (Classes vs Functions) **Do this.** `ReturnsService` is a class with no state — every method reaches for module globals instead. `ReturnManager` is a class whose only field is never assigned. Meanwhile `policy.check_eligibility` is a function that needs three collaborators. Go through the package and, for each unit, answer: does it hold state across calls, or is it a function wearing a class? Convert in both directions where the answer says so. **Observable outcome.** No behavioural test changes. `service.py` shrinks; `ai_helpers.ReturnManager` disappears or becomes two functions. **Hints.** - A class earns its keep when it holds state, when it has more than one implementation, or when it bundles collaborators a caller should not have to assemble. `ReturnsService` will qualify on the third once exercise 2.2 gives it constructor arguments. - A module of functions is already a namespace. You do not need a class to get one. **Forces to weigh.** Functions are easier to read, test and move; objects are easier to configure and substitute. The system's current problem is not that it has classes, it is that its classes hold no state and its functions hold all of it — in module globals. --- ## 2.2 Inject the collaborators **Lessons:** 0016 (Injecting Behavior) **Do this.** Give `ReturnsService` a constructor that takes what it needs: a return repository, an order-system client, the label port from exercise 1.7, a payments port, and a way to publish notifications. Delete the module-level `SERVICE` and build the object where the process starts. **Observable outcome.** `tests/support.reset_world()` gets measurably shorter. Tests can construct a service with fakes and no SQLite. Nothing in `fitness/test_fitness_global_state.py` is green yet — that is 2.5 and 2.6 — but this is the move that makes them possible. **Hints.** - Constructor injection, not a service locator. If the service asks a registry for its dependencies, you have moved the global rather than removed it. - Six constructor arguments is a smell that says the class still does too much. It is the same smell as exercise 1.5's seven reasons to change. **Forces to weigh.** Injection moves the assembly problem somewhere else — a composition root — and that root is genuinely ugly. The trade is one ugly place you can read against eleven implicit ones you cannot. --- ## 2.3 Depend on protocols, not on implementations **Lessons:** 0017 (Protocols & ABCs) **Do this.** Write the ports from exercises 1.7 and 2.2 as `typing.Protocol`s — `ReturnLabels`, `RefundGateway`, `ReturnRepository`, `NotificationPublisher` — and type the service's constructor against them. Write one in-memory fake per protocol for the tests. **Observable outcome.** `test_there_is_a_declared_boundary_between_components` (5.4) goes green if you put them in a `ports.py` or `contracts.py`. Every characterization test that previously needed SQLite or a vendor fake can now be written against the fakes. **Hints.** - `Protocol` for things you do not own and things with many shapes; ABC when you want shared behaviour or a registration hook. Here it is Protocol. - The protocol is written from the *caller's* need. If it has a method only one implementation could satisfy, it is not a protocol, it is a header file. **Forces to weigh.** Protocols are structural: nothing forces an implementation to declare itself, so a rename can silently orphan an adapter. ABCs are explicit and rigid. The mitigation is a contract test each implementation must pass — cheaper than choosing rigidity for the whole system. --- ## 2.4 Stream the export instead of building it **Lessons:** 0018 (Generators & Streaming) **Do this.** `reporting.refund_rows_for_export` materialises every refunded row into a list, and finance's real table has two years of history in it. Turn it into a generator, and add a batched version of the migration backfill that exercise 3.5 will need. **Observable outcome.** `test_the_csv_export_row_shape_is_frozen` still passes with `list(...)` around the call. Memory use stops scaling with history. **Hints.** - The row shape is a published contract with finance — see `tests/test_legacy_consumer_reporting.py`. Streaming changes when rows are produced, not what they contain. - A generator that holds a database cursor open holds a transaction open too. Decide whether you are streaming from a cursor or paging by id, and say why. **Forces to weigh.** Generators are lazy, and laziness moves failures to the consumer — a `for` loop three call frames away now raises the database error. They also make retries harder, because a partially consumed generator cannot be replayed. For a nightly export, worth it. For a request-response handler with a 2,000-row cap, not. --- ## 2.5 Cut the phone lines **Lessons:** 0019 (Global Coupling) **Do this.** `config.SETTINGS` is a module-level mutable dict imported by seven modules. `config.COUNTERS`, `config.HANDLERS`, `notifications._QUEUE`, `notifications.OUTBOX`, `service._SEQ`, `db._CONN`, `ai_helpers._FORMAT_CACHE`, `ORDER_SYSTEM`, `SHIPMASTER`, `PAYPORTAL`, `MANAGER` are the rest. Replace the settings dict with an immutable configuration object built once and passed in, and give every remaining global an owner. **Observable outcome.** In `fitness/test_fitness_global_state.py`: - `test_no_module_holds_mutable_state_at_import_time` - `test_configuration_is_passed_in_rather_than_imported` - `test_the_return_id_sequence_is_not_a_module_counter` **Hints.** - A frozen dataclass for settings, built from defaults plus environment, passed to the composition root. Reading it is then a data dependency, not a phone line. - The return-id sequence is not configuration; it is identity. Either the database owns it (an autoincrement) or the caller supplies it (a UUID). A module counter that resets on restart will collide. - `config.HANDLERS` is populated by importing `api.py`. That means the route table depends on import order. Build the table explicitly instead. **Forces to weigh.** Passing settings through five call frames is tedious, and the tedium is the honest price of visible dependencies. The counter-argument — "it is read-only in practice" — is falsified by this system: `SETTINGS` is mutated by the tests, by the demo, and could be mutated at runtime, changing what every in-flight return is worth. --- ## 2.6 Give the shared state an owner **Lessons:** 0020 (Shared State & Ownership) **Do this.** `config.CURRENT_USER` is a process-wide singleton that `api.handle` mutates on every request and never restores — one of the characterization tests pins exactly that. Two concurrent requests share one identity. Replace it with a request context passed explicitly from the edge. Then do the same for `db._CONN`. **Observable outcome.** In `fitness/test_fitness_global_state.py`: - `test_there_is_no_process_wide_current_user` - `test_the_database_connection_is_owned_by_something_nameable` `test_the_role_comes_from_a_process_wide_singleton` goes red. Delete it: it exists to record the defect. **Hints.** - The identity of the caller is an argument to the operation, not an ambient fact. `submit(command, actor)` is honest; `submit(command)` reading a global is not. - `contextvars` is the tempting middle road. It fixes the concurrency bug and keeps the invisibility. Use it if you must, but be clear that you have made the coupling safe rather than removed it. - The connection: one owner, created at the composition root, handed to the repository. The `_WRITE_LOCK` in `db.py` disappears with it. **Forces to weigh.** Threading an actor through every call is noise in every signature, and in a web framework the ambient-context pattern is idiomatic and well understood. The question is whether the ambient value can be *wrong* — and here it demonstrably can, because nothing restores it. --- ## 2.7 Build an error taxonomy **Lessons:** 0021 (Error Handling Strategies) **Do this.** The system currently distinguishes exactly two failures: vendor exceptions and everything else. Write the vocabulary: a base `ReturnsError`; `ReturnNotEligible` and `ReturnAlreadyExists` (the caller is wrong); `CarrierUnavailable` and `RefundGatewayUnavailable` (retryable); `OrderSystemUnavailable` (retryable, upstream). Raise them in the domain, translate them once at the edge. **Observable outcome.** `api.py` has one `except` per error class and no catch-all. Each maps to a status code you can defend: 409 for a duplicate, 422 for ineligible, 502 for an upstream outage, 500 only for a bug. **Hints.** - Retryable and not-retryable is the distinction the caller actually needs. Encode it in the type, not in a string. - Retry belongs in the adapter that wraps the port, never in the rule. Lesson 0021's `RetryingPaymentGateway` is the shape. - A denial is not an error. It is a successful answer of "no", and the characterization tests show the API already returns 201 for it — keep that and document it. **Forces to weigh.** Typed errors are a published contract of their own: adding one is a client-visible change, and clients will branch on them. Fewer, broader types are easier to evolve and less useful. Aim for the smallest set the callers actually branch on — three, here, not eight. --- ## 2.8 Stop swallowing failures **Lessons:** 0021 (Error Handling Strategies), 0014 (Fail-Fast) **Do this.** Five `except` blocks neither re-raise nor log: `fraud.assess` (twice), `service.issue_label`, `service.issue_refund`, `api.submit_return`, and `ai_helpers.parse_timestamp`. The entire incident record is `config.COUNTERS["errors_swallowed"]`. Add structured logging, and make each of those five either re-raise, translate, or log-and-continue with a written reason. **Observable outcome.** In `fitness/test_fitness_observability.py`: - `test_no_exception_is_swallowed_without_a_trace` - `test_the_package_configures_a_logger` `test_a_carrier_outage_leaves_the_return_stranded_in_APPROVED` and `test_an_order_system_outage_is_swallowed_and_the_watchlist_rule_skipped` will need decisions rather than deletions. **Hints.** - Log once, at the boundary, with the traceback — not at every frame on the way up. Lesson 0021's `logger.exception("checkout failed")` is the pattern. - `except Exception: pass` around `datetime.fromisoformat` is hiding a data problem you would want to know about. - The carrier outage is the interesting one: the return is stranded in `APPROVED` and nothing will retry it. Logging it is the floor. Exercise 3.3 builds the retry. **Forces to weigh.** Logging everything is its own failure mode — a log nobody reads, and a per-request cost. The test to apply per site: if this fires at 03:00, what would the on-call engineer need in order to act? If the answer is "nothing, it is expected", it is not an error and should not be an `except`. --- ## 2.9 Decide where SQL is allowed to appear **Lessons:** 0022 (Module Organization) **Do this.** Write the import rule for this package as a table — which layer may import which — and then enforce it. Reorganise the package to match: domain (`models`, `policy`, `fraud`, the Return aggregate), application (`service`), infrastructure (`db`, `repositories`, adapters), edge (`api`). **Observable outcome.** In `fitness/test_fitness_dependency_direction.py`: - `test_no_domain_module_contains_sql` - `test_domain_modules_import_no_infrastructure_module` stays green **Hints.** - Packages, not prefixes. `returns_lab/domain/`, `returns_lab/adapters/`. - `fitness/_marks.py` already has the tools: `imported_siblings`, `modules`, `source`. Your enforcement is about eight lines. - Update `DOMAIN_MODULES` and `INFRASTRUCTURE_MODULES` in `_marks.py` when you move files — the fitness functions read those lists. **Forces to weigh.** A layered package layout makes the dependency rule checkable and makes "where does this go?" answerable. It also spreads one feature across four directories, which is worse for reading a single change. A feature-first layout (`returns/`, `refunds/`, `labels/`) trades the opposite way. Pick one, write down why, and make the fitness function match the choice — lesson 0022's point is that either is fine and *unenforced* is not. --- ## 2.10 Adapters for everything outside, including the clock **Lessons:** 0023 (Ports & Adapters for Infrastructure) **Do this.** Inventory every piece of infrastructure this system touches: SQLite, the order system, ShipMaster, PayPortal, the email gateway, the SMS gateway, `datetime.now()` (five modules), `uuid4`, `threading`. Give each a port and an adapter, or a written reason it does not need one. **Observable outcome.** `test_a_second_return_for_the_same_line_is_denied` and every window-expiry test can be written against a fixed clock instead of relative fixture dates. `orders.py`'s `_days_ago` helper stops being load bearing. **Hints.** - The clock is the one people skip, and it is the one that makes time-dependent rules testable. `Clock.now()` as a protocol; a `FrozenClock` in tests. - Not everything needs a port. `uuid4` behind an `IdGenerator` is worth it if you assert on ids; `json.dumps` is not. - The composition root wires all of them. That file is allowed to be ugly and is the only one that knows every concrete type. **Forces to weigh.** Each port is an extra file and an extra indirection for a thing with exactly one implementation. The rule that holds up: port it if it makes a test slow, flaky, or impossible — or if it is something you might buy differently later. That is three of the nine here, plus the clock. --- ## 2.11 Get the async work out of the domain **Lessons:** 0024 (Async Out of Domain Logic) **Do this.** `service.py` calls `notifications.notify` seven times, so the workflow knows there is an email gateway, a worker thread and a retry policy. Remove that knowledge: the service records what happened; something else decides who to tell. **Observable outcome.** In `fitness/test_fitness_contracts_and_async.py`: - `test_the_domain_does_not_start_background_work` **Hints.** - The intermediate step is a `NotificationPublisher` port. The better step is exercise 2.12's events. - The worker thread, the queue and the retry policy are infrastructure. None of them should be importable from the domain package after exercise 2.9. **Forces to weigh.** Making the domain synchronous and pure means something else must own delivery, and that thing needs to exist, be started, be monitored, and be tested. You are moving complexity, not deleting it — but you are moving it to a place where it can be tested without a database and restarted without touching the rules. --- ## 2.12 Publish events instead of calling consumers **Lessons:** 0025 (Event-Driven Architecture), 0022 (Module Organization) **Do this.** Replace the seven direct notification calls with facts the service records: `ReturnSubmitted`, `ReturnDenied`, `ReturnHeldForReview`, `ReturnApproved`, `LabelIssued`, `RefundIssued`. Publish them after the write commits. Let notifications subscribe. **Observable outcome.** `test_no_module_depends_on_more_than_five_siblings` goes green — `service.py` currently imports ten. The notification tests move to "given this event, this message goes out", and stop needing the whole workflow. **Hints.** - Events are facts in the past tense, not instructions. `RefundIssued`, not `SendRefundEmail`. Lesson 0025's whole point. - Publish after the transaction commits, or you will notify customers about refunds that rolled back. This matters more after exercise 5.2. - An in-process synchronous event bus is enough here. Do not add a broker. **Forces to weigh.** Events decouple the producer from its consumers and make the call graph unreadable — you can no longer answer "what happens when a refund is issued?" by following the code. The price is real. Pay it where the consumer list grows (notifications, analytics, the fraud model, finance) and not where there is exactly one consumer forever. --- ## 2.13 Buy the retry, or build it **Lessons:** 0026 (Libraries vs Building) **Do this.** The notification worker needs backoff, a retry budget, a dead-letter destination and at-least-once semantics. Write the two-column comparison: what a library gives you (`tenacity`, a real broker) versus the forty lines you would write. Then decide — and if you decide to build, say what you are *not* building. **Observable outcome.** A written decision. Whichever way it goes, the lab must still run with no external services, so a "buy" decision here means a port with an in-process implementation for the lab and a named library for production. **Hints.** - The 80% test from lesson 0026: does the library do 80% of what you need now, and will it still do 80% in three years? - Retry logic is famously easy to write badly. Backoff with jitter, a maximum budget, and idempotent delivery are three separate problems, and this system's current attempt gets all three wrong. **Forces to weigh.** A dependency is a promise to keep upgrading something. A hand-rolled retry is forty lines you own and nobody understands at 03:00. The deciding question is not size, it is whether the problem is yours: retry semantics are not this business's differentiator, and eligibility rules are. --- ## Done when - These go green: `test_no_module_holds_mutable_state_at_import_time`, `test_there_is_no_process_wide_current_user`, `test_configuration_is_passed_in_rather_than_imported`, `test_the_database_connection_is_owned_by_something_nameable`, `test_the_return_id_sequence_is_not_a_module_counter`, `test_no_exception_is_swallowed_without_a_trace`, `test_the_package_configures_a_logger`, `test_no_domain_module_contains_sql`, `test_no_module_depends_on_more_than_five_siblings`, `test_the_domain_does_not_start_background_work`. - `tests/support.reset_world()` is under ten lines, or gone. - You can construct the whole system in a test with fakes and no SQLite. Next: `phase-3.md`.