Fix teaching accuracy issues from external review

README.md

@@ -20,7 +20,7 @@ Every pattern has:
 | Category | Pattern | Bookshop example | Key benefit |
 |---|---|---|---|
 | Creational | [Builder](src/main/java/com/bookshop/creational/builder/README.md) | Assembling an `Order` with optional gift message, promo code, express delivery | Readable construction of immutable objects; no telescoping constructors |
-| Creational | [Factory Method](src/main/java/com/bookshop/creational/factorymethod/README.md) | Creating card/PayPal/gift-card processors from a `PaymentMethod` | Callers never couple to concrete classes; new methods touch one place |
+| Creational | [Simple Factory](src/main/java/com/bookshop/creational/simplefactory/README.md) | Creating card/PayPal/gift-card processors from a `PaymentMethod` | Callers never couple to concrete classes; a new method is one compiler-checked change |
 | Creational | [Singleton](src/main/java/com/bookshop/creational/singleton/README.md) | Shop-wide config and feature flags via the enum idiom | Guaranteed single shared instance -- with an honest "prefer DI" caveat |
 | Structural | [Adapter](src/main/java/com/bookshop/structural/adapter/README.md) | Wrapping a legacy pence-and-status-codes payment gateway behind a clean interface | Incompatible APIs cooperate; the ugliness is quarantined in one class |
 | Structural | [Decorator](src/main/java/com/bookshop/structural/decorator/README.md) | Stacking gift wrap, express handling and greeting card onto an order | Combine extras at runtime without 2^n subclasses |
@@ -67,17 +67,22 @@ mvn verify   # lints (Checkstyle) and runs every pattern's tests
 
 ## What's deliberately left out -- and why
 
-Half the Gang of Four catalogue rarely appears in modern application code. Excluded on purpose:
+This is a curated list, not a complete catalogue: the bar for inclusion is "working Java
+developers hand-write this regularly". The rest of the Gang of Four is left out as a scope
+choice -- these patterns are real and still appear, just not often enough in ordinary application
+code to earn a chapter here:
 
-- **Command** -- its niche (tasks as objects) is covered by `Runnable` and lambdas; hand-rolled
-  undo stacks are rare.
+- **Command** -- when the need is simply "a task as an object", `Runnable` and lambdas usually
+  carry it; the full pattern (queues, undo stacks, audit logs of operations) is genuinely useful
+  but comparatively rare.
 - **State** -- real projects usually reach for an enum + switch or a state-machine library rather
   than the class-per-state shape.
 - **Composite** -- common *inside* UI/tree libraries, rarely hand-written in application code.
 - **Iterator** -- absorbed into the language: `Iterable` and for-each.
 - **Abstract Factory, Prototype, Bridge, Flyweight, Visitor, Mediator, Memento, Interpreter** --
-  superseded by dependency injection, enums and lambdas, or simply too rare to earn a place in a
-  "patterns you'll actually use" list.
+  each has its niche (a DI container does much of Abstract Factory's everyday job; Visitor still
+  shines in compilers and AST tooling), but in typical application code they're rare enough that a
+  focused list serves you better than a complete one.
 
 If you're deciding whether to use a pattern at all: the tests here show the *problem* each pattern
 earns its keep on. No matching problem, no pattern -- the simplest code that works wins. The full

docs/anti-patterns.md

@@ -34,9 +34,10 @@ if (status == 3) { refund(order); }                                  // 3 means.
 **How it happens:** the value was obvious to whoever typed it, that afternoon. **The damage:** the
 business rule is unfindable and unsearchable; when "free shipping over GBP 25" becomes GBP 30, some of
 the 25.00s get updated (see [copy-paste programming](#3-copy-paste-programming)); `status == 3`
-invites off-by-one bugs no compiler can catch. **The counter:** named constants
-(`FREE_SHIPPING_THRESHOLD`) and enums --
-[`PaymentMethod`](../src/main/java/com/bookshop/creational/factorymethod/PaymentMethod.java.md) makes
+invites off-by-one bugs no compiler can catch. **The counter:** named constants --
+[`StandardShipping`](../src/main/java/com/bookshop/solid/ocp/StandardShipping.java.md)'s
+`FREE_SHIPPING_THRESHOLD` names exactly this rule -- and enums --
+[`PaymentMethod`](../src/main/java/com/bookshop/creational/simplefactory/PaymentMethod.java.md) makes
 invalid payment types unrepresentable, and the compiler checks every `switch` over it.
 
 ## 3. Copy-paste programming

docs/use-with-judgement.md

@@ -12,7 +12,7 @@ The benefit column in this repo's tables is real, but so is the invoice:
 | Tool | Solves | Costs when the problem isn't there |
 |---|---|---|
 | Builder | Many optional fields | Fifty lines of ceremony for a two-field class -- a constructor was clearer |
-| Factory Method | Callers coupled to concrete types | An indirection layer over a single `new` that never varies |
+| Simple Factory | Callers coupled to concrete types | An indirection layer over a single `new` that never varies |
 | Strategy | Multiple interchangeable algorithms | An interface, a context, and one lonely implementation -- an `if` was clearer |
 | Interfaces everywhere | Multiple implementations / substitution in tests | `BookService` + `BookServiceImpl` pairs that only ever have one member |
 | SRP | Unrelated reasons-to-change tangled together | Logic shattered into confetti -- ten two-line classes nobody can follow |

src/main/java/com/bookshop/behavioral/chainofresponsibility/OrderCheck.java

@@ -10,14 +10,23 @@ public abstract class OrderCheck {
 
     private OrderCheck next;
 
-    /** Links the given checks in order and returns the head of the chain. */
+    /**
+     * Links the given checks in order and returns the head of the chain.
+     *
+     * <p>Linking stores each handler's successor in the handler itself, so a handler
+     * instance belongs to <em>one</em> chain at a time -- to assemble several pipelines
+     * from the same parts, build each from fresh instances. Relinking the same
+     * instances into a new chain is safe: every {@code next} is overwritten, including
+     * the last handler's, so no tail from a previous, longer chain survives.
+     */
     public static OrderCheck chainOf(List<OrderCheck> checks) {
         if (checks.isEmpty()) {
             throw new IllegalArgumentException("chain needs at least one check");
         }
         for (int i = 0; i < checks.size() - 1; i++) {
             checks.get(i).next = checks.get(i + 1);
         }
+        checks.get(checks.size() - 1).next = null;
         return checks.get(0);
     }
 

src/main/java/com/bookshop/behavioral/chainofresponsibility/README.md

@@ -28,7 +28,10 @@ This is the servlet-filter / middleware shape: the pipeline is *configuration*,
 - **Short-circuiting for free** -- a rejection stops the chain; later (possibly expensive) checks
   never run.
 - **Separation of *what checks exist* from *which run*** -- different channels (web, phone, trade)
-  can assemble different chains from the same parts.
+  can each assemble their own chain from the same check *classes*. (One honest limitation of the
+  linked-handler shape: each handler *instance* stores its successor, so it belongs to one chain
+  at a time -- give each chain its own instances. An immutable list-based pipeline avoids this at
+  the cost of a less canonical shape.)
 
 ## Seen in the wild
 

src/main/java/com/bookshop/behavioral/templatemethod/CsvSalesReport.java

@@ -3,7 +3,13 @@
 import com.bookshop.domain.Book;
 import java.math.BigDecimal;
 
-/** CSV variant: supplies only the formatting steps, inherits the algorithm. */
+/**
+ * CSV variant: supplies only the formatting steps, inherits the algorithm.
+ *
+ * <p>Deliberately simplified: real CSV output must quote fields containing commas or
+ * quotes (RFC 4180) -- use a CSV library for that. The lesson here is the Template
+ * Method shape, not CSV serialization.
+ */
 public class CsvSalesReport extends SalesReport {
 
     @Override

src/main/java/com/bookshop/behavioral/templatemethod/HtmlSalesReport.java

@@ -3,7 +3,13 @@
 import com.bookshop.domain.Book;
 import java.math.BigDecimal;
 
-/** HTML variant: different formatting, identical structure and totalling logic. */
+/**
+ * HTML variant: different formatting, identical structure and totalling logic.
+ *
+ * <p>Deliberately simplified: real HTML output must escape {@code < > &} in field
+ * values -- use a templating library for that. The lesson here is the Template Method
+ * shape, not HTML generation.
+ */
 public class HtmlSalesReport extends SalesReport {
 
     @Override

src/main/java/com/bookshop/behavioral/templatemethod/README.md

@@ -25,6 +25,10 @@ String html = new HtmlSalesReport().generate(sales); // different steps
   never *whether the footer comes last*.
 - **Adding a format is trivial** -- implement three small methods; the hard part is inherited.
 
+One caveat so nobody copies these as serializers: the formatters are deliberately minimal. Real
+CSV needs field quoting (RFC 4180) and real HTML needs escaping -- both jobs for a library. The
+lesson here is the *skeleton*, not the formats.
+
 ## A note on modern practice
 
 When the base class would have only one hook, prefer passing a lambda (Strategy) instead of

src/main/java/com/bookshop/creational/factorymethod/PaymentMethod.java → src/main/java/com/bookshop/creational/simplefactory/PaymentMethod.java

@@ -1,4 +1,4 @@
-package com.bookshop.creational.factorymethod;
+package com.bookshop.creational.simplefactory;
 
 /** The payment options the shop offers at checkout. */
 public enum PaymentMethod {

src/main/java/com/bookshop/creational/factorymethod/PaymentProcessor.java → src/main/java/com/bookshop/creational/simplefactory/PaymentProcessor.java

@@ -1,4 +1,4 @@
-package com.bookshop.creational.factorymethod;
+package com.bookshop.creational.simplefactory;
 
 import java.math.BigDecimal;
 

src/main/java/com/bookshop/creational/factorymethod/PaymentProcessors.java → src/main/java/com/bookshop/creational/simplefactory/PaymentProcessors.java

@@ -1,9 +1,9 @@
-package com.bookshop.creational.factorymethod;
+package com.bookshop.creational.simplefactory;
 
 import java.math.BigDecimal;
 
 /**
- * The factory method: the only place in the codebase that knows which concrete
+ * The simple factory: the only place in the codebase that knows which concrete
  * processor class backs each {@link PaymentMethod}.
  */
 public final class PaymentProcessors {

src/main/java/com/bookshop/creational/factorymethod/PaymentReceipt.java → src/main/java/com/bookshop/creational/simplefactory/PaymentReceipt.java

@@ -1,4 +1,4 @@
-package com.bookshop.creational.factorymethod;
+package com.bookshop.creational.simplefactory;
 
 import java.math.BigDecimal;
 

src/main/java/com/bookshop/creational/factorymethod/README.md → src/main/java/com/bookshop/creational/simplefactory/README.md

@@ -1,4 +1,4 @@
-# Factory Method
+# Simple Factory (Static Factory)
 
 ## The problem
 
@@ -8,7 +8,7 @@ about every concrete class -- and adding a payment method means hunting down all
 
 ## The pattern
 
-Creation is centralised behind one factory method. Callers name *what* they want
+Creation is centralised behind one static factory method. Callers name *what* they want
 (a `PaymentMethod`), not *how* it's built:
 
 ```java
@@ -17,22 +17,33 @@ PaymentReceipt receipt = processor.process(total);
 ```
 
 The concrete classes are `private` nested types -- clients *cannot* couple to them even if they try.
-This is the shape the pattern almost always takes in modern Java: a static factory method returning
-an interface. (The GoF "subclass overrides the creator" variant survives mostly inside frameworks.)
+
+## Not the GoF "Factory Method"
+
+This shape is often loosely called "Factory Method", but the Gang of Four pattern of that name is
+something more specific: an abstract *creator* class whose **subclasses override** a method to
+decide which product to build. What this package shows -- one static method with a `switch` -- is
+the **simple factory** (Effective Java's "static factory method", Item 1), and it's what working
+Java code almost always uses. The true GoF shape survives mostly inside frameworks, where the
+framework owns the algorithm and your subclass supplies the objects (e.g. overriding a
+`createXyz()` hook). If an interviewer or a design review says "Factory Method", it's worth
+checking which of the two they mean.
 
 ## Benefits
 
 - **Decoupling** -- checkout depends only on the `PaymentProcessor` interface; concrete classes can
   be renamed, replaced, or rewritten freely.
-- **One place to change** -- supporting a new payment method touches the factory and nothing else;
-  the exhaustive `switch` over the enum makes the compiler flag any case you forget.
+- **One package to change** -- supporting a new payment method means adding an enum constant and a
+  `switch` case, both in this package; because the `switch` over the enum is exhaustive, the
+  compiler flags every spot you forget. Callers don't change at all.
 - **Hidden implementation choice** -- the factory may return a new instance, a cached one, or a
   subtype; callers can't tell and don't care.
 
 ## Seen in the wild
 
 `List.of()`, `Optional.of()`, `Files.newBufferedReader()`, JDBC's `DriverManager.getConnection()`,
-`Executors.newFixedThreadPool()`, Spring's `FactoryBean`.
+`Executors.newFixedThreadPool()`. Spring's `FactoryBean` and servlet/framework `createXyz()` hooks
+are where the GoF subclass-driven variant still earns its keep.
 
 ## Implementation
 
@@ -44,4 +55,4 @@ an interface. (The GoF "subclass overrides the creator" variant survives mostly
 
 ## Example test
 
-[`PaymentProcessorsTest`](../../../../../../test/java/com/bookshop/creational/factorymethod/PaymentProcessorsTest.java.md)
+[`PaymentProcessorsTest`](../../../../../../test/java/com/bookshop/creational/simplefactory/PaymentProcessorsTest.java.md)

src/main/java/com/bookshop/solid/ocp/ExpressShipping.java

@@ -2,16 +2,18 @@
 
 import java.math.BigDecimal;
 
-/** Next-day courier: flat 7.50. */
+/** Next-day courier: flat rate, never free. */
 public class ExpressShipping implements ShippingRate {
 
+    static final BigDecimal EXPRESS_COST = new BigDecimal("7.50");
+
     @Override
     public String name() {
         return "express";
     }
 
     @Override
     public BigDecimal costFor(BigDecimal orderTotal) {
-        return new BigDecimal("7.50");
+        return EXPRESS_COST;
     }
 }

src/main/java/com/bookshop/solid/ocp/StandardShipping.java

@@ -2,18 +2,22 @@
 
 import java.math.BigDecimal;
 
-/** Standard post: 3.00, free for orders of 25.00 or more. */
+/** Standard post: flat rate, free above the threshold. */
 public class StandardShipping implements ShippingRate {
 
+    /** Business rule: "free shipping over GBP 25" -- named so the rule is findable and searchable. */
+    static final BigDecimal FREE_SHIPPING_THRESHOLD = new BigDecimal("25.00");
+    static final BigDecimal STANDARD_COST = new BigDecimal("3.00");
+
     @Override
     public String name() {
         return "standard";
     }
 
     @Override
     public BigDecimal costFor(BigDecimal orderTotal) {
-        return orderTotal.compareTo(new BigDecimal("25.00")) >= 0
+        return orderTotal.compareTo(FREE_SHIPPING_THRESHOLD) >= 0
                 ? BigDecimal.ZERO
-                : new BigDecimal("3.00");
+                : STANDARD_COST;
     }
 }

src/test/java/com/bookshop/behavioral/chainofresponsibility/OrderValidationChainTest.java

@@ -55,6 +55,18 @@ void chainOrderIsConfigurable() {
         assertEquals("address", addressFirst.validate(request).rejectedBy());
     }
 
+    @Test
+    @DisplayName("relinking the same checks into a shorter chain drops the old tail")
+    void relinkedShorterChainHasNoStaleTail() {
+        StockCheck stock = new StockCheck(Set.of(rareBook.isbn()));
+        OrderCheck.chainOf(List.of(stock, new FraudCheck()));
+        OrderRequest fraudWouldReject = new OrderRequest(newBob, List.of(rareBook), "1 High Street");
+
+        OrderCheck stockOnly = OrderCheck.chainOf(List.of(stock));
+
+        assertTrue(stockOnly.validate(fraudWouldReject).valid());
+    }
+
     @Test
     @DisplayName("each check is a small unit, testable entirely on its own")
     void checksAreIndependentlyTestable() {

src/test/java/com/bookshop/creational/factorymethod/PaymentProcessorsTest.java → src/test/java/com/bookshop/creational/simplefactory/PaymentProcessorsTest.java

@@ -1,4 +1,4 @@
-package com.bookshop.creational.factorymethod;
+package com.bookshop.creational.simplefactory;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
@@ -7,7 +7,7 @@
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.CsvSource;
 
-@DisplayName("Factory Method -- checkout never touches a concrete processor class")
+@DisplayName("Simple Factory -- checkout never touches a concrete processor class")
 class PaymentProcessorsTest {
 
     @ParameterizedTest(name = "{0} -> handled by the {1} provider")

src/test/java/com/bookshop/structural/facade/CheckoutFacadeTest.java

@@ -19,13 +19,26 @@ class CheckoutFacadeTest {
             new BigDecimal("32.50"));
     private final Customer alice = new Customer("c-1", "Alice", "alice@example.com", 1);
 
+    /** Records every charge so tests can assert what the facade did -- or didn't -- ask for. */
+    private static class RecordingPaymentService extends PaymentService {
+        private int chargesAttempted;
+
+        @Override
+        public String charge(Customer customer, BigDecimal amount) {
+            chargesAttempted++;
+            return super.charge(customer, amount);
+        }
+    }
+
     private InventoryService inventory;
+    private RecordingPaymentService payments;
     private CheckoutFacade checkout;
 
     @BeforeEach
     void setUp() {
         inventory = new InventoryService();
-        checkout = new CheckoutFacade(inventory, new PaymentService(), new ShippingService());
+        payments = new RecordingPaymentService();
+        checkout = new CheckoutFacade(inventory, payments, new ShippingService());
     }
 
     @Test
@@ -46,6 +59,8 @@ void oneCallDoesEverything() {
     void outOfStockStopsTheFlowBeforePayment() {
         assertThrows(IllegalStateException.class,
                 () -> checkout.checkout(alice, List.of(book), "1 High Street"));
+
+        assertEquals(0, payments.chargesAttempted, "payment must never be attempted without stock");
         assertEquals(0, inventory.stockLevel(book));
     }
 }