By now you can draw the library six ways. This chapter is about judging a design from its diagrams: recognising a known shape (a design pattern), spotting a bad one (a god class, a cycle, an anemic model), and changing a design safely, on paper first. It finishes with a real change to the library, made in the order a careful team would work: review the current design, redraw it, then change the code and check that nothing broke.
What was run, and what was not
Run here (Python 3.14, Mermaid 12.1.0): the library from chapter 9 refactored for two new requirements (the 10 old tests pass unchanged, and 13 more were added); five pattern diagrams each checked against the code by a script, which I also tampered with to prove it can fail; three smell detectors I wrote, run on the course's own code and on a deliberately flawed module; and a dependency check on a package cycle and its fix.
Not re-read: the pattern catalogue (the "Gang of Four" book) is from general knowledge; patterns are explained, with their trade-offs, in Design Patterns (State is its chapter 8), and SOLID in Clean Code, SOLID & Refactoring 6 and 7. This chapter shows the diagrams, and does not re-teach the patterns. The detectors' thresholds are my own and arbitrary; they are prompts for a review, not verdicts.
Five Patterns, Drawn and Checked
A pattern is a recurring arrangement of classes that solves a recurring problem. On a diagram it is a shape: if you can recognise the shape, you know the intent without reading the code. For each pattern below there is a diagram using the library's own names, the code it describes, and a note on where the diagram and code agree.
Pattern
Problem it solves
In the library
Strategy
A rule that varies, to be chosen at run time
FinePolicy (how a fine is worked out), added in this chapter
Observer
Tell interested parties when something happens, without the source knowing them
LoanDesk publishes events to Subscribers
State
Behaviour that depends on the object's state
The loan lifecycle of chapter 8, one class per state
Factory
One place that decides which class to create
KindFactory creating the right MemberKind
Composite
Treat one thing and a group of things the same way
Holdings: a Single copy or a Shelf of them
Strategy
The shape: a client that holds (or is given) an interface, and several classes that realise it. Member.give_back uses a FinePolicy without knowing which; PerDayFine is one.
"""How a fine is worked out. A Strategy: the rule can be replaced without touching Member, Loan or the desk."""
from __future__ import annotations
from typing import TYPE_CHECKING, Protocol, runtime_checkable
if TYPE_CHECKING: # only for the type hints: no import at run time, so no import cycle with model.py
from library.domain.model import MemberKind
ITEM_MULTIPLIER = {"book": 1, "dvd": 2}
@runtime_checkable
class FinePolicy(Protocol):
def fine(self, days_overdue: int, kind: MemberKind, item_type: str) -> int: ...
class PerDayFine:
"""Days overdue times the member kind's daily rate times the item type's multiplier (a DVD costs double)."""
def fine(self, days_overdue: int, kind: MemberKind, item_type: str) -> int:
return max(0, days_overdue) * kind.fine_per_day() * ITEM_MULTIPLIER[item_type]
Observer
The shape: a subject with a list of subscribers behind an interface, and events it publishes. The desk knows Subscriber, never ConsoleNotifier.
State
The shape: a context that points at a current state object, and one class per state. This is exactly the code of chapter 8 (loan_state_pattern.py); the diagram is checked against it. Active is the composite state from chapter 8's diagram, here as a shared parent class.
Factory and Composite
Factory: callers ask KindFactory.create("junior") and get a MemberKind. Only the factory names Standard and Junior.
Composite: a Shelf is a Holdings and holds Holdings (single copies or other shelves), so count() recurses.
"""Two small patterns the library does not otherwise need, written so every diagram in the chapter has real code behind it."""
from abc import ABC, abstractmethod
from library.domain import Copy, Junior, MemberKind, Refusal, Standard
class KindFactory:
"""Factory: one place that turns a name into the right MemberKind, so callers do not know the concrete classes."""
registry = {"standard": Standard, "junior": Junior}
@classmethod
def create(cls, name: str) -> MemberKind:
if name not in cls.registry:
raise Refusal(f"unknown member kind {name!r}")
return cls.registry[name]()
class Holdings(ABC):
"""Composite: a single copy and a whole shelf of things can both be asked how many copies they hold."""
@abstractmethod
def count(self) -> int: ...
class Single(Holdings):
def __init__(self, copy: Copy):
self.copy = copy
def count(self) -> int:
return 1
class Shelf(Holdings):
def __init__(self, label: str):
self.label = label
self.children: list[Holdings] = []
def add(self, child: Holdings) -> "Shelf":
self.children.append(child)
return self
def count(self) -> int:
return sum(child.count() for child in self.children)
Are the Diagrams True?
A pattern diagram is only worth having if it matches the code. The check below reads each diagram and verifies, against the source files, that every drawn operation and attribute exists, that every drawn inheritance is real, and that every drawn realisation is real (by actually testing, at run time, that the class fits the interface):
"""Check each pattern diagram against the code it describes: operations and attributes exist, inheritance is real, realisation is real.
Names only: this cannot tell whether the pattern is applied well, just that the diagram is not lying about what exists.
"""
import ast
import importlib
import pathlib
import re
HERE = pathlib.Path(__file__).parent
CHECKS = {
"strategy.mmd": ["library/domain/fines.py", "library/domain/model.py", "library/application/desk.py"],
"observer.mmd": ["library/application/desk.py", "library/application/ports.py", "library/application/events.py", "library/infrastructure/notifier.py"],
"state.mmd": ["loan_state_pattern.py"],
"factory.mmd": ["patterns_extra.py", "library/domain/model.py"],
"composite.mmd": ["patterns_extra.py"],
"before.mmd": ["library_before/domain/model.py", "library_before/application/desk.py", "library_before/application/ports.py"],
"after.mmd": ["library/domain/fines.py", "library/domain/model.py", "library/application/desk.py", "library/application/ports.py",
"library/infrastructure/notifier.py"],
}
MODULES = ["library.domain", "library.application", "library.infrastructure", "patterns_extra", "loan_state_pattern"]
def diagram(path: pathlib.Path):
ops, attrs, inherits, realises, current = {}, {}, [], [], None
for line in path.read_text(encoding="utf8").splitlines():
c = re.match(r"\s*class (\w+)", line)
if c:
current = c.group(1)
ops.setdefault(current, set())
attrs.setdefault(current, set())
if line.rstrip().endswith("}") is False and "{" not in line:
current = None if False else current
continue
o = re.match(r"\s*\+(\w+)\(.*\)", line)
if o and current:
ops[current].add(o.group(1))
continue
a = re.match(r"\s*\+(\w+):", line)
if a and current:
attrs[current].add(a.group(1))
continue
i = re.match(r"\s*(\w+) --\|> (\w+)", line)
if i:
inherits.append((i.group(1), i.group(2)))
r = re.match(r"\s*(\w+) \.\.\|> (\w+)", line)
if r:
realises.append((r.group(1), r.group(2)))
return ops, attrs, inherits, realises
def code(paths):
methods, fields, bases = {}, {}, {}
for rel in paths:
for node in ast.parse((HERE / rel).read_text(encoding="utf8")).body:
if isinstance(node, ast.ClassDef):
methods.setdefault(node.name, set()).update(f.name for f in node.body if isinstance(f, ast.FunctionDef))
fields.setdefault(node.name, set()).update(s.target.id for s in node.body if isinstance(s, ast.AnnAssign) and isinstance(s.target, ast.Name))
bases[node.name] = {b.id for b in node.bases if isinstance(b, ast.Name)}
return methods, fields, bases
def find_class(name: str):
for module in MODULES:
mod = importlib.import_module(module)
if hasattr(mod, name):
return getattr(mod, name)
try: # classes not exported by a package's __init__
for dotted in ("library.domain.model", "library.domain.fines", "library.application.ports", "library.application.events"):
mod = importlib.import_module(dotted)
if hasattr(mod, name):
return getattr(mod, name)
except ImportError:
pass
return None
def main() -> int:
problems = 0
for name, files in CHECKS.items():
ops, attrs, inherits, realises = diagram(HERE / "diagrams" / name)
methods, fields, bases = code(files)
issues = []
for cls, wanted in ops.items():
if cls in methods:
issues += [f"{cls}.{op}() is drawn but not in the code" for op in sorted(wanted - methods[cls])]
for cls, wanted in attrs.items():
if wanted and cls in fields:
issues += [f"{cls}.{a} is drawn but is not a field in the code" for a in sorted(wanted - fields[cls])]
for sub, base in inherits:
if base not in bases.get(sub, set()):
issues.append(f"{sub} --|> {base} is drawn, but {sub} does not inherit from {base} in the code")
for impl, iface in realises:
a, b = find_class(impl), find_class(iface)
try:
ok = a is not None and b is not None and issubclass(a, b)
except TypeError:
ok = False
if not ok:
issues.append(f"{impl} ..|> {iface} is drawn, but {impl} does not fit {iface} in the code")
print(f"{name:<14} {'OK' if not issues else str(len(issues)) + ' problem(s)'}")
for i in issues:
print(" ", i)
problems += len(issues)
return 1 if problems else 0
if __name__ == "__main__":
raise SystemExit(main())
strategy.mmd OK
observer.mmd OK
state.mmd OK
factory.mmd OK
composite.mmd OK
before.mmd OK
after.mmd OK
All seven diagrams (the five patterns plus the library's before and after, below) pass. A check that passes first time deserves suspicion, so here it is on three deliberately tampered diagrams: an operation renamed in the strategy diagram, a wrong interface in the observer diagram, and a wrong parent in the state diagram:
strategy.mmd 1 problem(s)
FinePolicy.calculate() is drawn but not in the code
observer.mmd 1 problem(s)
ConsoleNotifier ..|> FinePolicy is drawn, but ConsoleNotifier does not fit FinePolicy in the code
state.mmd 1 problem(s)
OnLoan --|> State is drawn, but OnLoan does not inherit from State in the code
(exit status 1)
Each lie is caught and named. What the check cannot say is whether the pattern is used well: it checks names, as in chapter 10.
Principles You Can See on a Diagram
SOLID is covered properly in Clean Code, SOLID & Refactoring 6 and 7. What this course adds is that several of its ideas show up as shapes:
Dependency inversion: interfaces are owned by the client, and arrows point at them. In the after diagram below, LoanDesk has three arrows, all to interfaces (Catalogue, Subscriber, FinePolicy), and every concrete class has a dotted arrow into an interface. A concrete class with an arrow pointing out of the desk would be a sign that high-level code depends on low-level detail. Chapter 9's dependency check makes the package-level version of this a test.
Open for extension, closed for change: a new box that touches no old box. Adding a subscriber is a new class; nothing existing is edited (Exercise 2 proves it by fingerprinting the library's files before and after).
Cohesion: clusters. Classes that change together should be close together and linked; classes that do not should not be. In chapter 9's package diagram, the clusters are the packages. Inside a class, the equivalent is "do its methods use the same data?", which the god class detector below measures.
Coupling: count the arrows leaving a box. A box with arrows to everything has too many reasons to change. A box with none (Copy, in the domain) is easy to understand and test.
A Design Review Checklist
A review is a short, structured look at a design (diagrams plus a few key files) by someone other than its author, done before the cost of change is high. Questions, each with what to look for on a diagram and, where one exists, an automatic check from this course:
Question
Look for on the diagram
Automatic check
Does each class have one job?
A box with many unrelated operations; a name ending in Manager, Handler or Util
God class detector (below)
Do the dependencies point the right way?
A high-level box with arrows to concrete low-level boxes
if kind == ... chains; one concrete class per variation with no interface
Search for type-tag comparisons (chapter 6)
What will the next likely change touch?
Trace the change across the boxes: how many must be edited?
Diff statistics after a trial change (this chapter)
What if something fails or runs twice?
Calls to outside things with no failure path (the e-mail server, a second desk)
Tests for failure paths (the broken subscriber below)
Do the diagrams still match the code?
Operation or class names that do not exist
Diagram check (above), drift check (chapter 10)
Finding a God Class, a Cycle and an Anemic Model
Three common problems, and their shape on a diagram:
God class: one big box with arrows out to nearly everything, and a long list of operations that fall into several unrelated groups (lending, e-mail, reports, files, search).
Cyclic dependency: a loop of arrows, typically between packages, so that neither side can be understood or tested alone (chapter 9 showed one stop the program importing).
Anemic model: chapter 6's picture: several boxes with fields and no operations, and one service box with every operation and an arrow to each of them.
Detectors can point at candidates. These are mine, small, and honest about being crude (they measure: for a god class, how many unrelated groups a class's methods fall into, linking two methods when they use the same field or call each other, a measure called LCOM4 as far as I remember; for mutual references, which classes name each other; for anemia, how many classes hold only data and how often a service reaches into them from outside):
"""Three design smells, found by counting. My own simple detectors, written for this chapter; thresholds are arbitrary and say so.
god class a class whose methods split into unrelated groups (LCOM4 > 1: methods are linked when they use a common field or call each
other; each separate group is a separate job), together with a large number of methods
mutual refs two classes that each name the other (not always wrong: a two-way association is legitimate, but it is worth a look)
anemic classes with several fields and no behaviour, beside a class that reads and writes their fields from outside
py smells.py module.py [module.py ...]
"""
import ast
import pathlib
import sys
GOD_METHODS = 8 # arbitrary: this many methods in one class is "a lot" for this course's examples
def classes_of(path: str):
tree = ast.parse(pathlib.Path(path).read_text(encoding="utf8"))
return {n.name: n for n in tree.body if isinstance(n, ast.ClassDef)}
def fields_of(cls: ast.ClassDef) -> set[str]:
out = {s.target.id for s in cls.body if isinstance(s, ast.AnnAssign) and isinstance(s.target, ast.Name)}
for fn in cls.body:
if isinstance(fn, ast.FunctionDef) and fn.name == "__init__":
out |= {n.attr for n in ast.walk(fn) if isinstance(n, ast.Attribute) and isinstance(n.value, ast.Name) and n.value.id == "self"
and isinstance(n.ctx, ast.Store)}
return out
def lcom4(cls: ast.ClassDef) -> tuple[int, list[list[str]]]:
"""Connected groups of methods, linking two methods when they touch the same field or one calls the other."""
own = fields_of(cls)
methods = {f.name: f for f in cls.body if isinstance(f, ast.FunctionDef) and not f.name.startswith("__")}
uses, calls = {}, {}
for name, fn in methods.items():
uses[name] = {n.attr for n in ast.walk(fn) if isinstance(n, ast.Attribute) and isinstance(n.value, ast.Name) and n.value.id == "self" and n.attr in own}
calls[name] = {n.func.attr for n in ast.walk(fn) if isinstance(n, ast.Call) and isinstance(n.func, ast.Attribute)
and isinstance(n.func.value, ast.Name) and n.func.value.id == "self" and n.func.attr in methods}
parent = {m: m for m in methods}
def find(x):
while parent[x] != x:
parent[x] = parent[parent[x]]
x = parent[x]
return x
names = list(methods)
for i, a in enumerate(names):
for b in names[i + 1:]:
if uses[a] & uses[b] or b in calls[a] or a in calls[b]:
parent[find(a)] = find(b)
groups: dict[str, list[str]] = {}
for m in names:
groups.setdefault(find(m), []).append(m)
return len(groups), list(groups.values())
def named_classes(cls: ast.ClassDef, all_names: set[str]) -> set[str]:
found = set()
for n in ast.walk(cls):
if isinstance(n, ast.Name) and n.id in all_names and n.id != cls.name:
found.add(n.id)
if isinstance(n, ast.Constant) and isinstance(n.value, str): # forward references written as strings
for name in all_names:
if name != cls.name and name in n.value.replace("|", " ").split():
found.add(name)
return found
def analyse(path: str) -> list[str]:
classes = classes_of(path)
report = []
for name, node in classes.items():
methods = [f for f in node.body if isinstance(f, ast.FunctionDef) and not f.name.startswith("__")]
count, groups = lcom4(node)
if count > 1 and len(methods) >= GOD_METHODS:
report.append(f"GOD CLASS {name}: {len(methods)} methods in {count} unrelated groups: " + "; ".join("{" + ", ".join(g) + "}" for g in groups))
refs = {n: named_classes(c, set(classes)) for n, c in classes.items()}
seen = set()
for a in refs:
for b in refs[a]:
if a in refs.get(b, set()) and (b, a) not in seen:
seen.add((a, b))
report.append(f"MUTUAL {a} and {b} each name the other")
data_only = [n for n, c in classes.items() if len(fields_of(c)) >= 2 and not [f for f in c.body if isinstance(f, ast.FunctionDef) and not f.name.startswith("__")]]
if data_only:
biggest = max(classes, key=lambda n: len([f for f in classes[n].body if isinstance(f, ast.FunctionDef)]))
field_names = {f for n in data_only for f in fields_of(classes[n])}
poking = sum(1 for n in ast.walk(classes[biggest]) if isinstance(n, ast.Attribute) and not (isinstance(n.value, ast.Name) and n.value.id == "self")
and n.attr in field_names)
report.append(f"ANEMIC? {len(data_only)} of {len(classes)} classes hold only data ({', '.join(data_only)}); "
f"{biggest} reads or writes their fields {poking} times from outside")
return report
if __name__ == "__main__":
for path in sys.argv[1:]:
print(path)
for line in analyse(path) or ["(nothing flagged)"]:
print(" " + line)
The flawed module they are run on (a manager that does everything, two classes that reference each other, two data-only classes):
"""A deliberately flawed library module, for the design review exercises: a god class, mutual references, and data-only classes."""
from dataclasses import dataclass, field
from datetime import date, timedelta
@dataclass
class Copy:
barcode: str
on_loan: bool = False
@dataclass
class Fine:
card: str
pence: int
paid: bool = False
@dataclass
class Member:
card: str
email: str
loans: list = field(default_factory=list)
def record_loan(self, loan: "Loan") -> None:
self.loans.append(loan)
loan.member = self # the Member reaches into the Loan ...
@dataclass
class Loan:
barcode: str
due_on: date
member: "Member | None" = None
def overdue_fine(self, today: date) -> int:
owner: Member = self.member # ... and the Loan reaches back into the Member
return max(0, (today - self.due_on).days) * (5 if owner.card.startswith("J") else 10)
class LibraryManager:
"""Does everything: lending, fines, e-mail, reports, files, and searching."""
def __init__(self, smtp_host: str, data_file: str):
self.members: dict[str, Member] = {}
self.copies: dict[str, Copy] = {}
self.fines: list[Fine] = []
self.smtp_host = smtp_host
self.sent: list[str] = []
self.data_file = data_file
self.report_lines: list[str] = []
self.catalogue_index: dict[str, list[str]] = {}
# lending
def borrow(self, card: str, barcode: str, today: date) -> Loan:
member, copy = self.members[card], self.copies[barcode]
if copy.on_loan:
raise ValueError("already on loan")
loan = Loan(barcode, today + timedelta(days=21))
member.record_loan(loan)
copy.on_loan = True
return loan
def return_book(self, card: str, barcode: str, today: date) -> int:
member = self.members[card]
loan = next(l for l in member.loans if l.barcode == barcode)
fine = loan.overdue_fine(today)
if fine:
self.fines.append(Fine(card, fine))
self.send_email(member.email, f"You owe {fine}p")
self.copies[barcode].on_loan = False
member.loans.remove(loan)
return fine
# e-mail
def send_email(self, to: str, text: str) -> None:
self.sent.append(f"{self.smtp_host}: to {to}: {text}")
# reporting
def build_report(self) -> list[str]:
self.report_lines = [f"{f.card}: {f.pence}p" for f in self.fines if not f.paid]
return self.report_lines
def print_report(self) -> str:
return "\n".join(self.report_lines)
# persistence
def save(self) -> str:
return f"saved {len(self.members)} members to {self.data_file}"
def load(self) -> str:
return f"loaded from {self.data_file}"
# searching
def index_title(self, title: str, barcode: str) -> None:
self.catalogue_index.setdefault(title.lower(), []).append(barcode)
def search(self, word: str) -> list[str]:
return self.catalogue_index.get(word.lower(), [])
The results on the course's own earlier code and the flawed module:
library_a.py
ANEMIC? 3 of 5 classes hold only data (Member, Copy, Loan); LibraryService reads or writes their fields 22 times from outside
library_b.py
(nothing flagged)
flawed_library.py
GOD CLASS LibraryManager: 9 methods in 2 unrelated groups: {borrow, return_book, send_email, build_report, print_report, save, load}; {index_title, search}
MUTUAL Member and Loan each name the other
ANEMIC? 2 of 5 classes hold only data (Copy, Fine); LibraryManager reads or writes their fields 7 times from outside
Chapter 6's version A is flagged as anemic (3 of its 5 classes hold only data and the service reaches into them 22 times), and version B is not flagged. The detector reproduces the judgement of chapter 6 from the code alone.
The flawed module gets all three flags: the god class, the mutual reference between Member and Loan, and an anemic suspicion for Copy and Fine.
The god-class verdict is weaker than it looks. A reader sees five jobs in LibraryManager (lending, e-mail, reporting, persistence, searching). The detector found only two groups, because the jobs share fields (the member list, the fines) and one method calls another. Exercise 1 shows this. A detector's "one group" does not clear a class, and its "two" is a floor, not a count.
The mutual reference is not necessarily wrong. A member and its loans may legitimately know each other. Here the flaw is how: each reaches into the other's data (loan.member = self; owner.card.startswith("J") to choose a fine rate: a rule hiding in the wrong class, with a type test in disguise). The detector asks the question; the review answers it.
The same code scanned file by file in the current library finds nothing to flag:
And a package-level cycle, on a tiny tree where billing and members import each other, caught by chapter 9's checker:
analysed 3 files, found 2 cross-package imports
package dependencies found in the code (from -> to : first place):
billing -> members library\billing\__init__.py:1 (library.members)
members -> billing library\members\__init__.py:1 (library.billing)
R1 direction:
R2 no cycles:
VIOLATION cycle: billing -> members -> billing
R3 public API only:
result: 1 violation(s)
Refactoring the Library: Diagram First, Then Code
Two requirements arrive. One: a DVD that comes back late should cost double a book. Two: the member should be sent a notice whenever a loan is recorded and when a late return adds a fine. This is the chapter-9 library:
The library's core before the change (checked against the code: all drawn operations exist).
The Review
Applying the checklist's "what will the next change touch?" question to requirement one: the fine is worked out in Loan.fine(today, kind), which knows only the member's kind. A copy's type (book or DVD) lives nowhere, and a loan holds its copy as a barcode, not as an object. To vary by item type I would have to edit Loan, Member.give_back and Copy and keep them consistent, and the next rule would edit all three again. That is a fine rule with no home of its own.
For requirement two: the obvious place to send a notice is inside LoanDesk.borrow. That would make the desk (high-level code) depend on e-mail or printing (low-level detail), would make every desk test send messages, and would let a broken mail server stop a loan being recorded. It would also be the first step towards the god class above.
The Redrawn Design
Before touching code, the new shape, as a diagram: the fine rule becomes a Strategy (FinePolicy, with the item type on Copy passed to it), and the notices become an Observer arrangement (Subscriber and ConsoleNotifier). Both interfaces are owned by the code that uses them.
After. LoanDesk depends on three interfaces; each new class points into one of them. (This diagram is also checked against the code: see the seven-diagram check above.)
The Code
Now the change. The unchanged parts of the package are the safety net: chapter 9's 10 tests, run before and after without being edited. The changes:
"""Things that happen at the desk, announced to whoever has subscribed (an Observer arrangement)."""
from dataclasses import dataclass
from datetime import date
@dataclass(frozen=True)
class LoanRecorded:
card: str
barcode: str
due_on: date
@dataclass(frozen=True)
class BookReturned:
card: str
barcode: str
fine_pence: int
"""The interfaces the application REQUIRES. Defined here, in the application, not beside the code that provides them."""
from typing import Protocol, runtime_checkable
from library.domain import Copy, Loan, Member
@runtime_checkable
class Catalogue(Protocol):
def member(self, card: str) -> Member: ...
def copy(self, barcode: str) -> Copy: ...
def open_loan_for(self, barcode: str) -> tuple[Member, Loan]: ...
def save(self) -> None: ...
@runtime_checkable
class Subscriber(Protocol):
"""Anything that wants to hear about desk events (an email sender, a log, a counter)."""
def notify(self, event: object) -> None: ...
"""The use cases of the lending desk. Depends on the domain and on the interfaces it requires; knows nothing about storage or about who is listening."""
from datetime import date
from library.application.events import BookReturned, LoanRecorded
from library.application.ports import Catalogue, Subscriber
from library.domain import FinePolicy, Loan
class LoanDesk:
def __init__(self, catalogue: Catalogue, subscribers: tuple[Subscriber, ...] = (), fine_policy: FinePolicy | None = None):
self.catalogue = catalogue
self.subscribers = list(subscribers)
self.fine_policy = fine_policy
self.errors: list[str] = [] # a subscriber that failed: recorded, never allowed to undo a loan
def subscribe(self, subscriber: Subscriber) -> None:
self.subscribers.append(subscriber)
def borrow(self, card: str, barcode: str, today: date) -> Loan:
loan = self.catalogue.member(card).borrow(self.catalogue.copy(barcode), today)
self.catalogue.save()
self._publish(LoanRecorded(card, barcode, loan.due_on))
return loan
def return_book(self, barcode: str, today: date) -> int:
member, loan = self.catalogue.open_loan_for(barcode)
fine = member.give_back(loan, self.catalogue.copy(barcode), today, self.fine_policy)
self.catalogue.save()
self._publish(BookReturned(member.card, barcode, fine))
return fine
def _publish(self, event: object) -> None:
for subscriber in self.subscribers:
try:
subscriber.notify(event)
except Exception as e: # noqa: BLE001 - one broken subscriber must not break the desk or silence the others
self.errors.append(f"{type(subscriber).__name__} failed on {type(event).__name__}: {type(e).__name__}")
"""A subscriber that turns desk events into messages (printed here; an email sender would be another subscriber)."""
from library.application import BookReturned, LoanRecorded
class ConsoleNotifier:
def __init__(self, write=print):
self.write = write
def notify(self, event: object) -> None:
if isinstance(event, LoanRecorded):
self.write(f"notice to {event.card}: {event.barcode} is due back on {event.due_on}")
elif isinstance(event, BookReturned) and event.fine_pence:
self.write(f"notice to {event.card}: a fine of {event.fine_pence}p was added for {event.barcode}")
Three decisions in the desk are worth reading twice. A subscriber's failure never undoes a loan: the loan is saved first, then published, and each notification is isolated, so a broken subscriber is recorded and the others still hear. The desk publishes after saving, so subscribers never hear about a loan that was not recorded. And the old data files still load: files without an item type load as books (item_type defaults to "book").
Three new files and eight edited, 100 lines added and 19 removed. The domain's model.py, where the fine rule used to live, changed by only seven lines added and four removed. Not one line of the member, loan or catalogue behaviour changed except the fine's calculation moving out. Whether that is "a small change" is a judgement; the point is that it is countable, and that the same question can be asked of the next change.
Does It Still Work, and Is the Architecture Intact?
"""Tests for the refactored library: the strategy, the subscribers, and the promise that old data still loads."""
import json
import os
import tempfile
import unittest
from datetime import date
from library.application import BookReturned, LoanDesk, LoanRecorded, Subscriber
from library.domain import Copy, FinePolicy, Member, PerDayFine
from library.infrastructure import ConsoleNotifier, InMemoryCatalogue, JsonCatalogue
TODAY = date(2026, 10, 10)
LATE = date(2026, 11, 3) # a loan taken on TODAY falls due on 2026-10-31: this is 3 days late
def catalogue():
return InMemoryCatalogue([Member("M1", date(2027, 1, 1), "standard")], [Copy("B1"), Copy("D1", item_type="dvd")])
class Recorder:
def __init__(self):
self.events = []
def notify(self, event):
self.events.append(event)
class Broken:
def notify(self, event):
raise RuntimeError("the mail server is down")
class Strategy(unittest.TestCase):
def test_a_dvd_costs_double_for_the_same_lateness(self):
desk = LoanDesk(catalogue())
desk.borrow("M1", "B1", TODAY)
desk.borrow("M1", "D1", TODAY)
self.assertEqual(desk.return_book("B1", LATE), 30)
# M1 now has a fine, but returning is still allowed: only borrowing is blocked
self.assertEqual(desk.return_book("D1", LATE), 60)
def test_the_default_policy_satisfies_the_interface(self):
self.assertIsInstance(PerDayFine(), FinePolicy)
def test_a_different_policy_can_be_plugged_in_without_changing_anything_else(self):
class Amnesty:
def fine(self, days_overdue, kind, item_type):
return 0
desk = LoanDesk(catalogue(), fine_policy=Amnesty())
desk.borrow("M1", "B1", TODAY)
self.assertEqual(desk.return_book("B1", LATE), 0)
class Observer(unittest.TestCase):
def test_subscribers_hear_events_in_order(self):
rec = Recorder()
desk = LoanDesk(catalogue(), subscribers=(rec,))
desk.borrow("M1", "B1", TODAY)
desk.return_book("B1", LATE)
self.assertEqual(rec.events, [LoanRecorded("M1", "B1", date(2026, 10, 31)), BookReturned("M1", "B1", 30)])
def test_a_broken_subscriber_neither_undoes_the_loan_nor_silences_the_others(self):
rec = Recorder()
desk = LoanDesk(catalogue(), subscribers=(Broken(), rec))
loan = desk.borrow("M1", "B1", TODAY)
self.assertEqual(loan.barcode, "B1") # the loan stands
self.assertEqual(len(rec.events), 1) # the next subscriber still heard it
self.assertEqual(desk.errors, ["Broken failed on LoanRecorded: RuntimeError"])
def test_the_desk_does_not_know_who_is_listening(self):
self.assertIsInstance(Recorder(), Subscriber)
self.assertIsInstance(ConsoleNotifier(), Subscriber)
def test_console_notifier_messages(self):
lines = []
desk = LoanDesk(catalogue(), subscribers=(ConsoleNotifier(lines.append),))
desk.borrow("M1", "B1", TODAY)
desk.return_book("B1", date(2026, 10, 31)) # on time: no fine message
self.assertEqual(lines, ["notice to M1: B1 is due back on 2026-10-31"])
class Compatibility(unittest.TestCase):
def test_a_data_file_from_before_item_types_still_loads_as_books(self):
old = {"members": [{"card": "M1", "expires_on": "2027-01-01", "kind": "standard", "fines_pence": 0, "loans": []}],
"copies": [{"barcode": "B1", "on_loan": False}]}
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, "old.json")
with open(path, "w", encoding="utf8") as f:
json.dump(old, f)
self.assertEqual(JsonCatalogue(path).copy("B1").item_type, "book")
def test_item_type_survives_a_save_and_load(self):
with tempfile.TemporaryDirectory() as d:
path = os.path.join(d, "new.json")
c = JsonCatalogue(path)
c.add(copy=Copy("D1", item_type="dvd"))
c.save()
self.assertEqual(JsonCatalogue(path).copy("D1").item_type, "dvd")
if __name__ == "__main__":
unittest.main()
Ran 23 tests in 0.223s
OK
All 23 tests pass: chapter 9's 10 (unmodified), the 9 new behaviour tests, the 2 architecture tests, and 2 for the extra patterns. The dependency rules from chapter 9 are unchanged and still clean (the new infrastructure -> application edge is the notifier importing the events, which the rules allow). And running the command-line program shows the observer at work:
$ py -m library.ui.cli --data library.json init
initialised: library.json
$ ... borrow M1 B1 --today 2026-10-10
notice to M1: B1 is due back on 2026-10-31
loan recorded: B1 due 2026-10-31
$ ... return B1 --today 2026-11-03
notice to M1: a fine of 30p was added for B1
returned; fine 30p
Usual Mistakes
Mistake
Fix
Reaching for a pattern before there is a problem
Wait for the second variation or the second listener; the diagram of the simple version is better than a pattern with one member
Naming a class after its pattern ("StrategyManager")
Name it for its job (FinePolicy); the diagram shows the shape
Treating a smell detector's silence as a clean bill of health
It gave two groups for five jobs; read the class
A pattern diagram that is wishful
Check it against the code, and tamper with the check once to see it fail
Refactoring with no safety net
Run the old tests before and after, unedited; add new ones for the new behaviour
Observers that can break the subject
Isolate each notification and record failures
Reviewing the code but not the change
Count what a plausible next change would touch
Mistakes while preparing this chapter
My first Strategy created an import cycle. The default policy lived in fines.py, which needed MemberKind from model.py, while model.py needed PerDayFine. I first hid the cycle with an import inside a method and a comment explaining why: a warning sign I had written down myself. The proper fix is for fines.py to import MemberKind only for type hints (under TYPE_CHECKING), so nothing is imported at run time and there is no cycle. Takeaway: a comment that explains why an import is in the wrong place is a design problem, not a solution.
A scripted edit put a comment inside an expression. Patching jsonfile.py with a text-replacement script inserted my trailing comment before a closing parenthesis, and the file stopped parsing (SyntaxError: '(' was never closed). Running the tests straight away found it in seconds. This is the same kind of slip as in chapters 7 and 10: use the editor's replace for code edits, and run the code immediately.
The god class detector under-counted. I wrote the flawed module with five jobs and expected the detector to find five. It found two. I kept the result in the chapter and added Exercise 1 on it. Takeaway: validate a detector on a case where you already know the answer, and report the gap.
An over-strict guard in my own tool, found by a new exercise. Chapter 9's dependency checker reported "nothing was analysed" when a fixed tree had no imports between packages at all, which is a legitimate state. Exercise 3 hit it. I changed the guard to fail only when no files are found, corrected chapter 9's script and its text, and re-ran its tests. Takeaway: a guard against "checking nothing" must not mistake "nothing to report" for "nothing checked".
A check that passed first time. The diagram check said every diagram was fine on its first run. That is the moment to distrust it, so I wrote the tampered-diagram demonstration. It is in the chapter because the first pass alone proves nothing.
Hands-On Exercises
Exercise 1
Run the smell detector on the flawed module, then list the jobs you see in LibraryManager yourself and compare with what the detector reports. Explain the difference, and say how you would split the class (name the new classes and the interfaces between them).
Add a third subscriber to the refactored desk: a counter of how many loans fall due on each day. Write it without editing the library package, run it alongside the console notifier, and prove the library's files did not change.
Build a two-package tree where billing and members import each other and show the dependency checker reporting the cycle. Remove the cycle by having members declare the interface it needs and billing provide it, show the checker clean, and show the fixed code running.
Patterns are shapes on a diagram: Strategy (client + interface + implementations), Observer (subject + subscriber interface), State (context + one class per state), Factory (one creator), Composite (a node that contains nodes); this chapter shows the diagrams, Design Patterns teaches the patterns
Check pattern diagrams against the code (operations, inheritance, realisation), and tamper with the check once to see it fail
Principles as shapes: interfaces owned by the client with arrows pointing at them; new behaviour as a new box that touches no old box; clusters for cohesion; few arrows out of a box
Review questions: one job per class? dependencies pointing the right way? cycles? where does behaviour live? what varies? what will the next change touch? what if it fails? do the diagrams match the code?
Smell detectors point, people decide: god class (methods in unrelated groups, a floor on the count), mutual references (legitimate or not), anemic (data-only classes beside a service that reaches in)
Refactor on the diagram first, change the code with the old tests as the safety net, then count what the change touched and check the architecture rules
An observer must not be able to break the subject: save first, publish after, isolate and record failures
Coming next
Object-Oriented Design with UML 12 (Capstone): design a new system, a workshop-booking service, from requirements to use cases, domain model, class design, key sequence and state diagrams and a component view, all as diagram-as-code files, implement and run the core in Python, and produce a short design document. Course complete, with pointers to Design Patterns, Software Architecture Fundamentals and Clean Code, SOLID & Refactoring.