Shotgun Surgery: When One Change Requires Many Edits
Shotgun Surgery is a code smell where a single logical change requires you to modify many different classes. Learn how to identify and fix it by consolidating responsibilities.
You have a simple task: add a new user type, “Auditor,” to your application. This new user should have read-only access. You expect it to be a quick change, but it turns into a nightmare. You find yourself hunting through a dozen different files, adding if (user.type === 'Auditor') checks in multiple places:
- In the
PaymentControllerto block payment actions. - In the
UserProfileUIto hide the “Edit Profile” button. - In the
DataExportServiceto prevent exporting sensitive data. - In the
AdminDashboardto filter them out of the active users list.
This is Shotgun Surgery. It’s the code smell where one logical change forces you to make small changes in many different classes. Every time you touch a common feature, you’re peppering the codebase with edits, like a shotgun blast.
Why Shotgun Surgery is a Problem
- Error-Prone: It’s incredibly easy to miss one of the required edits. If you forget the check in the
PaymentController, an Auditor might be able to make payments, creating a serious bug. - High Maintenance Cost: What should be a simple change becomes a time-consuming and risky expedition through the codebase.
- Violates DRY and SRP: Shotgun Surgery is a clear violation of the “Don’t Repeat Yourself” (DRY) and “Single Responsibility Principle” (SRP). The logic for a single concept (e.g., “user permissions”) is smeared across many unrelated classes instead of being encapsulated in one place.
Example: Managing User Permissions
Let’s look at a concrete example in TypeScript. We have a system where different user roles have different capabilities.
Before: The Shotgun Surgery Smell
Imagine we add a new isPremium flag for users. Now we have to change logic everywhere.
// user.ts
interface User {
id: string;
role: 'Admin' | 'Editor' | 'Viewer';
isPremium: boolean;
}
// article-editor.ts
class ArticleEditor {
editArticle(user: User, article: Article) {
// Check 1: Smells bad
if (user.role === 'Admin' || user.role === 'Editor' || user.isPremium) {
console.log("Editing article...");
} else {
throw new Error("Permission denied.");
}
}
}
// video-processor.ts
class VideoProcessor {
processHighQualityVideo(user: User, video: Video) {
// Check 2: The same logic is repeated here
if (user.role === 'Admin' || user.isPremium) {
console.log("Processing in 4K...");
} else {
console.log("Processing in 1080p.");
}
}
}
// analytics-dashboard.ts
class AnalyticsDashboard {
viewAdvancedMetrics(user: User) {
// Check 3: And again here...
if (user.role === 'Admin' || user.isPremium) {
console.log("Showing advanced metrics.");
} else {
throw new Error("Access to advanced metrics denied.");
}
}
}If we want to change the rule (e.g., Editors also get access to advanced metrics), we have to hunt down and change every single if statement. This is fragile and tedious.
The Solution: Consolidate the Responsibility
The fix for Shotgun Surgery is to move all the scattered logic into a single, cohesive class or module. In this case, we can create a PermissionService or add the logic to the User model itself.
After: A Centralized Permission Class
Let’s create a new class whose only job is to understand permissions.
// user.ts (remains the same)
interface User {
id: string;
role: 'Admin' | 'Editor' | 'Viewer';
isPremium: boolean;
}
// NEW: permission.service.ts
class PermissionService {
public static canEditContent(user: User): boolean {
return user.role === 'Admin' || user.role === 'Editor' || user.isPremium;
}
public static canAccessPremiumFeatures(user: User): boolean {
return user.role === 'Admin' || user.isPremium;
}
public static canViewAdvancedMetrics(user: User): boolean {
// Let's say we changed the rule for Editors
return user.role === 'Admin' || user.role === 'Editor' || user.isPremium;
}
}
// Now, our classes are much cleaner and decoupled from permission logic.
// article-editor.ts
class ArticleEditor {
editArticle(user: User, article: Article) {
if (!PermissionService.canEditContent(user)) {
throw new Error("Permission denied.");
}
console.log("Editing article...");
}
}
// video-processor.ts
class VideoProcessor {
processHighQualityVideo(user: User, video: Video) {
if (PermissionService.canAccessPremiumFeatures(user)) {
console.log("Processing in 4K...");
} else {
console.log("Processing in 1080p.");
}
}
}
// analytics-dashboard.ts
class AnalyticsDashboard {
viewAdvancedMetrics(user: User) {
if (!PermissionService.canViewAdvancedMetrics(user)) {
throw new Error("Access to advanced metrics denied.");
}
console.log("Showing advanced metrics.");
}
}Now, if we need to change a permission rule, we go to one place: PermissionService.ts. The change is simple, safe, and easy. We’ve fixed the Shotgun Surgery.
Python Example: Order Validation
In Python, the problem is identical. Imagine validating an order object in different parts of an e-commerce system.
Before: Scattered Validation Logic
# In discounts.py
def apply_coupon(order: dict, user: dict):
# Validation check 1
if not order.get("items"):
raise ValueError("Order has no items.")
if user.get("is_banned"):
raise ValueError("User is banned.")
# ... apply coupon logic ...
# In shipping.py
def calculate_shipping_cost(order: dict, user: dict):
# Validation check 2 (repeated)
if not order.get("items"):
raise ValueError("Order has no items.")
if not order.get("shipping_address"):
raise ValueError("Order has no shipping address.")
# ... calculate shipping logic ...
# In payments.py
def process_payment(order: dict, user: dict):
# Validation check 3 (partially repeated)
if user.get("is_banned"):
raise ValueError("User is banned.")
if sum(item['price'] for item in order.get("items", [])) <= 0:
raise ValueError("Cannot process zero-value order.")
# ... process payment logic ...After: A Central OrderValidator
We can consolidate this logic into a dedicated validator class.
# NEW: validators.py
class OrderValidator:
def __init__(self, order: dict, user: dict):
self._order = order
self._user = user
self._errors = []
def validate(self):
self._validate_items()
self._validate_user()
self._validate_address_for_shipping()
if self._errors:
raise ValueError(f"Invalid order: {', '.join(self._errors)}")
def _validate_items(self):
if not self._order.get("items"):
self._errors.append("Order has no items")
if sum(item.get('price', 0) for item in self._order.get("items", [])) <= 0:
self._errors.append("Order value must be positive")
def _validate_user(self):
if self._user.get("is_banned"):
self._errors.append("User is banned")
def _validate_address_for_shipping(self):
# This logic only runs if it's a physical order, for example
if self._order.get("needs_shipping") and not self._order.get("shipping_address"):
self._errors.append("Shipping address is required")
# Now the other modules are clean
def apply_coupon(order: dict, user: dict):
OrderValidator(order, user).validate() # Run all standard validations
# ... apply coupon logic ...
def calculate_shipping_cost(order: dict, user: dict):
OrderValidator(order, user).validate()
# ... calculate shipping logic ...How to Fix Shotgun Surgery
- Identify the scattered logic. Look for similar
ifstatements, validation checks, or setup code that appears in multiple classes. - Find a home for the responsibility. Create a new class or module, or find an existing class that is the logical owner of this behavior. The goal is to give this concept a single, authoritative home.
- Move the logic. Use refactoring techniques like “Move Method” or “Extract Class” to consolidate the scattered code into the new home.
- Replace the old logic with calls to the new module. Go back to all the places you identified in step 1 and replace the old, duplicated code with a single call to your new, centralized method.
By being vigilant for Shotgun Surgery, you can create a codebase that is more cohesive, easier to maintain, and far less prone to bugs.