Clean Code from the trenches – Validation
Let’s directly start with an example. Consider a simple web service which allows clients to place order to a shop. A very simplified version of the order controller could look something like below –
@RestController
@RequestMapping(value = "/",
consumes = MediaType.APPLICATION_JSON_VALUE,
produces = MediaType.APPLICATION_JSON_VALUE)
public class OrderController {
private final OrderService orderService;
public OrderController(OrderService orderService) {
this.orderService = orderService;
}
@PostMapping
public void doSomething(@Valid @RequestBody OrderDTO order) {
orderService.createOrder(order);
}
}And the corresponding DTO class
@Getter
@Setter
@ToString
public class OrderDTO {
@NotNull
private String customerId;
@NotNull
@Size(min = 1)
private List<OrderItem> orderItems;
@Getter
@Setter
@ToString
public static class OrderItem {
private String menuId;
private String description;
private String price;
private Integer quantity;
}
}The most common approach for creating an order from this DTO is to pass it to a service, validate it as necessary, and then persist it in the database
@Service
@Slf4j
class OrderService {
private final MenuRepository menuRepository;
OrderService(MenuRepository menuRepository) {
this.menuRepository = menuRepository;
}
void createOrder(OrderDTO orderDTO) {
orderDTO.getOrderItems()
.forEach(this::validate);
log.info("Order {} saved", orderDTO);
}
private void validate(OrderItem orderItem) {
String menuId = orderItem.getMenuId();
if (menuId == null || menuId.trim().isEmpty()) {
throw new IllegalArgumentException("A menu item must be specified.");
}
if (!menuRepository.menuExists(menuId.trim())) {
throw new IllegalArgumentException("Given menu " + menuId + " does not exist.");
}
String description = orderItem.getDescription();
if (description == null || description.trim().isEmpty()) {
throw new IllegalArgumentException("Item description should be provided");
}
String price = orderItem.getPrice();
if (price == null || price.trim().isEmpty()) {
throw new IllegalArgumentException("Price cannot be empty.");
}
try {
new BigDecimal(price);
} catch (NumberFormatException ex) {
throw new IllegalArgumentException("Given price is not in valid format", ex);
}
if (orderItem.getQuantity() == null) {
throw new IllegalArgumentException("Quantity must be given");
}
if (orderItem.getQuantity() <= 0) {
throw new IllegalArgumentException("Given quantity "
+ orderItem.getQuantity()
+ " is not valid.");
}
}
}The validate method is not well written. It is very hard to test. Introducing new validation rule in the future is also hard, and so is removing/modifying any of the existing ones. From my experience I have seen that most people write a few generic assertions for this type of validation check, typically in an integration test class, touching only one or two (or more, but not all) of the validation rules. As a result, refactoring in the future can only be done in Edit and Pray mode.
We can improve the code structure if we use Polymorphism to replace these conditionals. Let’s create a common super type for representing a single validation rule
public interface OrderItemValidator {
void validate(OrderItem orderItem);
}Next step is to create validation rule implementations which will focus on separate validation areas for the DTO. Let’s start with the menu validator
public class MenuValidator implements OrderItemValidator {
private final MenuRepository menuRepository;
public MenuValidator(MenuRepository menuRepository) {
this.menuRepository = menuRepository;
}
@Override
public void validate(OrderItem orderItem) {
String menuId = Optional.ofNullable(orderItem.getMenuId())
.map(String::trim)
.filter(id -> !id.isEmpty())
.orElseThrow(() -> new IllegalArgumentException("A menu item must be specified."));
if (!menuRepository.menuExists(menuId)) {
throw new IllegalArgumentException("Given menu [" + menuId + "] does not exist.");
}
}
}Then the item description validator
public class ItemDescriptionValidator implements OrderItemValidator {
@Override
public void validate(OrderItem orderItem) {
Optional.ofNullable(orderItem)
.map(OrderItem::getDescription)
.map(String::trim)
.filter(description -> !description.isEmpty())
.orElseThrow(() -> new IllegalArgumentException("Item description should be provided"));
}
}Price validator
public class PriceValidator implements OrderItemValidator {
@Override
public void validate(OrderItem orderItem) {
String price = Optional.ofNullable(orderItem)
.map(OrderItem::getPrice)
.map(String::trim)
.filter(itemPrice -> !itemPrice.isEmpty())
.orElseThrow(() -> new IllegalArgumentException("Price cannot be empty."));
try {
new BigDecimal(price);
} catch (NumberFormatException ex) {
throw new IllegalArgumentException("Given price [" + price + "] is not in valid format", ex);
}
}
}And finally, the quantity validator
public class QuantityValidator implements OrderItemValidator {
@Override
public void validate(OrderItem orderItem) {
Integer quantity = Optional.ofNullable(orderItem)
.map(OrderItem::getQuantity)
.orElseThrow(() -> new IllegalArgumentException("Quantity must be given"));
if (quantity <= 0) {
throw new IllegalArgumentException("Given quantity " + quantity + " is not valid.");
}
}
}Each of these validator implementations can now be easily tested, independently from each other. Reasoning about each one of them also becomes easier. and so are future addition/modification/removal.
Now the wiring part. How can we integrate these validators with the order service?
One way would be to directly create a list in the OrderService constructor, and populate it with the validators. Or we could use Spring to inject a List into the OrderService
@Service
@Slf4j
class OrderService {
private final List<OrderItemValidator> validators;
OrderService(List<OrderItemValidator> validators) {
this.validators = validators;
}
void createOrder(OrderDTO orderDTO) {
orderDTO.getOrderItems()
.forEach(this::validate);
log.info("Order {} saved", orderDTO);
}
private void validate(OrderItem orderItem) {
validators.forEach(validator -> validator.validate(orderItem));
}
}In order for this to work we will have to declare each of the validator implementations as a Spring Bean.
We could improve our abstraction even further. The OrderService is now accepting a List of the validators. However, we can change it to be only aware of the OrderItemValidator type, and nothing else. This gives us the flexibility of injecting either a single validator or any composition of validators in the future.
So now our goal is to change the order service to treat a composition of order item validators in the same way as a single validator. There is a well-known design pattern called
Composite which lets us do exactly that.
Let’s create a new implementation of the validator interface, which will be the composite
class OrderItemValidatorComposite implements OrderItemValidator {
private final List<OrderItemValidator> validators;
OrderItemValidatorComposite(List<OrderItemValidator> validators) {
this.validators = validators;
}
@Override
public void validate(OrderItem orderItem) {
validators.forEach(validators -> validators.validate(orderItem));
}
}We then create a new Spring configuration class, which will instantiate and initialize this composite, and then expose it as a bean
@Configuration
class ValidatorConfiguration {
@Bean
OrderItemValidator orderItemValidator(MenuRepository menuRepository) {
return new OrderItemValidatorComposite(Arrays.asList(
new MenuValidator(menuRepository),
new ItemDescriptionValidator(),
new PriceValidator(),
new QuantityValidator()
));
}
}We then change the OrderService class in the following way
@Service
@Slf4j
class OrderService {
private final OrderItemValidator validator;
OrderService(OrderItemValidator orderItemValidator) {
this.validator = orderItemValidator;
}
void createOrder(OrderDTO orderDTO) {
orderDTO.getOrderItems()
.forEach(validator::validate);
log.info("Order {} saved", orderDTO);
}
}And we are done!
The benefits of this approach is many. The whole validation logic has completely been abstracted away from the ordering service. Testing is easier. Future maintenance is easier. Clients only know about one validator type, and nothing else.
However, all of the above come with some problems too. Sometimes people are not comfortable with this design. They may feel like this is just too much abstraction, or that they will not be needing this much flexibility or testability for future maintenance. I’d suggest to adopt this approach based on the team culture. After all, there is no single right way of doing things in Software Development.
Note that for the sake of this article I have taken some short cuts here as well. These includes throwing a generic IllegalArgumentException when validation fails. You’d probably want a more specific/custom exception in a production-grade application to identify between different scenarios. The decimal parsing is also done naively, you might want to fix on a specific format, and then use DecimalFormat to parse it.
The full code has been uploaded to Github.
| Reference: | Clean Code from the trenches from our JCG partner Sayem Ahmed at the Codesod blog. |
