Common SRP Misconceptions

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74

/**
 * Order Aggregate - Acceptable "multi-responsibility" design
 * 
 * This appears to have multiple responsibilities, but all methods
 * serve one conceptual responsibility: "being a complete, valid Order."
 * 
 * The aggregate root ensures:
 * - Invariants are always maintained
 * - State transitions are valid
 * - Business rules are enforced
 * - The Order concept is coherent
 */
public class Order {
    private final OrderId id;
    private final List<LineItem> items;
    private OrderStatus status;
    private PaymentDetails payment;
    private Money totalAmount;
    
    // ========== Line Item "Responsibility" ==========
    public void addItem(Product product, int quantity) {
        ensureEditable();
        validateProduct(product);
        items.add(new LineItem(product, quantity));
        recalculateTotal();  // Interdependency: items affect total
    }
    
    public void removeItem(LineItemId itemId) {
        ensureEditable();
        items.removeIf(item -> item.getId().equals(itemId));
        recalculateTotal();  // Interdependency
    }
    
    // ========== Pricing "Responsibility" ==========
    public Money getTotal() {
        return totalAmount;
    }
    
    private void recalculateTotal() {
        Money subtotal = items.stream()
            .map(LineItem::getAmount)
            .reduce(Money.ZERO, Money::add);
        this.totalAmount = applyTax(subtotal);  // Interdependency: tax rules
    }
    
    // ========== State Management "Responsibility" ==========
    public void submit() {
        ensureNotEmpty();                // Invariant: can't submit empty order
        validateAllItemsInStock();       // Invariant: items must be available
        this.status = OrderStatus.SUBMITTED;
    }
    
    public void confirm(PaymentConfirmation paymentConfirmation) {
        ensureStatus(OrderStatus.SUBMITTED);
        this.payment = paymentConfirmation.toPaymentDetails();
        this.status = OrderStatus.CONFIRMED;  // Interdependency: payment + status
    }
    
    // ========== Validation "Responsibility" ==========
    private void ensureEditable() {
        if (status != OrderStatus.DRAFT) {
            throw new OrderNotEditableException(id, status);
        }
    }
    
    private void ensureNotEmpty() {
        if (items.isEmpty()) {
            throw new EmptyOrderException(id);
        }
    }
    
    // All these "responsibilities" are interdependent facets of ORDER
    // Splitting them would break invariant enforcement
}

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65

// Option 1: SRP-pure approach with extracted logging concern
public class PaymentProcessor {
    private final PaymentGateway gateway;
    private final PaymentLogger logger;  // Injected dependency
    
    public PaymentResult process(Payment payment) {
        logger.logPaymentStarted(payment);
        
        ValidationResult validation = validate(payment);
        logger.logValidationComplete(validation);
        
        if (!validation.isValid()) {
            logger.logPaymentRejected(payment, validation);
            return PaymentResult.rejected(validation);
        }
        
        GatewayResponse response = gateway.charge(payment);
        logger.logGatewayResponse(response);
        
        PaymentResult result = toResult(response);
        logger.logPaymentComplete(result);
        
        return result;
    }
}
 
// Problems:
// - 8 lines of business logic, 5 lines of logging calls
// - Logger needs to understand PaymentProcessor's internal flow
// - PaymentLogger interface couples to PaymentProcessor internals
// - Changing logging requires touching both classes
 
// Option 2: Pragmatic approach with inline logging
public class PaymentProcessor {
    private final PaymentGateway gateway;
    private final Logger log = LoggerFactory.getLogger(PaymentProcessor.class);
    
    public PaymentResult process(Payment payment) {
        log.info("Processing payment: customerId={}, amount={}", 
                 payment.getCustomerId(), payment.getAmount());
        
        ValidationResult validation = validate(payment);
        if (!validation.isValid()) {
            log.warn("Payment validation failed: {}", validation.getErrors());
            return PaymentResult.rejected(validation);
        }
        
        log.debug("Validation passed, charging gateway");
        GatewayResponse response = gateway.charge(payment);
        
        if (!response.isSuccessful()) {
            log.error("Gateway charge failed: {}", response.getError());
            return PaymentResult.failed(response.getError());
        }
        
        log.info("Payment successful: transactionId={}", response.getTransactionId());
        return PaymentResult.success(response.getTransactionId());
    }
}
 
// Benefits:
// - Logging is contextual and immediately understood
// - Business logic and its observability are co-located
// - No artificial abstraction layer
// - Changes to flow naturally include logging changes

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55

/**
 * OrderFacade - Acceptable multi-concern coordination
 * 
 * This class coordinates inventory, payment, shipping, and notification.
 * It APPEARS to have 4 responsibilities, but its ACTUAL responsibility
 * is "providing a simple interface for order processing."
 * 
 * Each concern is implemented elsewhere; this facade orchestrates them.
 */
public class OrderFacade {
    private final InventoryService inventoryService;
    private final PaymentService paymentService;
    private final ShippingService shippingService;
    private final NotificationService notificationService;
    
    /**
     * The facade's responsibility: Coordinate order processing workflow.
     * It doesn't IMPLEMENT any of these concerns; it ORCHESTRATES them.
     */
    public OrderResult processOrder(Order order) {
        // Step 1: Reserve inventory
        InventoryReservation reservation = inventoryService.reserve(order.getItems());
        if (!reservation.isSuccessful()) {
            return OrderResult.inventoryFailed(reservation);
        }
        
        try {
            // Step 2: Process payment
            PaymentResult payment = paymentService.charge(order.getPaymentMethod(), 
                                                         order.getTotal());
            if (!payment.isSuccessful()) {
                inventoryService.release(reservation);  // Rollback
                return OrderResult.paymentFailed(payment);
            }
            
            // Step 3: Arrange shipping
            ShippingLabel label = shippingService.createLabel(order.getShippingAddress(),
                                                              order.getItems());
            
            // Step 4: Notify customer
            notificationService.sendOrderConfirmation(order.getCustomerEmail(), 
                                                      order, label);
            
            return OrderResult.success(order, payment, label);
            
        } catch (Exception e) {
            inventoryService.release(reservation);  // Rollback on any failure
            throw new OrderProcessingException(order.getId(), e);
        }
    }
}
 
// The facade's ONE responsibility: "Coordinate the order processing workflow"
// It delegates each concern to specialists
// Splitting the facade would scatter the workflow, making it harder to understand

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47

/**
 * UserRegistrationService - Application layer orchestrator
 * 
 * Coordinates: validation, user creation, email verification, welcome flow.
 * Responsibility: "Orchestrating the user registration use case."
 */
@Service
@Transactional
public class UserRegistrationService {
    private final UserValidator validator;
    private final UserRepository userRepository;
    private final EmailVerificationService emailService;
    private final WelcomeFlowService welcomeFlow;
    private final EventPublisher eventPublisher;
    
    public RegistrationResult register(RegistrationRequest request) {
        // Validate
        ValidationResult validation = validator.validate(request);
        if (!validation.isValid()) {
            return RegistrationResult.invalid(validation);
        }
        
        // Check uniqueness
        if (userRepository.existsByEmail(request.getEmail())) {
            return RegistrationResult.emailTaken();
        }
        
        // Create user
        User user = User.create(request.getEmail(), request.getName());
        userRepository.save(user);
        
        // Initiate email verification
        emailService.sendVerification(user);
        
        // Start welcome flow
        welcomeFlow.initiate(user);
        
        // Publish event for other systems
        eventPublisher.publish(new UserRegisteredEvent(user));
        
        return RegistrationResult.success(user);
    }
}
 
// This touches validation, persistence, email, welcome flow, and events.
// But its single responsibility is: "Execute the registration use case."
// Each concern is delegated; this class only coordinates.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79

/**
 * OrderController - Thin infrastructure glue
 * 
 * Does this violate SRP? It handles:
 * - HTTP request mapping
 * - Request body parsing
 * - Validation triggering
 * - Service delegation
 * - Response formatting
 * - Error translation
 * 
 * But its actual responsibility is: "Translate HTTP to application layer."
 */
@RestController
@RequestMapping("/api/orders")
public class OrderController {
    private final OrderApplicationService orderService;
    private final OrderResponseMapper mapper;
    
    @PostMapping
    public ResponseEntity<OrderResponse> createOrder(
            @Valid @RequestBody CreateOrderRequest request,
            @AuthenticationPrincipal User user) {
        
        // Translate HTTP request to application command
        CreateOrderCommand command = new CreateOrderCommand(
            user.getId(),
            request.getItems(),
            request.getShippingAddressId()
        );
        
        // Delegate to application layer
        OrderResult result = orderService.createOrder(command);
        
        // Translate application result to HTTP response
        if (result.isSuccessful()) {
            OrderResponse response = mapper.toResponse(result.getOrder());
            return ResponseEntity.status(HttpStatus.CREATED).body(response);
        }
        
        return handleFailure(result);
    }
    
    @GetMapping("/{orderId}")
    public ResponseEntity<OrderResponse> getOrder(
            @PathVariable OrderId orderId,
            @AuthenticationPrincipal User user) {
        
        return orderService.findOrder(orderId, user.getId())
            .map(mapper::toResponse)
            .map(ResponseEntity::ok)
            .orElse(ResponseEntity.notFound().build());
    }
    
    @PutMapping("/{orderId}/cancel")
    public ResponseEntity<Void> cancelOrder(
            @PathVariable OrderId orderId,
            @AuthenticationPrincipal User user) {
        
        CancelResult result = orderService.cancelOrder(orderId, user.getId());
        return result.isSuccessful() 
            ? ResponseEntity.noContent().build()
            : ResponseEntity.status(HttpStatus.CONFLICT).build();
    }
    
    private ResponseEntity<OrderResponse> handleFailure(OrderResult result) {
        return switch (result.getFailureType()) {
            case VALIDATION -> ResponseEntity.badRequest().body(errorResponse(result));
            case INVENTORY -> ResponseEntity.status(HttpStatus.CONFLICT).body(errorResponse(result));
            case PAYMENT -> ResponseEntity.status(HttpStatus.PAYMENT_REQUIRED).body(errorResponse(result));
            default -> ResponseEntity.internalServerError().build();
        };
    }
}
 
// This controller touches HTTP, validation, authorization, services, and mapping.
// But its ONE responsibility is: "Expose order operations via HTTP."
// Splitting into CreateOrderController, GetOrderController, CancelOrderController
// would scatter related endpoints with no benefit.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56

/**
 * ApplicationConfiguration - Composition root
 * 
 * This class configures: database, caching, messaging, security, and more.
 * It MUST touch all these concerns—that's its entire purpose.
 * 
 * Responsibility: "Wire up application dependencies correctly."
 */
@Configuration
public class ApplicationConfiguration {
    
    @Bean
    public DataSource dataSource(DatabaseProperties props) {
        HikariConfig config = new HikariConfig();
        config.setJdbcUrl(props.getUrl());
        config.setUsername(props.getUsername());
        config.setPassword(props.getPassword());
        config.setMaximumPoolSize(props.getPoolSize());
        return new HikariDataSource(config);
    }
    
    @Bean
    public CacheManager cacheManager(CacheProperties props) {
        CaffeineCacheManager manager = new CaffeineCacheManager();
        manager.setCaffeine(Caffeine.newBuilder()
            .expireAfterWrite(props.getTtl())
            .maximumSize(props.getMaxSize()));
        return manager;
    }
    
    @Bean
    public MessageQueue messageQueue(MessagingProperties props) {
        return new RabbitMQQueue(props.getHost(), props.getPort());
    }
    
    @Bean
    public PasswordEncoder passwordEncoder() {
        return new BCryptPasswordEncoder(12);
    }
    
    @Bean
    public ObjectMapper objectMapper() {
        return new ObjectMapper()
            .registerModule(new JavaTimeModule())
            .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false);
    }
    
    @Bean
    public Clock clock() {
        return Clock.systemUTC();  // Enables testing with fake clocks
    }
}
 
// This touches database, caching, messaging, security, serialization...
// But its ONE responsibility is: "Provide correctly configured dependencies."
// Asking this class to follow domain-level SRP is nonsensical.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44

/**
 * ApplicationBootstrap - Startup coordination
 * 
 * Ensures correct initialization order:
 * 1. Validate configuration
 * 2. Run database migrations
 * 3. Warm caches
 * 4. Register with service discovery
 * 5. Start accepting traffic
 */
@Component
public class ApplicationBootstrap implements ApplicationListener<ApplicationReadyEvent> {
    private final ConfigurationValidator configValidator;
    private final DatabaseMigrator migrator;
    private final CacheWarmer cacheWarmer;
    private final ServiceRegistry serviceRegistry;
    private final HealthIndicator healthIndicator;
    
    @Override
    public void onApplicationEvent(ApplicationReadyEvent event) {
        log.info("Application starting up...");
        
        // Phase 1: Configuration
        configValidator.validate();
        log.info("Configuration validated");
        
        // Phase 2: Database
        migrator.migrate();
        log.info("Database migrations complete");
        
        // Phase 3: Caching
        cacheWarmer.warmCriticalCaches();
        log.info("Caches warmed");
        
        // Phase 4: Service discovery
        serviceRegistry.register(healthIndicator);
        log.info("Registered with service discovery");
        
        log.info("Application ready to serve traffic");
    }
}
 
// Responsibility: "Ensure correct application startup."
// This MUST touch all startup concerns by definition.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109

/**
 * Money - Value object with multiple operations
 * 
 * Operations include:
 * - Factory methods (parsing)
 * - Validation (invariant enforcement)
 * - Arithmetic (add, subtract, multiply)
 * - Comparison (equals, compareTo)
 * - Formatting (toString, for display)
 * 
 * These aren't separate responsibilities—they're facets of "being Money."
 */
public final class Money implements Comparable<Money> {
    public static final Money ZERO = new Money(BigDecimal.ZERO, Currency.USD);
    
    private final BigDecimal amount;
    private final Currency currency;
    
    // === Factory/Parsing "Responsibility" ===
    
    public static Money of(BigDecimal amount, Currency currency) {
        requireNonNull(amount, "amount");
        requireNonNull(currency, "currency");
        if (amount.scale() > currency.getDecimalPlaces()) {
            throw new InvalidMoneyException("Too many decimal places");
        }
        return new Money(amount, currency);
    }
    
    public static Money parse(String text) {
        // Parse "$100.00 USD" format
    }
    
    public static Money dollars(double amount) {
        return of(BigDecimal.valueOf(amount), Currency.USD);
    }
    
    // === Arithmetic "Responsibility" ===
    
    public Money add(Money other) {
        requireSameCurrency(other);
        return new Money(amount.add(other.amount), currency);
    }
    
    public Money subtract(Money other) {
        requireSameCurrency(other);
        return new Money(amount.subtract(other.amount), currency);
    }
    
    public Money multiply(BigDecimal factor) {
        return new Money(amount.multiply(factor), currency);
    }
    
    public Money percentage(BigDecimal percent) {
        return multiply(percent.divide(BigDecimal.valueOf(100)));
    }
    
    // === Comparison "Responsibility" ===
    
    public boolean isPositive() {
        return amount.signum() > 0;
    }
    
    public boolean isNegative() {
        return amount.signum() < 0;
    }
    
    public boolean isZero() {
        return amount.signum() == 0;
    }
    
    public boolean isGreaterThan(Money other) {
        requireSameCurrency(other);
        return amount.compareTo(other.amount) > 0;
    }
    
    @Override
    public int compareTo(Money other) {
        requireSameCurrency(other);
        return amount.compareTo(other.amount);
    }
    
    // === Formatting "Responsibility" ===
    
    public String format() {
        return currency.format(amount);
    }
    
    public String formatWithSymbol() {
        return currency.getSymbol() + format();
    }
    
    @Override
    public String toString() {
        return String.format("%s %s", amount, currency.getCode());
    }
    
    // === Validation (enforced in constructor and operations) ===
    
    private void requireSameCurrency(Money other) {
        if (!currency.equals(other.currency)) {
            throw new CurrencyMismatchException(currency, other.currency);
        }
    }
}
 
// All these operations answer ONE question: "How does Money behave?"
// Extracting MoneyParser, MoneyArithmetic, MoneyFormatter would destroy
// the coherence of the Money concept.

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59

// Applying the checklist to OrderAggregate from earlier:
 
/**
 * 1. ONE conceptual purpose? 
 *    YES - "Represent and manage a valid Order"
 * 
 * 2. Would splitting break cohesion?
 *    YES - Items, pricing, and status are interdependent invariants
 * 
 * 3. Delegation not implementation?
 *    PARTIAL - Some logic is internal, but domain entities should own behavior
 * 
 * 4. Infrastructure glue?
 *    NO - This is domain logic, not infrastructure
 * 
 * 5. One actor/stakeholder?
 *    YES - The Ordering team owns all Order behavior
 * 
 * 6. Changes affect everything together?
 *    YES - Order logic changes affect multiple methods
 * 
 * 7. Does the name make sense?
 *    YES - "Order" perfectly captures the concept
 * 
 * RESULT: 6/7 YES - Acceptable, not an SRP violation
 */
public class Order {
    // ... rich aggregate with many methods
}
 
// Applying to a problematic class:
 
/**
 * 1. ONE conceptual purpose?
 *    NO - Imports data AND sends notifications AND generates reports
 * 
 * 2. Would splitting break cohesion?
 *    NO - These are independent operations with no shared state
 * 
 * 3. Delegation not implementation?
 *    NO - Each operation fully implemented here
 * 
 * 4. Infrastructure glue?
 *    NO - This is application logic
 * 
 * 5. One actor/stakeholder?
 *    NO - Data team, marketing, and analytics all request changes
 * 
 * 6. Changes affect everything together?
 *    NO - Import changes don't affect notification logic
 * 
 * 7. Does the name make sense?
 *    NO - Named "DataHandler" with vague scope
 * 
 * RESULT: 0/7 YES - Genuine SRP violation, needs refactoring
 */
public class DataHandler {
    // ... should be split into DataImporter, NotificationService, ReportGenerator
}