# Design Pattern Anti Patterns > Identifies and remediates anti-patterns arising from misuse of GoF design patterns including over-engineering, gold plating, dependency inversion violations, and structural code smells in Python systems. - Skill: `paulpas/design-pattern-anti-patterns` (Agent Skill) - Install (CLI): `npx skillmds@latest add paulpas/design-pattern-anti-patterns` - Raw SKILL.md: https://api.skillmd.com/api/skills/paulpas/design-pattern-anti-patterns/raw - Safety review: pending - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: Coding & Dev Tools - License: MIT - Author: paulpas (https://skillmd.com/u/paulpas) - Updated: 2026-09-17 - Page: https://skillmd.com/skills/paulpas/design-pattern-anti-patterns --- # Design Pattern Anti-Patterns & Code Smells Senior engineer auditing code for design pattern misuse and SOLID principle violations. This skill makes the model recognize when GoF patterns are applied incorrectly — over-engineered, under-justified, or structurally harmful — and provide concrete remediation that collapses unnecessary abstractions while preserving legitimate extensibility points. ## TL;DR Checklist - [ ] Count GoF patterns per module; more than 3 without clear responsibility separation is a red flag - [ ] Verify every interface has at least one concrete current consumer (no phantom polymorphism) - [ ] Check that Factory classes have non-trivial creation logic (simple `__init__` does not justify a factory) - [ ] Audit Singleton usage — only appropriate for true shared resources with controlled lifecycle - [ ] Replace inheritance-based Template Method with composition-based Strategy when variation is behavioral - [ ] Ensure each class has exactly one reason to change (SRP) and no hidden global coupling --- ## When to Use Use this skill when: - A codebase feels over-factored — too many interfaces, factories, or wrapper layers for simple operations - New developers struggle to trace what happens because behavior is split across ten classes instead of one function - Code reviews repeatedly flag "this could be simpler" comments on specific modules - A team has adopted design patterns dogmatically without understanding when each pattern earns its keep - Technical debt audits reveal factories, observers, and decorators layered atop straightforward logic --- ## When NOT to Use Avoid this skill for: - Critiquing algorithmic efficiency or data structure choices — use `coding-algorithms` instead - Evaluating API surface design or HTTP endpoint structure — that is an API architecture concern - Analyzing runtime behavior issues like memory leaks or race conditions — those are debugging tasks - Codebases that genuinely require the patterns being critiqued (e.g., a plugin system with 50+ independently loaded modules) --- ## Core Workflow 1. **Scan for pattern overuse** — Count how many GoF patterns are employed in a single module or class hierarchy. If more than three distinct patterns coexist without clear separation of concern, flag for review. Check the number of interfaces defined per implementation class. **Checkpoint:** Does each declared interface have at least one concrete consumer? 2. **Check for YAGNI violations** — Identify abstractions that exist solely for hypothetical future needs: extra interfaces with zero implementations, unused factory classes, phantom strategies that no caller instantiates. Trace every import and constructor argument to verify the abstraction is actually exercised. **Checkpoint:** Is there a concrete current use case justifying this abstraction? 3. **Audit SOLID compliance** — Verify SRP (can you describe what this class does in one sentence without "and"?), LSP (can any subclass replace its parent without changing program behavior?), and DIP (do high-level modules depend on abstractions, not concretions?). Check OCP (can new behaviors be added without modifying existing code?). **Checkpoint:** Can you add a new payment processor or shipping method without touching the core order processing class? 4. **Remediate detected anti-patterns** — Apply the specific remediation strategy for each identified issue: collapse unnecessary hierarchies, inline over-factored methods, replace phantom abstractions with direct calls. Prioritize by severity — fix blocking issues first (hidden coupling, impossible-to-test classes), then nagging issues (unnecessary indirection). **Checkpoint:** Do all existing unit tests pass after the remediation? --- ## Anti-Pattern Catalog #### Gold Plating Applying design patterns where simple code would suffice. Creating unnecessary factory layers, strategy interfaces, and observer chains for problems that do not need them. This is the most common GoF-related anti-pattern: the pattern fits a toy example but adds real cost to production code. It inflates complexity without delivering matching benefits. ```python # ❌ BAD — Gold plating: three layers of indirection for what amounts to loading config from JSON class ConfigReader: """Interface that exists only because someone thought it might be useful.""" def read(self) -> dict: ... class JsonConfigReader(ConfigReader): def __init__(self, file_path: str) -> None: self._path = file_path def read(self) -> dict: with open(self._path, "r") as f: return json.load(f) class ConfigReaderFactory: """Factory that creates exactly one type of reader. Zero flexibility gained.""" @staticmethod def create(file_path: str) -> ConfigReader: # Always returns JsonConfigReader — the abstraction adds nothing return JsonConfigReader(file_path) class AppConfig(ConfigReader): """Wraps the factory + interface just to follow "patterns" from a tutorial.""" def __init__(self, path: str) -> None: self._reader = ConfigReaderFactory.create(path) @property def data(self) -> dict: return self._reader.read() def load_app_config(path: str) -> dict: """Caller needs to know about AppConfig class instead of just reading a file.""" app_config = AppConfig(path) return app_config.data # ✅ GOOD — Direct, simple, and does exactly what it says import json from pathlib import Path def load_app_config(file_path: str) -> dict: """Load application configuration from a JSON file. Args: file_path: Absolute or relative path to the configuration file. Returns: Parsed configuration as a dictionary. Raises: FileNotFoundError: If the configuration file does not exist. json.JSONDecodeError: If the file contains invalid JSON. """ with open(file_path, "r", encoding="utf-8") as f: return json.load(f) ``` **Why this is worse:** Gold plating creates 4 classes and 2 factory methods for what should be a single function call. Every future developer must navigate through `AppConfig → ConfigReaderFactory → JsonConfigReader → ConfigReader` to understand how config loads. The "extension point" (adding XML config) was never needed and likely never will be — it was designed for a hypothetical future that may never arrive. **When this occurs:** Developers who learned design patterns from tutorials or books often apply them reflexively, assuming every problem is the toy example from the pattern documentation. Code reviews that reward "good architectural practices" without scrutinizing actual need amplify this problem. --- #### Over-Engineering / Over-Abstraction Designing abstractions for hypothetical future requirements rather than current needs. Too many interface levels, phantom polymorphism, and excessive indirection through patterns when direct calls would work. The code becomes harder to read, harder to debug, and harder to modify correctly because the real logic is buried under layers of indirection. ```python # ❌ BAD — Over-abstraction: five layers deep for simple email sending class EmailMessage(Protocol): """Abstract message contract that adds no value over a dataclass.""" @property def recipient(self) -> str: ... @property def subject(self) -> str: ... @property def body(self) -> str: ... class MessageBuilder(Protocol): """Abstract builder for messages that have no complex construction logic.""" def build(self) -> EmailMessage: ... class SimpleMessageBuilder(MessageBuilder): """The only implementation — builder pattern overkill for immutable data.""" def __init__(self, recipient: str, subject: str, body: str) -> None: self._recipient = recipient self._subject = subject self._body = body def build(self) -> EmailMessage: return EmailData(self._recipient, self._subject, self._body) class EmailData(EmailMessage): def __init__(self, recipient: str, subject: str, body: str) -> None: self._recipient = recipient self._subject = subject self._body = body @property def recipient(self) -> str: return self._recipient @property def subject(self) -> str: return self._subject @property def body(self) -> str: return self._body class EmailTransport(Protocol): """Abstract transport layer for sending messages.""" def send(self, message: EmailMessage) -> None: ... class SmtpEmailTransport(EmailTransport): """Only email transport ever used — the abstraction prevents testing by hiding SMTP details.""" def __init__(self, smtp_host: str = "localhost", smtp_port: int = 587) -> None: self._smtp_host = smtp_host self._smtp_port = smtp_port def send(self, message: EmailMessage) -> None: # Complex SMTP logic buried here — but we can't test without a real server import smtplib server = smtplib.SMTP(self._smtp_host, self._smtp_port) server.starttls() server.send_message(message.body) # type: ignore[attr-defined] server.quit() class EmailNotificationService: """Coordinates builder + transport through the protocol layers.""" def __init__(self, transport: EmailTransport) -> None: self._transport = transport def send_welcome_email(self, recipient: str, name: str) -> None: builder = SimpleMessageBuilder(recipient, f"Welcome, {name}!", f"Hi {name}, welcome!") message = builder.build() self._transport.send(message) # ✅ GOOD — Straightforward function that is easy to read, test, and modify import smtplib from dataclasses import dataclass @dataclass(frozen=True) class EmailMessage: recipient: str subject: str body: str def send_welcome_email(recipient: str, name: str, *, smtp_host: str = "localhost") -> None: """Send a welcome email to a new user. Args: recipient: The recipient's email address. name: The recipient's display name. smtp_host: SMTP server hostname (default: localhost). Raises: smtplib.SMTPException: If the email cannot be delivered. """ subject = f"Welcome, {name}!" body = f"Hi {name}, welcome!" message = EmailMessage(recipient, subject, body) with smtplib.SMTP(smtp_host, 587) as server: server.starttls() # In production, authenticate here — but the test can mock smtplib.SMTP directly server.sendmail("noreply@example.com", [recipient], f"Subject: {subject}\n\n{body}") ``` **Why this is worse:** The over-abstracted version has 7 classes and 2 protocols for a single email-sending operation. Testing requires either mocking `EmailTransport` or spinning up a test SMTP server. Adding a new message format means creating new concrete builders, transports, and factories — not because the current code lacks flexibility, but because the abstraction was designed for flexibility that doesn't exist yet. **When this occurs:** Teams working on greenfield projects often design "extensible" systems before they know what needs extension. The result is a framework that solves problems which don't exist while creating real problems with testing and debugging that do. --- #### Dependency Inversion Violation High-level modules depending on low-level concrete implementations directly, or the opposite: forcing dependency injection everywhere even for simple utility classes where it adds boilerplate without benefit. DIP should reduce coupling, but inverted incorrectly it either increases coupling or creates DI container bloat. ```python # ❌ BAD — High-level module depends on low-level concrete class directly import sqlite3 class DatabaseConnection(sqlite3.Connection): """Concrete database connection — hard dependency on SQLite implementation.""" def __init__(self, db_path: str) -> None: super().__init__(db_path) class UserRepository: """User repository is tightly coupled to SQLite. Can't switch to Postgres without rewriting this class.""" def __init__(self, db_path: str) -> None: self._connection = sqlite3.connect(db_path) # Direct concrete dependency! self._create_tables() def _create_tables(self) -> None: self._connection.execute(""" CREATE TABLE IF NOT EXISTS users ( id INTEGER PRIMARY KEY, name TEXT NOT NULL, email TEXT UNIQUE NOT NULL ) """) self._connection.commit() def create_user(self, name: str, email: str) -> int: cursor = self._connection.execute( "INSERT INTO users (name, email) VALUES (?, ?)", (name, email) ) self._connection.commit() return cursor.lastrowid # type: ignore[return-value] def find_by_email(self, email: str) -> dict | None: cursor = self._connection.execute( "SELECT id, name, email FROM users WHERE email = ?", (email,) ) row = cursor.fetchone() if row is None: return None return {"id": row[0], "name": row[1], "email": row[2]} # ✅ GOOD — High-level module depends on abstraction; low-level modules implement it from abc import ABC, abstractmethod import sqlite3 import psycopg2 class UserDatabase(ABC): """Abstraction that the high-level business logic depends on.""" @abstractmethod def create_user(self, name: str, email: str) -> int: ... @abstractmethod def find_by_email(self, email: str) -> dict | None: ... @abstractmethod def close(self) -> None: ... class SqliteUserDatabase(UserDatabase): """Concrete SQLite implementation — low-level details encapsulated.""" def __init__(self, db_path: str) -> None: self._connection = sqlite3.connect(db_path) self._initialize() def _initialize(self) -> None: self._connection.execute(""" CREATE TABLE IF NOT EXISTS users ( id INTEGER PRIMARY KEY, name TEXT NOT NULL, email TEXT UNIQUE NOT NULL ) """) self._connection.commit() def create_user(self, name: str, email: str) -> int: cursor = self._connection.execute( "INSERT INTO users (name, email) VALUES (?, ?)", (name, email) ) self._connection.commit() return cursor.lastrowid # type: ignore[return-value] def find_by_email(self, email: str) -> dict | None: cursor = self._connection.execute( "SELECT id, name, email FROM users WHERE email = ?", (email,) ) row = cursor.fetchone() if row is None: return None return {"id": row[0], "name": row[1], "email": row[2]} def close(self) -> None: self._connection.close() class PostgresUserDatabase(UserDatabase): """Concrete PostgreSQL implementation — same interface, different storage.""" def __init__(self, connection_string: str) -> None: self._connection = psycopg2.connect(connection_string) self._initialize() def _initialize(self) -> None: self._connection.execute(""" CREATE TABLE IF NOT EXISTS users ( id SERIAL PRIMARY KEY, name TEXT NOT NULL, email TEXT UNIQUE NOT NULL ) """) self._connection.commit() def create_user(self, name: str, email: str) -> int: cursor = self._connection.execute( "INSERT INTO users (name, email) VALUES (%s, %s) RETURNING id", (name, email) ) self._connection.commit() return cursor.fetchone()[0] # type: ignore[misc] def find_by_email(self, email: str) -> dict | None: cursor = self._connection.execute( "SELECT id, name, email FROM users WHERE email = %s", (email,) ) row = cursor.fetchone() if row is None: return None return {"id": row[0], "name": row[1], "email": row[2]} def close(self) -> None: self._connection.close() class RegistrationService: """High-level business logic depends only on the abstraction.""" def __init__(self, user_db: UserDatabase) -> None: self._user_db = user_db # Injection at composition root, not everywhere def register(self, name: str, email: str) -> int: """Register a new user. Returns the user ID.""" existing = self._user_db.find_by_email(email) if existing is not None: raise ValueError(f"User with email {email} already exists") return self._user_db.create_user(name, email) ``` **Why this is worse:** In the BAD version, `UserRepository` embeds SQLite knowledge directly. You cannot unit-test it without a real database file, you cannot swap to PostgreSQL, and every change to the database layer requires modifying business logic. In the DI-overkill variant (not shown), every utility class becomes injectable through a DI container, creating circular dependency issues and making the codebase harder to understand because you must trace through the container configuration to know what dependencies exist. **When this occurs:** Developers who apply DIP from frameworks like Spring (Java) where DI containers are pervasive often over-apply it in Python, where `__init__` parameters and composition are usually sufficient. The opposite error — ignoring DIP entirely in larger systems — creates tangled dependency graphs that resist testing. --- #### Singleton Abuse Using Singleton as a global variable substitute, creating hidden coupling and making testing impossible. Also using it for objects that should be stateless or managed by a DI container. Singletons hide their dependencies, make parallel execution impossible, and turn unit tests into integration tests. ```python # ❌ BAD — Singleton used as a hidden global state holder import threading class MetricsCollector: """Singleton disguised as a class with a module-level instance. Hidden dependency: any code that calls log_metric() pulls in the entire collector.""" _instance: "MetricsCollector | None" = None _lock = threading.Lock() def __new__(cls) -> "MetricsCollector": if cls._instance is None: with cls._lock: if cls._instance is None: cls._instance = super().__new__(cls) cls._instance._metrics: dict[str, list[float]] = {} return cls._instance def _ensure_initialized(self) -> None: """Double-check that we are the actual instance.""" if self._instance is not self: raise RuntimeError("Use MetricsCollector(), do not subclass") def log_metric(self, name: str, value: float) -> None: self._ensure_initialized() if name not in self._metrics: self._metrics[name] = [] self._metrics[name].append(value) def get_average(self, name: str) -> float: self._ensure_initialized() values = self._metrics.get(name, []) if not values: return 0.0 return sum(values) / len(values) # Module-level convenience functions that hide the coupling entirely def log_metric(name: str, value: float) -> None: MetricsCollector().log_metric(name, value) def get_average(name: str) -> float: return MetricsCollector().get_average(name) # ❌ Also BAD — Singleton for something that should be stateless class DateUtils: """Stateless utility class turned into a singleton. No state to share.""" _instance: "DateUtils | None" = None def __new__(cls) -> "DateUtils": if cls._instance is None: cls._instance = super().__new__(cls) return cls._instance def format_date(self, timestamp: float) -> str: import datetime return datetime.datetime.fromtimestamp(timestamp).isoformat() def days_between(self, t1: float, t2: float) -> int: import datetime delta = abs(datetime.datetime.fromtimestamp(t2) - datetime.datetime.fromtimestamp(t1)) return delta.days # ✅ GOOD — Explicit singleton only for genuinely shared resources with lifecycle management from contextlib import contextmanager import threading from typing import ClassVar class MetricsCollector: """Singleton pattern applied correctly: only for a resource that must be shared across the application lifetime, with explicit creation and cleanup.""" _instance: ClassVar["MetricsCollector | None"] = None _lock: ClassVar[threading.Lock] = threading.Lock() def __new__(cls) -> "MetricsCollector": if cls._instance is None: with cls._lock: if cls._instance is None: instance = super().__new__(cls) instance._metrics: dict[str, list[float]] = {} instance._initialized = False cls._instance = instance return cls._instance def initialize(self) -> None: """Explicit initialization — visible in the code path.""" if self._initialized: return self._metrics.clear() self._initialized = True def log_metric(self, name: str, value: float) -> None: if not self._initialized: raise RuntimeError("MetricsCollector not initialized. Call initialize() first.") self._metrics.setdefault(name, []).append(value) @property def metrics(self) -> dict[str, list[float]]: return dict(self._metrics) def get_average(self, name: str) -> float | None: values = self._metrics.get(name) if not values: return None return sum(values) / len(values) def get_metrics_collector() -> MetricsCollector: """Module-level accessor — but callers are expected to call initialize().""" instance = MetricsCollector() if not instance._initialized: # type: ignore[attr-defined] raise RuntimeError("Call MetricsCollector.initialize() before using metrics") return instance # ✅ GOOD — Stateless utilities as plain functions (no class, no singleton) from datetime import datetime def format_date(timestamp: float) -> str: """Format an epoch timestamp as ISO 8601 string.""" return datetime.fromtimestamp(timestamp).isoformat() def days_between(t1: float, t2: float) -> int: """Calculate the absolute number of days between two epoch timestamps.""" delta = abs(datetime.fromtimestamp(t2) - datetime.fromtimestamp(t1)) return delta.days ``` **Why this is worse:** The BAD `MetricsCollector` hides its state in a module-level singleton that any code can access through `log_metric()`. Tests run in parallel will corrupt each other's metrics. You cannot inject a fake collector for testing without monkeypatching the module. The stateless `DateUtils` singleton wastes thread synchronization on a class that has no state to protect. **When this occurs:** Developers migrating from languages/frameworks where singletons are the default sharing mechanism (Java Spring, Laravel) often replicate the pattern in Python. They also create singletons for utility classes by habit rather than necessity. --- #### Factory Explosion Creating factory methods, abstract factories, and builder patterns where simple `__init__` or class methods would suffice. Each factory adds indirection without real value when the creation logic is straightforward. The result is a forest of small classes, each responsible for constructing one other class. ```python # ❌ BAD — Factory explosion for creating database models from abc import ABC, abstractmethod from dataclasses import dataclass @dataclass class UserProfile: user_id: int username: str email: str display_name: str class ProfileFactory(ABC): """Abstract factory interface — why does creating a dataclass need an interface?""" @abstractmethod def create_profile(self, user_id: int, username: str, email: str, display_name: str) -> UserProfile: ... class DefaultProfileFactory(ProfileFactory): """The only implementation. Abstract factory + concrete factory = two classes for one dataclass.""" def create_profile( self, user_id: int, username: str, email: str, display_name: str ) -> UserProfile: return UserProfile(user_id, username, email, display_name) @dataclass class OrderItem: item_id: str product_name: str quantity: int unit_price: float @property def total(self) -> float: return self.quantity * self.unit_price class OrderItemFactory(ABC): @abstractmethod def create_order_item(self, item_id: str, product_name: str, quantity: int, unit_price: float) -> OrderItem: ... class DefaultOrderItemFactory(OrderItemFactory): """Another factory for another dataclass. The pattern is the product now, not the domain.""" def create_order_item( self, item_id: str, product_name: str, quantity: int, unit_price: float ) -> OrderItem: if quantity < 1: raise ValueError("Quantity must be at least 1") return OrderItem(item_id, product_name, quantity, unit_price) class OrderFactory(ABC): @abstractmethod def create_order_item(self, item_id: str, product_name: str, quantity: int, unit_price: float) -> OrderItem: ... @abstractmethod def create_profile( self, user_id: int, username: str, email: str, display_name: str ) -> UserProfile: ... class DefaultOrderFactory(OrderFactory): """Abstract factory that combines two unrelated factories. Violates SRP itself.""" def __init__(self, profile_factory: ProfileFactory | None = None) -> None: self._profile_factory = profile_factory or DefaultProfileFactory() self._item_factory = DefaultOrderItemFactory() # No abstraction needed here def create_profile( self, user_id: int, username: str, email: str, display_name: str ) -> UserProfile: return self._profile_factory.create_profile(user_id, username, email, display_name) def create_order_item(self, item_id: str, product_name: str, quantity: int, unit_price: float) -> OrderItem: return self._item_factory.create_order_item(item_id, product_name, quantity, unit_price) # ✅ GOOD — Factory methods on the domain objects themselves, or just use __init__ from dataclasses import dataclass @dataclass(frozen=True) class UserProfile: user_id: int username: str email: str display_name: str @dataclass(frozen=True) class OrderItem: item_id: str product_name: str quantity: int unit_price: float def __post_init__(self) -> None: if self.quantity < 1: raise ValueError("Quantity must be at least 1") @property def total(self) -> float: return self.quantity * self.unit_price class OrderItemFactory: """Class-method factory only used when creation logic is genuinely complex. Here it simply delegates to the dataclass constructor — but the class exists so we can extend it later without changing call sites.""" @classmethod def create(cls, item_id: str, product_name: str, quantity: int, unit_price: float) -> OrderItem: """Create an order item with validation. Args: item_id: Unique identifier for the line item. product_name: Human-readable product name. quantity: Number of units (must be >= 1). unit_price: Price per unit in the currency's base denomination. Returns: A validated OrderItem instance. Raises: ValueError: If quantity is less than 1 or unit_price is negative. """ if unit_price < 0: raise ValueError(f"Unit price must be non-negative, got {unit_price}") return OrderItem(item_id, product_name, quantity, unit_price) ``` **Why this is worse:** The factory explosion version has 6 classes (2 dataclasses + 4 factories including abstract base classes) for creating what amounts to two dataclass instances. Every call site must import and instantiate a factory instead of just calling `UserProfile(...)`. When you add a third domain class, you need three more factory classes. The pattern becomes self-referential — factories exist to create factories. **When this occurs:** Teams that learn abstract factory and factory method patterns often apply them uniformly across the codebase, even for objects with trivial construction logic. Abstract Factory is most commonly abused because it looks impressive in architecture diagrams but rarely provides real value outside of creating families of related products (e.g., UI widgets for different platforms). --- #### Observer Spam Using Observer pattern everywhere instead of direct method calls for synchronous operations. Turning what should be a single function call into an event-driven callback chain that is hard to debug and trace. Event systems add latency, hide control flow, and make it nearly impossible to follow the execution path in a debugger. ```python # ❌ BAD — Observer pattern applied to synchronous operations that have nothing to gain from decoupling from abc import ABC, abstractmethod class UserEvent(ABC): """Abstract event class — adds indirection over simple data classes.""" @property def timestamp(self) -> float: ... class UserCreatedEvent(UserEvent): def __init__(self, user_id: int, username: str) -> None: import time self._timestamp = time.time() self._user_id = user_id self._username = username @property def timestamp(self) -> float: return self._timestamp @property def user_id(self) -> int: return self._user_id @property def username(self) -> str: return self._username class UserEventObserver(ABC): """Abstract observer interface for handling user events.""" @abstractmethod def on_user_created(self, event: UserCreatedEvent) -> None: ... class SendWelcomeEmailObserver(UserEventObserver): def on_user_created(self, event: UserCreatedEvent) -> None: # This could just be a method call — the observer pattern adds no decoupling benefit here self._send_welcome_email(event.user_id, event.username) def _send_welcome_email(self, user_id: int, username: str) -> None: pass # Would send email class UpdateSearchIndexObserver(UserEventObserver): def on_user_created(self, event: UserCreatedEvent) -> None: # Updating search index is a side effect that should be explicit, not hidden in an observer self._index_user(event.user_id, event.username) def _index_user(self, user_id: int, username: str) -> None: pass # Would add to search index class AnalyticsObserver(UserEventObserver): def on_user_created(self, event: UserCreatedEvent) -> None: # Tracking analytics as an observer makes it invisible in the registration flow self._track_signup(event.username) def _track_signup(self, username: str) -> None: pass # Would send analytics event class EventPublisher: """Central event hub that every user operation must know about. Every caller needs access to the publisher — tight coupling through a different path.""" def __init__(self) -> None: self._observers: list[UserEventObserver] = [] def register(self, observer: UserEventObserver) -> None: self._observers.append(observer) def notify_user_created(self, user_id: int, username: str) -> None: event = UserCreatedEvent(user_id, username) for observer in self._observers: observer.on_user_created(event) # Order of execution matters but is not documented class UserService: """Service class that must know about the event system to notify observers.""" def __init__(self, publisher: EventPublisher) -> None: self._publisher = publisher def register_user(self, username: str, email: str) -> int: user_id = self._store_user(username, email) self._publisher.notify_user_created(user_id, username) # Hidden side effects! return user_id def _store_user(self, username: str, email: str) -> int: return 42 # ✅ GOOD — Explicit method calls for operations that are part of the same transaction class UserService: """Service with explicit, visible side effects. No hidden observers or event systems.""" def __init__(self, user_store: "UserStore", email_sender: "EmailSender") -> None: self._user_store = user_store self._email_sender = email_sender def register_user(self, username: str, email: str) -> int: """Register a new user with all side effects made explicit. Args: username: Desired username. email: Contact email address. Returns: The assigned user ID. Side effects (all visible here): - Persists user to database - Sends welcome email - Updates search index - Tracks analytics event """ # Primary operation: persist the user user_id = self._user_store.create(username, email) # Explicit side effects — all in one place, visible at a glance try: self._email_sender.send_welcome_email(user_id, username) except EmailDeliveryError as exc: # Non-critical side effect failure — log but don't fail registration self._log_warning("Welcome email failed", user_id=user_id, error=exc) self._search_indexer.index_user(user_id, username) self._analytics.track_signup(username) return user_id class UserServiceWithAsyncSideEffects: """For genuinely async side effects that should not block registration, use explicit async dispatch — not a synchronous observer pattern.""" def __init__( self, user_store: "UserStore", event_dispatcher: "EventDispatcher", ) -> None: self._user_store = user_store self._event_dispatcher = event_dispatcher # Async, not hidden — explicitly passed async def register_user(self, username: str, email: str) -> int: user_id = await self._user_store.create(username, email) # Explicit async dispatch of side effects events = [ UserCreatedEvent(user_id, username), ] await self._event_dispatcher.publish_all(events) return user_id ``` **Why this is worse:** The observer version hides all side effects behind an event system. A developer reading `register_user()` cannot tell that email sending, search indexing, and analytics tracking happen — they have to hunt through the codebase for observers registered with the publisher. Debugging a bug requires tracing through asynchronous callback chains where the order of execution depends on registration order. **When this occurs:** Teams that adopt event-driven architecture too early, before the decoupling benefits outweigh the debugging costs. The Observer pattern is valuable when publish/subscribe semantics are genuinely needed (e.g., third-party integrations firing at unknown times), but it becomes a liability for synchronous operations within a single bounded context. --- #### Strategy Proliferation Creating separate strategy classes for variations that could be handled with simple conditionals or a single parameterized method. The Strategy pattern is useful when algorithms are genuinely complex and independently testable, not every branching choice deserves its own class. ```python # ❌ BAD — One strategy interface per minor variation from abc import ABC, abstractmethod from dataclasses import dataclass @dataclass class ShippingCalculation: cost: float estimated_days: int carrier: str class ShippingStrategy(ABC): """Abstract strategy for computing shipping costs.""" @abstractmethod def calculate(self, weight_kg: float, destination: str) -> ShippingCalculation: ... class GroundShippingStrategy(ShippingStrategy): def calculate(self, weight_kg: float, destination: str) -> ShippingCalculation: base_rate = 5.0 per_kg = 0.75 if destination == "international": return ShippingCalculation( cost=base_rate + (weight_kg * per_kg * 2.5), estimated_days=14, carrier="USPS Ground", ) return ShippingCalculation( cost=base_rate + (weight_kg * per_kg), estimated_days=5, carrier="FedEx Ground", ) class ExpressShippingStrategy(ShippingStrategy): def calculate(self, weight_kg: float, destination: str) -> ShippingCalculation: base_rate = 15.0 per_kg = 1.50 if destination == "international": return ShippingCalculation( cost=base_rate + (weight_kg * per_kg * 2.0), estimated_days=7, carrier="FedEx International", ) return ShippingCalculation( cost=base_rate + (weight_kg * per_kg), estimated_days=2, carrier="FedEx Express", ) class OvernightShippingStrategy(ShippingStrategy): def calculate(self, weight_kg: float, destination: str) -> ShippingCalculation: if destination != "domestic": raise ValueError("Overnight shipping not available internationally") return ShippingCalculation( cost=30.0 + (weight_kg * 2.0), estimated_days=1, carrier="UPS Overnight", ) class ShippingCalculator: """Must know about all strategy types and their instantiation.""" def __init__(self) -> None: self._strategies = { "ground": GroundShippingStrategy(), "express": ExpressShippingStrategy(), "overnight": OvernightShippingStrategy(), } def calculate( self, method: str, weight_kg: float, destination: str ) -> ShippingCalculation: strategy = self._strategies[method] return strategy.calculate(weight_kg, destination) # ✅ GOOD — Single function with parameterized logic when algorithms are related and simple def calculate_shipping( method: str, weight_kg: float, destination: str, ) -> ShippingCalculation: """Calculate shipping cost based on method, weight, and destination. Args: method: One of 'ground', 'express', or 'overnight'. weight_kg: Package weight in kilograms. destination: Either 'domestic' or 'international'. Returns: Shipping calculation with cost, estimated delivery days, and carrier. Raises: ValueError: If method is invalid or overnight requested for international shipping. """ if method not in ("ground", "express", "overnight"): raise ValueError(f"Invalid shipping method: {method}") if method == "overnight" and destination != "domestic": raise ValueError("Overnight shipping is not available for international orders") rates = { "ground": {"base": 5.0, "per_kg": 0.75, "intl_multiplier": 2.5, "days": 5, "carrier": "FedEx Ground"}, "express": {"base": 15.0, "per_kg": 1.50, "intl_multiplier": 2.0, "days": 2, "carrier": "FedEx Express"}, "overnight": {"base": 30.0, "per_kg": 2.0, "days": 1, "carrier": "UPS Overnight"}, } rate_config = rates[method] multiplier = rate_config.get("intl_multiplier", 1.0) if destination == "international" else 1.0 cost = rate_config["base"] + (weight_kg * rate_config["per_kg"] * multiplier) return ShippingCalculation( cost=round(cost, 2), estimated_days=rate_config["days"], carrier=rate_config["carrier"], ) # When Strategy pattern IS justified — genuinely complex, independently tested algorithms: from typing import Protocol class PricingAlgorithm(Protocol): """Strategy is appropriate when each algorithm h …(truncated)