Conversation
- Rename factorymethod package to simplefactory: the implementation is a simple/static factory, not the GoF Factory Method; the README now teaches the distinction explicitly instead of blurring it - Correct the "one place to change" claim: adding a payment method touches the enum and the switch (one package, compiler-checked), not "the factory and nothing else" - Name the free-shipping constants in StandardShipping/ExpressShipping so the OCP example no longer contains the exact magic-number code anti-patterns.md warns against; link it as the counter-example - Make OrderCheck.chainOf clear the last handler's next so relinking the same checks into a shorter chain cannot retain a stale tail; document the one-chain-per-handler constraint and add a regression test - Facade test now records payment attempts, so "no stock means no charge" actually asserts no charge happened - Label the CSV/HTML report formatters as deliberately simplified (no quoting/escaping) so nobody copies them as serializers - Reframe the excluded-patterns list as scope choices rather than claims that modern Java has superseded those patterns Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
An external review flagged several places where the repo teaches something its own code or docs contradict. Since the whole point is clear teaching, each fix keeps the examples minimal but makes the claims honest.
1. "Factory Method" was actually a Simple Factory
The GoF Factory Method's defining feature is subclasses overriding a creator method; a static
switchfactory is the simple factory (Effective Java Item 1 draws exactly this line). The package is renamedcreational/simplefactory, and its README now has a "Not the GoF Factory Method" section teaching the distinction instead of blurring it. Links updated in the main README, anti-patterns and use-with-judgement docs.2. "One place to change" overclaimed
Adding a payment method touches the enum and the switch. The benefit is now stated accurately: one package, compiler-checked, callers untouched.
3. The repo violated its own magic-number lesson
docs/anti-patterns.mdused a hard-coded25.00free-shipping check as its example of bad code -- andStandardShippingcontained exactly that code. Both shipping classes now use named constants (FREE_SHIPPING_THRESHOLD, etc.), and the anti-patterns doc linksStandardShippingas the live counter-example.4. Chain of Responsibility could retain a stale tail
chainOfnever cleared the last handler'snext, so relinking the same instances into a shorter chain silently kept the old tail. Fixed, documented (handlers belong to one chain at a time), README notes the linked-handler shape's limitation honestly, and a regression test proves the relink case.5. Facade test didn't prove its own name
"No stock means no charge, ever" never observed the payment service. A small recording fake now asserts zero charge attempts.
6. Report formatters could be copied as serializers
CSV/HTML formatters are now explicitly labelled as deliberately simplified (no RFC 4180 quoting / HTML escaping) in javadoc and README -- the lesson is the Template Method skeleton, not serialization.
7. Excluded-patterns list stated opinions as facts
"Covered by Runnable", "superseded by DI" etc. are reframed as deliberate scope choices with honest nuance (e.g. Visitor still shines in AST tooling).
Verification
mvn -B verifypasses locally: Checkstyle clean, 47/47 tests green (two new tests: chain relink regression, facade no-charge assertion).🤖 Generated with Claude Code