Critique & Architecture Studio

Annotated review — each finding is line → smell → rule:

• L4 — Cryptic, non-intention-revealing names (Chk, n, e, a, flag) — breaks use intention-revealing names. Should be IsEligibleMember(name, email, age, isPremium).
• L4, L13 — Flag argument bool flag — breaks avoid flag arguments; the function loudly admits it does more than one thing. Split the premium path out or make it data, not a branch.
• L1, L16, L21, L23, L25 — Hidden side effect via shared mutable static state: a function named like a pure query mutates the public global Errs, and line 16 silently erases errors recorded by earlier calls. Breaks no side effects and command–query separation. Results should be returned, not smuggled through a global.
• L6 — Console I/O inside validation logic — a second side effect and a Single-Responsibility (SRP) violation: validation is now welded to a UI concern and untestable without capturing stdout.
• L7–L24 — Arrow-shaped deep nesting (4 levels) — breaks small functions / minimal indenting. Invert to guard clauses: if (string.IsNullOrEmpty(name)) return Fail("bad name");…
• L9 — Magic numbers 16, 100 — breaks searchable names; extract MinimumAge/MaximumAge constants.
• L13 — flag == true — redundant comparison noise; write if (isPremium).
• L3, L15 — Noise / misleading comments — line 3 restates nothing usefully; line 15 "documents" behaviour (skipping a waiting list) that the code doesn't even implement. Breaks comments must not lie.

• L5–L37 — God function / does more than one thing: validates, calculates, formats and audits — four responsibilities in one method. Breaks functions do one thing and SRP.
• L3 + L34 — The hidden side effect: a method whose name promises "generate a report string" also mutates public global state. Why it's a side effect: the observable change (a new AuditTrail entry) is not part of the return value and not hinted at by the name — it's a lie of omission. Problems it introduces: (1) temporal coupling — calling the function twice audits twice, so "preview" and "commit" are indistinguishable; (2) untestability — tests must reset global state or they interfere with each other; (3) thread-unsafety — a shared mutable List with no locking; (4) callers can never reason locally about the function.
• L10, L16, L18, L22 — Mentally-mapped single-letter names (t, d, sub, r) — breaks intention-revealing names.
• L17, L30 — applyLoyalty == true noise, flag argument steering two code paths (avoid flag arguments), and the loyalty rule (10%) duplicated as a magic number.
• L8 — Stringly-typed error handling: returning "ERROR: no items" in the same channel as a valid report forces every caller to string-match. Prefer an exception or result type (prefer exceptions to error codes).
• L22–L32 — Mixed abstraction levels: penny-level string concatenation sits beside business policy. Breaks one level of abstraction per function.

// after the refactor: three small pieces, no globals
public static class InvoiceCalculator          // pure arithmetic — trivially testable
{
    public static decimal CalculateSubtotal(List<LineItem> items) { /* ... */ }
    public static decimal CalculateTotal(List<LineItem> items, decimal taxRate, bool applyLoyalty) { /* ... */ }
}

public static class InvoiceFormatter           // numbers -> report string, nothing else
{
    public static string Format(List<LineItem> items, string customer, decimal taxRate, bool applyLoyalty) { /* ... */ }
}

public static class Billing                    // thin composition root
{
    public static string GenerateReport(List<LineItem> items, string customer, decimal taxRate, bool applyLoyalty)
    {
        if (items == null || items.Count == 0) return "ERROR: no items";
        return InvoiceFormatter.Format(items, customer, taxRate, applyLoyalty);
    }
}

What to notice: the behaviour tests never went red — that is what makes this a refactoring (behaviour-preserving) rather than a rewrite. And auditing didn't get "lost": deleting AuditTrail forces the caller who actually needs an audit log to ask for one explicitly — the dependency becomes visible instead of hidden. That is the Boy Scout Rule (Chapter 7) executed with a harness.

• L18–L20 — Swallowed exception: an empty catch (Exception) turns every failure (missing file, bad data, I/O error) into a silent, plausible-looking 0. Breaks don't return/accept null-ish error codes; use exceptions honestly — this is worse than an error code, it's no signal at all. 0 °F is also a valid temperature, so failures are indistinguishable from data.
• L16 — Hidden side effect #1: a query named GetTemp appends to a log file. Command–query separation broken (no side effects).
• L1, L15 — Hidden side effect #2: mutation of shared static cache — global mutable state with the same testability/threading problems as Drill 2's AuditTrail.
• L4, L9 — Flag argument refresh (plus == false noise) — two functions pretending to be one.
• L14 — Magic numbers with a useless comment: * 1.8 + 32 is Celsius→Fahrenheit, but "// convert" explains nothing. Also a silent unit decision buried in a line — name it (CelsiusToFahrenheit(...)) instead of commenting it. Bonus: the line's broken indentation is a formatting deficiency in its own right (Chapter 7).
• L3 — Noise doc-comment that restates the name and hides the two side effects — a comment that actively misleads.
• L6 + L21 — Misleading default: initialising t = 0 just so the catch path has something to return is the mechanism that makes the swallowed exception dangerous.
• L13 — Hard-coded path + string-built path — environment coupling that makes the function untestable without a real file system (a seam an injected IFileStore would fix — see DIP).

God Object (a.k.a. The Blob): one class that knows everything and does everything — patients, scheduling, SMS, billing, persistence, reporting, config. It is the walking negation of SRP: at least five reasons to change, so every team edits the same file, every change risks every feature, and nothing can be tested in isolation. Escape: split along responsibilities — PatientRegistry, AppointmentScheduler, BillingService, ReportExporter — each behind an interface. If clients enjoyed the one-stop convenience, keep a thin Facade that delegates to the parts; the facade owns no logic. Refactor incrementally: extract one responsibility at a time behind a characterisation-test net (exactly the C01 move, scaled up).

Golden Hammer: one familiar tool (here Singleton) applied to every problem regardless of fit. TaxCalculator has no single-instance requirement at all — it's stateless maths. The failure mode: twenty pieces of global state, hidden dependencies everywhere (any method can grab any Instance), impossible test isolation, and invisible coupling — the same disease as Drill 2's static, times twenty. Escape: re-derive each choice from the problem: keep a single instance only where one is genuinely required, and even then prefer letting a DI container manage lifetime while classes receive dependencies through constructors (DIP; see the Singleton caveats in Chapter 3). Culturally: add "what problem does this pattern solve here?" to code review.

The three classic decay symptoms, one per complaint:

• Rigidity — hard to change: adding PDF forces edits to existing, working code (the switch), because the variation isn't encapsulated (OCP violation).
• Fragility — breaks in unrelated places: CSV quoting change broke XML because both formats share tangled private helpers.
• Immobility — can't be reused: the totalling logic is welded to a hard-coded SqlDb and SmtpMailer (DIP violation), so extracting it drags infrastructure along.

Escape: encapsulate what varies — an IReportFormatter strategy per format (Strategy) kills the switch; pure SalesTotaller for the maths; inject IDataSource and IReportSender abstractions so the service composes parts it doesn't construct. Each symptom then disappears for a named reason: new format = new class (rigidity), formats isolated (fragility), pure logic portable (immobility).

Over-DRY — the wrong abstraction. Three coincidentally similar computations were merged as if they were one piece of knowledge. They then evolved apart, and every divergence was patched with another boolean parameter — the flag arguments are the tell (five parameters steering one function = three functions in a trench coat). Now every caller pays the complexity of all three domains, and changing refund rules risks checkout. The counter-principle is AHA — Avoid Hasty Abstractions: duplication is cheaper than the wrong abstraction, because duplication is visible and mechanical to unify later, while a wrong abstraction spreads conditionals everywhere. Escape: inline the shared function back into three honest ones — CalcCheckoutTotal, CalcRefundAmount, CalcLoyaltyPoints — let each keep only the branches it uses, and re-extract only what remains genuinely common (perhaps just the rounding policy). DRY real knowledge (the tax rate constant); don't DRY resemblance.

Class explosion through combinatorial inheritance. Options are independent axes, but inheritance forces every combination to be a named class: 3 options already cost 6 subclasses; with Hazmat it's up to 2⁴−1 = 15; n options → 2ⁿ−1. Each addition breaks OCP (you extend the codebase by writing yet more near-identical classes and touching client code that dispatches on type). Knowledge is duplicated — the ×1.5 rule appears in four classes, so a price change is a shotgun edit (fragility). Combinations are frozen at compile time: a parcel can't become refrigerated at runtime (the explosion problem, Chapter 2). And the hierarchy misuses is-a: "RefrigeratedInternationalExpressDelivery" is not a kind of delivery, it's a delivery with three add-ons — has-a wearing an is-a costume.

Redesign: one interface IShippingSurcharge { decimal Apply(decimal baseCost); }; one small class per option (ExpressSurcharge, RefrigeratedSurcharge, InternationalSurcharge); a single Shipment that has a list of surcharges and sums them over the base cost. Any combination is a list, not a class.

Justification (this is where the marks are): options become composable at runtime — any subset is a list, so n options need n classes, not 2ⁿ−1. Adding Hazmat = one new class, zero edits to Shipment or existing surcharges — OCP satisfied. Each pricing rule lives in exactly one place (no shotgun edits). Shipment depends only on the abstraction (program to an interface, DIP), and each tiny class is testable alone. That is the concrete content of "favour composition over inheritance": inheritance freezes a static taxonomy at compile time; composition assembles behaviour per object at runtime. Common drawing mistakes: using a closed triangle (inheritance) instead of the dashed open-triangle realization to the interface; drawing Shipment→surcharge as inheritance instead of an aggregation/association with multiplicity 0..*; forgetting the «interface» stereotype.

Step5_ArbitraryCombination_JustComposes is the money test: express+refrigerated+international priced by one class fed a three-element list. When it passes, you have physically demonstrated the sentence the exam wants you to write. Note also what you deleted: all six legacy subclasses, with no behaviour lost — the strongest possible evidence that the taxonomy carried no real knowledge.

[Fact]
public void Fare_IsBasePlusDistance()
{
    Assert.Equal(14.50m, FareCalculator.Fare(distanceKm: 10m, minutes: 0m, surge: 1.0m));
}

One rule, one assertion, neutral values for everything else (0 minutes, surge 1.0). It ignores time, surge, the minimum and validation on purpose — each of those deserves its own red. Starting with the error cases ("negative distance throws") is legal but weak: it forces you to design the signature around exceptions before the happy path has shaped it.

At Stage 1 the honest minimum is almost embarrassing (return 2.50m + distanceKm * 1.20m;) — that's correct TDD, not laziness; generality must be demanded by a test. Around Stage 3 you'll feel the pull to restructure into (base + distance + time) * surge — do it in the refactor step, under green. Stage 4's minimum-fare rule will fight the naive formula and force an explicit policy line — notice how the test drove that decision into the open. The finished shape (small, pure, guard-clauses first) was never "designed"; the tests squeezed it into existence.

1 — Testability pressure forces modularity. Because the test is written first, you design the API from the caller's side, and anything hard to test (hidden globals, file I/O, tangled dependencies) hurts immediately — so it gets designed out. FareCalculator came out pure and dependency-free because a static, side-effecting version would have been painful to test from the first red. At scale, this same pressure pushes toward small units, injected dependencies (DIP) and single responsibilities — TDD is a design tool wearing a testing costume. 2 — The green bar makes refactoring (and all future change) safe. The suite is a regression net built as a free by-product: at Stage 3 you restructured the whole formula with zero fear, because five seconds later the bar said "still correct". That is the exact mechanism that keeps code maintainable for years — the Boy Scout Rule is only practical when a net exists (you used the same trick in Lab C01, where pinned tests made a scary refactor routine). Secondary credit: the tests are executable, always-current documentation of every fare rule.

The key sentence: MVC is primarily a pattern for organising the presentation tier — the whole triangle refines how UI concerns are separated, while the Model reaches down into the business (and, through it, data) tiers. The distinction matters for web apps because the presentation tier is where change is fastest and most parallel: many views (HTML, JSON API, mobile) must reuse one set of business rules, and UI specialists must work without touching domain logic. MVC gives the presentation tier internal structure that plain 3-tier ("everything UI-ish goes in tier 1") does not. They compose rather than compete: a typical web app is 3-tier and MVC — V and C in/near tier 1, M across tiers 2–3. (Full walkthrough: Chapter 8 §MVC.)

The one-line summary the marker wants: RMI trades universality for simplicity (Java end-to-end, no IDL); CORBA trades simplicity for universality (any language, but through an IDL and ORB machinery). gRPC is the modern synthesis — CORBA's language-neutral IDL contract with RMI-grade developer ease, on commodity web infrastructure (see the comparison table in Chapter 9 §IPC technologies).

• Seat hold → one-to-one, synchronous (request/response, e.g. REST or gRPC). A single, known receiver (Seat-Inventory) and the caller is blocked by the business, not just the code — the UI literally cannot proceed without the yes/no. Async here would add latency and complexity for nothing. Guard the call with a timeout and circuit breaker, since a hung Seat-Inventory must not hang Booking.
• On-sale announcement → one-to-many, asynchronous (publish/subscribe via a broker, e.g. Kafka/RabbitMQ). The publisher can't and shouldn't know its receivers — thousands of follower notifications, plus tomorrow maybe Analytics and Pricing, all consume the same TicketsOnSale event. No response is needed; the broker absorbs the fan-out spike; new subscribers appear without touching the publisher (event-driven style, Chapter 10).
• Checkout → split it. Booking→Payment: one-to-one, synchronous — the outcome gates the business decision (no ticket without a successful charge), so Booking must wait for the result. Then Booking→Notifications: one-to-one, asynchronous (notification/message queue) — exactly one receiver but no reply needed, and the email must not delay or fail the purchase; a queued message survives a Notifications outage and is retried. (Publishing a PurchaseCompleted event instead is also creditworthy — argue it lets receipt-email, analytics and fraud all subscribe.)

Recommend orchestration. Three mechanism-level reasons, all grounded in the scenario: (1) Five services with conditional branches. In choreography the workflow exists only as an emergent property of who subscribes to what — the scholarship and lab-course branches would be encoded as subscription conditions scattered across services, with no single artefact stating the enrolment rules. An orchestrator holds the workflow, branches included, in one readable place. (2) The support phone call. Orchestration persists saga state per enrolment, so "step 3 of 5, awaiting Seat-Inventory reply" is one row to query; choreography would mean correlating events across six services' logs and hoping a correlation id was attached. (3) Compensation ownership is explicit. When Billing fails after seats are held, the orchestrator's compensation stack records exactly which steps ran and therefore which undos are owed. Choreography can do the same job correctly — several services each running their own compensation in response to one failure event is normal — the broker just delivers that event once per subscriber — but nothing states who owns which undo, so as the workflow grows it becomes easy for a compensation to be claimed twice or by nobody. (Note the risk is unclear ownership, plus at-least-once redelivery of the failure event — not the mere fact that two services subscribe to it.)

The two supporting patterns. Idempotency keys — flaky wifi means at-least-once delivery, so a retried "enrol" request must not consume a second seat or bill the student twice; the orchestrator sends a key with every command and each participant claims it atomically before acting. Dead letter queues — when a step fails past its retry ladder, or a compensation itself fails, the message goes to a DLQ with an alert, so a stuck enrolment becomes a visible item for a human rather than silently vanishing during the busiest week of the year.

The honest cost. The orchestrator is a new service to build, deploy, scale and monitor, and it is both a single point of failure and a change bottleneck — every new enrolment rule edits it. Sagas also give up isolation: a partially-enrolled student is visible to other queries mid-saga, which is why the enrolment record needs an explicit PENDING status rather than pretending the intermediate state doesn't exist.

That your compensations unwind in reverse order and only for steps that actually happened; that a failed undo is escalated to a human rather than swallowed, that the compensations below it on the stack still run, and that the result says CompensationFailed — because "we tried to undo it" and "we undid it" are the same outcome only if you never look at the data afterwards; that a duplicate request replays the stored outcome instead of charging twice — including when that outcome was a failure, which is the case almost every implementation forgets; and that a malformed payload is dead-lettered on attempt 1 rather than burning the whole retry ladder to reach the same conclusion. The reference implementation is in Solution/ — attempt first.

Each step of a saga commits locally in its own service's database, so by the time a later step fails there is nothing left to roll back — the earlier transactions are durable and visible, and no global transaction manager exists to undo them. A compensation is therefore a new business transaction that offsets an old one, and both remain in the record: refunding a payment is not the same as never charging it, since the customer saw the charge and the statement shows two lines. Some compensations are only partial — cancelling a booking may keep a cancellation fee — and some are impossible: once Notifications has emailed "you are enrolled in CS3400", no compensation un-sends it; the best you can do is send a correction, which is itself a new action with its own consequences.

// Encapsulated value object — the rating enforces its own rule
public class MaturityRating
{
    public string Code { get; }
    private readonly int _minAge;
    public MaturityRating(string code, int minAge) { Code = code; _minAge = minAge; }
    public bool IsAppropriateFor(int userAge) => userAge >= _minAge;
}

public class Genre { public string Name { get; } public Genre(string name) => Name = name; }

// The abstraction the WatchList depends on (NFR: abstraction)
public interface IWatchable
{
    string Title { get; }
    MaturityRating Rating { get; }
    IReadOnlyList<Genre> Genres { get; }
    int GetDurationMinutes();
}

public class Movie : IWatchable      { /* has-a List<Genre>; single duration + stream URL */ }
public class Episode : IWatchable    { /* season/episode numbers; owned by a Season       */ }
public class Season                  { /* composition: has Episodes                        */ }
public class TvSeries                { /* composition: has Seasons                         */ }

The arguments: genres are composition — a Movie has a list of Genre tags, so "Horror and Comedy simultaneously" is just two list entries; single inheritance makes it structurally impossible (a class has one parent) — this is Section 3's lesson applied. IWatchable is the abstraction that lets the WatchList hold movies and episodes uniformly (program to an interface). Series→Season→Episode is composition (an episode does not outlive its series — Chapter 1 lifetime argument). MaturityRating is an encapsulated value object: the rule (min age) lives with the data (the code), so the check can't be forgotten or duplicated as scattered string comparisons.

public interface ISubscriptionPlan          // the Strategy interface
{
    decimal MonthlyCost { get; }
    int MaxScreens { get; }
    bool CanStream4K { get; }
    bool CanDownload { get; }
}
public class BasicPlan : ISubscriptionPlan    { /* 8.99, 1, false, false */ }
public class StandardPlan : ISubscriptionPlan { /* 12.99, 2, false, false */ }
public class PremiumPlan : ISubscriptionPlan  { /* 19.99, 4, true, true  */ }

public class User                            // the Context
{
    public ISubscriptionPlan Plan { get; private set; }
    public void ChangePlan(ISubscriptionPlan newPlan) => Plan = newPlan;  // runtime swap
    public bool TryWatch4K() => Plan.CanStream4K;   // ask the plan — no type checks
}

Strategy (Chapter 5): each tier is an interchangeable policy object behind ISubscriptionPlan; the User delegates every privilege question to its current plan, so upgrading is one reference assignment and takes effect on the very next call — that's the required "immediate benefit change". BillingService iterates users summing user.Plan.MonthlyCost — it depends only on the abstraction, so Family Plan is a new class and nothing else (OCP). The payment token stays private inside User/billing — the ContentPlayer never sees it (encapsulation NFR).

public interface IRegionValidator { bool IsAllowed(string contentId, string region); }
public interface IAnalyticsLogger { void LogStart(string title); }

// ISP: two capabilities, separately promised
public interface IStreamable   { string GetStreamUrl(); }
public interface IDownloadable { double GetDownloadSizeMb(); }

public class StandardMovie  : IStreamable, IDownloadable { /* both */ }
public class LiveSportEvent : IStreamable                { /* stream only */ }

public class VideoPlayer
{
    private readonly IRegionValidator _validator;
    private readonly IAnalyticsLogger _logger;
    public VideoPlayer(IRegionValidator validator, IAnalyticsLogger logger)  // DI
    { _validator = validator; _logger = logger; }

    public void Play(IStreamable content, string region) { /* validate, log, play */ }
}

public class DownloadManager
{
    public void Queue(IDownloadable content) { /* compile-time safety: */ }
    // DownloadManager.Queue(liveSportEvent)  ← does not compile. ISP did its job.
}

Dependency Injection (the mechanism serving DIP): VideoPlayer receives IRegionValidator/IAnalyticsLogger through its constructor and never news them, so a test injects MockRegionValidator (always-allow) to simulate a traveller without any real geo service. Interface Segregation (ISP): splitting IStreamable from IDownloadable means LiveSportEvent never carries a lying GetDownloadSizeMb() that throws — and DownloadManager.Queue(IDownloadable …) makes the illegal call unrepresentable at compile time, which beats any runtime is-check.

Structure: IRecommendationAlgorithm { IReadOnlyList<string> Generate(User u); }, two concrete algorithms, and a RecommendationService holding the current one (Strategy again — polymorphic Generate call, swap method for admins). The principle in "swap without unexpected behaviour" is Liskov Substitution (LSP): every implementation must honour the interface's behavioural contract, not just its signature — same preconditions (any valid user, including one with zero history), same guarantees (non-null list, possibly empty, no exceptions for valid input, no hidden side effects like mutating the user). If HistoryBased threw on a new user, it would compile fine and still violate LSP — the dashboard would "behave unexpectedly", which is precisely what the requirement forbids. LSP is what makes the Strategy swap safe rather than merely possible.

Wire it in Program.cs top-level statements; each proof is 3–6 lines given the Phase-model classes. If a proof feels hard to write, the corresponding design decision is wrong — e.g. if proof 2 needs an if (user is PremiumUser) anywhere, Phase 2 regressed to type-checking. Want the same rhythm with a harness? Labs C03 (composition) and C02 (test-first) are the drilled versions of proofs 1 and 2's underlying skills.

Example 1 — new content type (say, Documentary): one new class implementing IWatchable (and IStreamable/IDownloadable as appropriate). The WatchList, VideoPlayer and DownloadManager don't change — they depend only on the abstractions (program to an interface, DIP), and genres attach by composition so no tree is disturbed. Example 2 — new plan (Family Plan): one new ISubscriptionPlan implementation; User and BillingService are untouched because privileges and cost are answered by the plan object itself (Strategy + OCP), and LSP guarantees the newcomer behaves at every site the old plans did. The pattern behind both: every axis of change was identified up front and put behind an abstraction (encapsulate what varies) — so change arrives as addition, and the "core logic" the requirement protects is precisely the set of classes that only ever see interfaces.