Published on

Stop putting constants in catch-all classes.

The problem

At my previous gig, the outsourced IT team put in place some catch-all "constant" classes like: ValidationConstants, ErrorConstants, EventTypes.

Those "technical" classes existed solely to hold constants from across the entire codebase...

public class ValidationConstants {
    public static final Set<String> VALID_ORDER_STATUSES_FOR_CANCELLATION = Set.of("PENDING", "CONFIRMED");
    public static final Set<String> VALID_ORDER_STATUSES_FOR_REFUND = Set.of("SHIPPED", "DELIVERED");
    public static final int MAX_INVENTORY_RESERVATION_MINUTES = 15;
    public static final String SOME_UNRELATED_VALIDATION_THING = "...";
    // ... 200 more constants
}

These classes have low functional cohesion. They group things by what they are (constants) rather than what they do.

A better approach

Put constants where they're used:

public class OrderCancellationService {

    private static final Set<String> CANCELLABLE_STATUSES = Set.of("PENDING", "CONFIRMED");
    private static final String ERROR_NOT_CANCELLABLE = "ORDER_001";

    public CancellationResult cancel(Order order) {
        if (!CANCELLABLE_STATUSES.contains(order.getStatus())) {
            return CancellationResult.rejected(ERROR_NOT_CANCELLABLE);
        }
        // ...
    }
}

Now OrderCancellationService is self-contained. You don't need to click through to three different files to understand what it does.

Package-by-layer vs package-by-feature

This is the same debate as package-by-layer vs package-by-feature.

Package-by-layer groups by technical role:

├── controllers/
├── services/
├── constants/    ← everything dumped here
└── mappers/

Package-by-feature groups by business capability:

├── orders/
│   ├── OrderController.java
│   ├── OrderCancellationService.java  ← constants live here
│   └── OrderRefundService.java
└── inventory/

The second is more useful for the code reader because related things are together.