Imported from paulpas/agent-skill-router (
skills/coding/design-pattern-anti-patterns/SKILL.md). Install upstream withnpx skills add paulpas/agent-skill-router --skill design-pattern-anti-patterns. Copyright stays with the author (MIT).
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.daysWhy 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_idWhy 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 has complex internal logic,
independent test suites, and may be swapped at runtime based on user configuration."""
def compute(self, cart_total: float, customer: "Customer") -> float: ...
class FlatRatePricing:
def compute(self, cart_total: float, customer: "Customer") -> float:
return cart_total * 1.08 # Standard 8% tax
class VolumeDiscountPricing:
"""Complex calculation with tiered thresholds — strategy pattern justified here."""
TIER_THRESHOLDS = [(1000.0, 0.92), (500.0, 0.95), (100.0, 0.98)]
def compute(self, cart_total: float, customer: "Customer") -> float:
discount_rate = 1.0
for threshold, rate in self.TIER_THRESHOLDS:
if cart_total >= threshold:
discount_rate = rate
break
return cart_total * discount_rate
class DynamicPricingService:
"""Runtime strategy selection is justified when the algorithm depends on user configuration."""
def __init__(self, pricing_algorithm: PricingAlgorithm) -> None:
self._algorithm = pricing_algorithm
def get_total(self, cart_total: float, customer: "Customer") -> float:
return self._algorithm.compute(cart_total, customer)Why this is worse: The BAD version has four classes for shipping calculations that are essentially a lookup table plus some arithmetic. Adding a new shipping method requires creating a new class and registering it with the ShippingCalculator. Every strategy class contains conditional logic about destination type — defeating the purpose of separating algorithms.
When Strategy pattern IS justified: When each variant has genuinely complex internal logic (e.g., the volume discount calculation with tiered thresholds), when different strategies need independent test suites, or when users can select which algorithm to use at runtime. The key signal is: does each strategy have a rich enough body to warrant its own file?
Decorator Chaining Fatigue
Stacking so many decorator layers that the final object's behavior is impossible to understand by reading any single class. Each decorator adds one line of wrapping but understanding requires following the entire chain. Debugging becomes an exercise in tracing through multiple classes that each do one small thing.
# ❌ BAD — Deeply nested decorators that make behavior opaque
from abc import ABC, abstractmethod
class Image(ABC):
@abstractmethod
def render(self, width: int, height: int) -> str: ...
@abstractmethod
def format_name(self) -> str: ...
class RealImage(Image):
def __init__(self, file_path: str) -> None:
self._file_path = file_path
def render(self, width: int, height: int) -> str:
return f"[Image loaded from {self._file_path}: {width}x{height}]"
def format_name(self) -> str:
return self._file_path.split("/")[-1]
class CachedImage(Image):
"""Caching decorator — caches rendered images by resolution."""
def __init__(self, image: Image) -> None:
self._image = image
self._cache: dict[tuple[int, int], str] = {}
def render(self, width: int, height: int) -> str:
key = (width, height)
if key not in self._cache:
self._cache[key] = self._image.render(width, height)
return self._cache[key]
def format_name(self) -> str:
return self._image.format_name()
class ResizedImage(Image):
"""Resizing decorator — scales the image to specified dimensions."""
def __init__(self, image: Image, target_width: int, target_height: int) -> None:
self._image = image
self._width = target_width
self._height = target_height
def render(self, width: int, height: int) -> str:
return f"[{self._width}x{self._height}] " + self._image.render(self._width, self._height)
def format_name(self) -> str:
return f"{self._image.format_name()}@{self._width}x{self._height}"
class WatermarkedImage(Image):
"""Watermark decorator — adds overlay text to the rendered image."""
def __init__(self, image: Image, watermark_text: str = "CONFIDENTIAL") -> None:
self._image = image
self._watermark = watermark_text
def render(self, width: int, height: int) -> str:
base = self._image.render(width, height)
return f"{base} + watermark('{self._watermark}')"
def format_name(self) -> str:
return f"{self._image.format_name()}+watermark"
class FormattedImage(Image):
"""Format conversion decorator — converts to specified output format."""
def __init__(self, image: Image, output_format: str) -> None:
self._image = image
self._format = output_format
def render(self, width: int, height: int) -> str:
return f"[{self._format.upper()}] " + self._image.render(width, height)
def format_name(self) -> str:
return f"{self._image.format_name()}.{self._format}"
# Usage: five layers of wrapping — no single place shows the full transformation chain
original = RealImage("photo.jpg")
cached = CachedImage(original)
resized = ResizedImage(cached, 800, 600)
watermarked = WatermarkedImage(resized)
final = FormattedImage(watermarked, "webp")
# How do I know what the final render() call actually does without reading all 5 classes?
print(final.render(800, 600))
# ✅ GOOD — Explicit composition with a single pipeline function
def render_image(
file_path: str,
target_width: int = 800,
target_height: int = 600,
cache_key: str | None = None,
watermark: str | None = None,
output_format: str = "jpeg",
) -> str:
"""Render an image through a configurable pipeline.
All transformations are explicit in the function signature and body.
No hidden layering or chain-of-objects to trace through.
Args:
file_path: Path to the source image file.
target_width: Desired output width in pixels.
target_height: Desired output height in pixels.
cache_key: If provided, use cached result for identical parameters.
watermark: Optional text overlay. None means no watermark.
output_format: Output format (jpeg, png, webp).
Returns:
Rendered image representation as a string.
"""
# Step 1: Load image
image_data = _load_image(file_path)
# Step 2: Resize
resized_data = _resize(image_data, target_width, target_height)
# Step 3: Cache (if key provided)
if cache_key and _has_cache(cache_key):
return _get_cached(cache_key)
# Step 4: Apply watermark
final_data = resized_data
if watermark:
final_data = _apply_watermark(final_data, watermark)
# Step 5: Convert format
rendered = _convert_format(final_data, output_format)
# Step 6: Cache result
if cache_key:
_set_cache(cache_key, rendered)
return rendered
# ✅ GOOD — Decorator pattern when it actually adds value (logging/monitoring)
import functools
from typing import Callable, TypeVar
T = TypeVar("T")
def timed_operation(func: Callable[..., T]) -> Callable[..., T]:
"""Decorator that logs execution time — this is where decorators add real value."""
@functools.wraps(func)
def wrapper(*args: object, **kwargs: object) -> T:
import time
start = time.monotonic()
try:
result = func(*args, **kwargs)
return result
finally:
elapsed = time.monotonic() - start
logger = __import__("logging").getLogger(__name__)
logger.info(f"{func.__name__} completed in {elapsed:.3f}s")
return wrapper
def cached_operation(cache: dict) -> Callable[[Callable[..., T]], Callable[..., T]]:
"""Decorator for caching function results by argument tuple."""
def decorator(func: Callable[..., T]) -> Callable[..., T]:
@functools.wraps(func)
def wrapper(*args: object, **kwargs: object) -> T:
key = (args, tuple(sorted(kwargs.items())))
if key in cache:
return cache[key]
result = func(*args, **kwargs)
cache[key] = result
return result
return wrapper
return decoratorWhy this is worse: The chained-decorator version makes it impossible to understand what final.render(800, 600) does without reading all five wrapper classes. Each class adds one transformation but also adds one more layer of delegation. When the rendered output is wrong, you must trace through every decorator in the chain to find which one produced the unexpected behavior.
When decorators are appropriate: For cross-cutting concerns like logging, caching, authentication checks, or metrics collection — where the core domain logic stays clean and the decorator adds orthogonal behavior that applies uniformly across many functions.
Command Object Overkill
Wrapping simple function calls in Command objects for queuing or undo when there is no actual need for those capabilities. Every synchronous call becomes a method invocation plus an object allocation. The Command pattern is useful for undo/redo systems, job queues, or request logging — but not for every operation.
# ❌ BAD — Command objects for simple operations with no queue or undo support
from abc import ABC, abstractmethod
class Command(ABC):
"""Abstract command interface — every operation must be a class."""
@abstractmethod
def execute(self) -> None: ...
@abstractmethod
def undo(self) -> None: ... # No undo system exists!
class CreateUserCommand(Command):
def __init__(self, user_store: object, username: str, email: str) -> None:
self._user_store = user_store
self._username = username
self._email = email
self._user_id: int | None = None
def execute(self) -> None:
self._user_id = self._user_store.create_user(self._username, self._email)
def undo(self) -> None:
# Would need to delete the user — but this code path is never used
if self._user_id:
raise NotImplementedError("Undo not implemented")
class UpdateUserEmailCommand(Command):
def __init__(self, user_store: object, user_id: int, new_email: str) -> None:
self._user_store = user_store
self._user_id = user_id
self._new_email = new_email
self._old_email: str | None = None
def execute(self) -> None:
self._old_email = self._user_store.get_user_email(self._user_id)
self._user_store.update_email(self._user_id, self._new_email)
def undo(self) -> None:
# Undo is dead code — the system never calls it
if self._old_email:
raise NotImplementedError("Undo not implemented")
class DeleteUserCommand(Command):
def __init__(self, user_store: object, user_id: int) -> None:
self._user_store = user_store
self._user_id = user_id
def execute(self) -> None:
self._user_store.delete_user(self._user_id)
def undo(self) -> None:
raise NotImplementedError("Cannot undo user deletion")
class CommandQueue:
"""Queuing system that nobody uses — commands are executed immediately."""
def __init__(self) -> None:
self._queue: list[Command] = [] # Always empty
def enqueue(self, command: Command) -> None:
self._queue.append(command)
def process_next(self) -> None:
if not self._queue:
return # Always returns immediately
cmd = self._queue.pop(0)
cmd.execute()
class UserController:
"""Controller must know about commands instead of just calling service methods."""
def __init__(self, user_store: object, command_queue: CommandQueue) -> None:
self._user_store = user_store
self._command_queue = command_queue # Queue is never used for anything
def create_user(self, username: str, email: str) -> int:
cmd = CreateUserCommand(self._user_store, username, email)
cmd.execute() # Why wrap this in a command? Just call the method directly.
return 1 # Simplified return
def update_email(self, user_id: int, new_email: str) -> None:
cmd = UpdateUserEmailCommand(self._user_store, user_id, new_email)
cmd.execute()
# ✅ GOOD — Simple functions and class methods when no queue or undo is needed
class UserRepository:
"""Simple data access with clear method signatures."""
def __init__(self, db: object) -> None:
self._db = db
def create_user(self, username: str, email: str) -> int:
"""Create a new user and return the assigned ID.
Args:
username: Unique login name.
email: Contact email address (must be unique).
Returns:
The new user's database ID.
Raises:
ValueError: If email is already registered.
DatabaseError: If the database write fails.
"""
if self.exists_by_email(email):
raise ValueError(f"Email {email} is already registered")
return self._db.execute(
"INSERT INTO users (username, email) VALUES (?, ?) RETURNING id",
(username, email),
)
def update_email(self, user_id: int, new_email: str) -> None:
"""Update a user's email address.
Args:
user_id: The user to modify.
new_email: The new email address.
Raises:
ValueError: If the new email is already in use.
"""
if self.exists_by_email(new_email):
raise ValueError(f"Email {new_email} is already registered")
self._db.execute("UPDATE users SET email = ? WHERE id = ?", (new_email, user_id))
def delete_user(self, user_id: int) -> None:
"""Permanently delete a user. Irreversible operation."""
self._db.execute("DELETE FROM users WHERE id = ?", (user_id,))
# ✅ GOOD — Command pattern is justified when undo/redo or queuing IS needed
from collections import deque
class UndoableCommand:
"""Base class for commands that support undo operations."""
def __init__(self) -> None:
self._executed = False
def execute(self) -> None:
self._do_execute()
self._executed = True
@abstractmethod
def _do_execute(self) -> None: ...
@abstractmethod
def undo(self) -> None: ...
class EditDocumentCommand(UndoableCommand):
"""Document editing command with real undo support — the Command pattern is justified here."""
def __init__(self, document: "Document", insertion_point: int, text_to_add: str) -> None:
super().__init__()
self._document = document
self._insertion_point = insertion_point
self._text_to_add = text_to_add
self._original_content: str = ""
def _do_execute(self) -> None:
doc_text = self._document.get_content()
self._original_content = doc_text
new_text = doc_text[:self._insertion_point] + self._text_to_add + doc_text[self._insertion_point:]
self._document.set_content(new_text)
def undo(self) -> None:
if not self._executed:
return
self._document.set_content(self._original_content)
class UndoManager:
"""The Command pattern is justified when you need to track and replay operations."""
def __init__(self, max_history: int = 100) -> None:
self._history: deque[UndoableCommand] = deque(maxlen=max_history)
def execute(self, command: UndoableCommand) -> None:
command.execute()
self._history.append(command)
def undo(self) -> None:
if not self._history:
return
command = self._history.pop()
command.undo()Why this is worse: The BAD version creates Command objects for operations where there is no undo system, no queue, and no need to log or retry commands. Each operation wraps a simple method call in an extra class plus a CommandQueue that always stays empty. The undo() methods are all unimplemented dead code.
When Command pattern IS justified: For UI edit operations with undo/redo stacks, job queues where tasks may be deferred to background workers, request logging systems, or macro recording features. The key signal is: do you need to store, replay, or defer the operation?
Template Method Rigidity
Using Template Method pattern (inheritance-based) instead of Strategy pattern (composition-based) when the variation points are behavioral rather than structural. This creates fragile base class problems, makes testing harder, and prevents combining behaviors independently. Python's duck typing and protocols make inheritance-heavy patterns especially unnatural.
# ❌ BAD — Template Method using deep inheritance hierarchy
from abc import ABC, abstractmethod
class DataProcessorPipeline(ABC):
"""Abstract pipeline with template method enforcing a specific processing order."""
def process(self, raw_data: dict) -> dict:
"""Template method — defines the fixed sequence of operations.
Subclasses override individual steps but cannot reorder them."""
validated = self.validate(raw_data)
transformed = self.transform(validated)
enriched = self.enrich(transformed)
return self.finalize(enriched)
@abstractmethod
def validate(self, data: dict) -> dict:
"""Validate and sanitize input data."""
...
@abstractmethod
def transform(self, data: dict) -> dict:
"""Transform validated data into the internal representation."""
...
@abstractmethod
def enrich(self, data: dict) -> dict:
"""Enrich data with additional information from external sources."""
...
def finalize(self, data: dict) -> dict:
"""Default finalization — shared by all subclasses."""
return {"status": "processed", "data": data}
class CsvProcessingPipeline(DataProcessorPipeline):
"""CSV-specific processing pipeline — inherits the template method."""
def validate(self, data: dict) -> dict:
if "csv_header" not in data:
raise ValueError("Missing CSV header")
return data
def transform(self, data: dict) -> dict:
rows = data["csv_rows"]
headers = data["csv_header"]
return [dict(zip(headers, row)) for row in rows]
def enrich(self, data: dict) -> dict:
# CSV enrichment logic — but now this class is stuck with the pipeline template
enriched = []
for row in data:
row["processed_at"] = "now"
enriched.append(row)
return enriched
class JsonProcessingPipeline(DataProcessorPipeline):
"""JSON-specific processing pipeline — must follow CSV's processing order even if JSON needs a different one."""
def validate(self, data: dict) -> dict:
if not isinstance(data.get("items"), list):
raise ValueError("Missing items array")
return data
def transform(self, data: dict) -> dict:
return {"records": data["items"], "count": len(data["items"])}
def enrich(self, data: dict) -> dict:
for record in data.get("records", []):
record["format_version"] = 2
return data
class ApiProcessingPipeline(DataProcessorPipeline):
"""API-specific pipeline — but it needs to skip enrichment and go straight to validation+transform.
The template method forces it to call enrich() even though API responses don't need enrichment."""
def validate(self, data: dict) -> dict:
if "api_response" no
*Truncated - read the full file at https://github.com/paulpas/agent-skill-router/blob/03735f82d38538c0a7ba34a6056fa8e5e3704be8/skills/coding/design-pattern-anti-patterns/SKILL.md.*