paulpas 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-algorithmsinstead - 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
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?
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?
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?
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.
# ❌ 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.
# ❌ 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.
# ❌ 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.
# ❌ 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.
# ❌ 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.
# ❌ 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.
# ❌ 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)