Function Design Review: SRP, Cohesion, and Coupling
Have you ever received a PR with a function that's 250 lines long? As you scroll down and down, the list of things this function does keeps growing: input verification, database queries, business logic calculation, external API calls, email sending, logging, cache updates. By the end, you can't answer in a single sentence what this function is actually supposed to do.
This is what violating the Single Responsibility Principle looks like in practice. It's a code smell that signals problems both now and in the future.
The "And" Test: The Simplest Design Check
When reviewing a PR, look at a function and ask: What does this function do?
If your one-sentence answer includes "and," the function is doing too much and has multiple reasons to change:
- "Save the user and send the email" → Two separate responsibilities
- "Verify orders, process payments, and deduct inventory" → Three separate concerns
- "Record logs, format errors, and send Slack notifications" → Three different systems involved
Each "and" represents a separate reason this function could need to be modified. If email sending logic changes, your user saving code is at risk. If the inventory system changes, payment processing code needs review. If logging format changes, Slack notification code might break.
The fix is separation and delegation:
// Before: too many responsibilities
public void processOrder(OrderRequest request) {
// 1. Validation
if (request.getItems() == null) throw new IllegalArgumentException();
if (request.getUserId() == null) throw new IllegalArgumentException();
// 2. Inventory management
for (OrderItem item : request.getItems()) {
Product product = productRepository.findById(item.getProductId());
product.decreaseStock(item.getQuantity());
productRepository.save(product);
}
// 3. Calculate total
BigDecimal totalAmount = calculateTotal(request.getItems());
// 4. Save order
Order order = new Order(...);
orderRepository.save(order);
// 5. Send email
User user = userRepository.findById(request.getUserId());
emailService.sendOrderConfirmation(user.getEmail(), order);
// 6. Award points
user.addPoints(totalAmount.intValue() / 100);
userRepository.save(user);
}
// After: separated responsibilities
public void processOrder(OrderRequest request) {
orderValidator.validate(request);
inventoryService.reserveStock(request.getItems());
BigDecimal totalAmount = orderPricingService.calculateTotal(request.getItems());
Order order = orderRepository.save(Order.create(request.getUserId(), request.getItems(), totalAmount));
eventPublisher.publish(new OrderPlacedEvent(order));
}
// Email and points handled by separate event subscribers
@Component
public class OrderEmailHandler {
@EventListener
public void onOrderPlaced(OrderPlacedEvent event) {
emailService.sendOrderConfirmation(event.getOrder());
}
}
Now `processOrder` is one sentence: "Process the order." No "and". Each change to email, points, or inventory logic happens in isolation, without risking order processing.
Three Patterns That Signal SRP Violations
Pattern 1: Multiple Section Comments
// 1. validation
...
// 2. data retrieval
...
// 3. logic processing
...
// 4. result persistence
...
If a function needs numbered section comments to organize its logic, that's a sign it can be divided into multiple functions — one per section.
Pattern 2: Unrelated Imports Mixed in One File
import { sendEmail } from '../email/emailService';
import { deductStock } from '../inventory/inventoryService';
import { chargeCard } from '../payment/stripeService';
import { generatePDF } from '../reports/pdfGenerator';
import { postToSlack } from '../notifications/slackClient';
If one file depends on five totally different domains, it's taking on too much responsibility. Different domains have different change frequencies and different owners.
Pattern 3: Functions That Complicate Testing
When writing tests, if you think "To test this function, I have to mock the DB, mock the email service, mock the payment gateway, mock the cache service..." the function is doing too much.
Well-designed, single-responsibility functions are easy to test. Complex tests are a symptom of complex design.
Cohesion: "Do They Change Together?"
Cohesion means code in the same module is actually together for the same reason. Consider this React component with low cohesion:
function Dashboard() {
const [user, setUser] = useState<User | null>(null);
const [orders, setOrders] = useState<Order[]>([]);
const [notifications, setNotifications] = useState<Notification[]>([]);
const [weatherData, setWeatherData] = useState<Weather | null>(null);
const [stockPrices, setStockPrices] = useState<Stock[]>([]);
useEffect(() => {
fetchUser().then(setUser);
fetchOrders().then(setOrders);
fetchNotifications().then(setNotifications);
fetchWeather().then(setWeatherData);
fetchStockPrices().then(setStockPrices);
}, []);
return (
<div>
<UserProfile user={user} />
<OrderList orders={orders} />
<NotificationPanel notifications={notifications} />
<WeatherWidget weather={weatherData} />
<StockTicker stocks={stockPrices} />
</div>
);
}
This component is modified if weather API changes. It's modified if ordering logic changes. These five things have no relationship to each other. Low cohesion means unrelated things are forced together.
High cohesion solution: Each child component owns its own data. `Dashboard` only orchestrates layout.
function Dashboard() {
return (
<div>
<UserProfile /> {/* uses useUser hook internally */}
<OrderList /> {/* uses useOrders hook internally */}
<NotificationPanel /> {/* uses useNotifications hook internally */}
<WeatherWidget />
<StockTicker />
</div>
);
}
Coupling: "Does Change Break Unexpected Places?"
High coupling means one module depends on the internal implementation details of another module. If changing A requires changing B, they're too tightly coupled.
Classic example of tight coupling:
// Before: tight coupling - Controller knows Repository internals
class UserController {
async getUser(req, res) {
const user = await db.query(
'SELECT id, email, name, created_at FROM users WHERE id = $1 AND deleted_at IS NULL',
[req.params.id]
);
if (!user.rows[0]) return res.status(404).json({ error: 'User not found' });
res.json({
id: user.rows[0].id,
email: user.rows[0].email,
name: user.rows[0].name,
joinedAt: user.rows[0].created_at, // Controller knows DB column names
});
}
}
// After: loose coupling - Controller only knows Repository interface
class UserRepository {
async findActiveById(userId) {
const result = await db.query(
'SELECT id, email, name, created_at FROM users WHERE id = $1 AND deleted_at IS NULL',
[userId]
);
if (!result.rows[0]) return null;
return User.fromRow(result.rows[0]); // Converts DB row to domain model
}
}
class UserController {
async getUser(req, res) {
const user = await this.userRepository.findActiveById(req.params.id);
if (!user) return res.status(404).json({ error: 'User not found' });
res.json(user.toResponse()); // Delegates to domain model
}
}
Now if the DB column `created_at` changes to `registered_at`, only `UserRepository` needs updating. `UserController` doesn't care about the internal details. It only knows the contract: "give me a user or null."
How to Communicate Design Issues
When you find a design problem, don't just say "this violates SRP." Explain the risk concretely:
Better: "This function handles user creation, email sending, point allocation, and subscription management. If email logic changes, user creation risks breaking. If points system changes, subscription code is at risk. These are independent concerns with different reasons to change. I suggest: move email into an event handler that subscribes to UserCreated events, move points into another event handler, and keep user creation logic focused."
This explains the specific risks that will materialize, making the design issue tangible.
The Cost of Poor Design: Real Examples
Example 1: The God Service
A service with 50 responsibilities: user management, email sending, payment processing, logging, caching, event publishing, external API calls. When you need to add one feature, you're in this massive file. When you need to test one thing, you mock 15 dependencies. When you fix one bug, three others appear. Six months later, developers fear touching it.
Example 2: The Tightly Coupled Layers
Controllers know about Repository internals. Services directly touch database column names. UI components know about API response structures. When the database schema changes, 30 files need updating. When the API response format changes, 50 UI components break. Changes that should be isolated cascade everywhere.
Example 3: The Circular Dependencies
UserService depends on OrderService. OrderService depends on UserService. You can't test one without the other. You can't deploy one without the other. You can't even import one in another file without circular dependency errors.
All three examples could have been prevented with better design during code review.
The Cost of Ignoring Design: Real Scenarios
Imagine three months pass. Your manager asks: "Can we add this new discount type to the system?" With well-designed code, you modify the DiscountService (30 minutes). With god-class code with 2000 lines and 15 responsibilities, you're afraid to change anything. The discount logic is intertwined with email, notifications, caching, and inventory. What should take 30 minutes takes 8 hours because you're carefully unwinding dependencies to change one thing.
Now multiply that by 50 similar changes over a year. A well-designed codebase saves hundreds of hours. But it's invisible benefit because it's the slowdown you prevented, not a feature you shipped.
Real Cost of Poor Design:
- Feature development slows from 2 weeks to 4 weeks as complexity increases
- Onboarding new team members takes 3 months instead of 2 weeks
- Bug fixes take 3x longer because code is interconnected
- Team retention drops because developers get frustrated
- Rewrites become necessary ("let's rebuild this from scratch")
Design Review is Prevention
Good design is preventive medicine. Well-designed code isn't faster to write initially — but it's significantly faster to maintain, modify, and debug later. It scales better as the codebase grows. It has fewer hidden failures in edge cases. It's easier to test. It's easier to understand.
When reviewing the next PR, try the "and" test. If a function has multiple "ands" in its description, it's time to split responsibilities. Your future self and your team will thank you when requirements change and you can modify one piece without touching five others. They'll thank you when onboarding new team members becomes easier because code is clearer and smaller.
Design Debt vs. Technical Debt
Technical debt is quick fixes that need to be cleaned up. Design debt is fundamental architectural problems that compound over time. Design debt is worse because it affects everything. A technical debt (one hack in one function) costs you 5 hours to fix. A design debt (entire architecture is tightly coupled) costs you weeks or months to fix.
Catch design debt early in code review. Once it's embedded in 50 files, it's expensive to change.
Comments
Post a Comment