Skip to main content

Anti-Patterns

A layer violation works at the moment you write it. The tests pass too. The trouble surfaces later, when you want to swap an implementation, when you want to call the same processing from a job, or when the table definition changes.

What follows are the ten implementations that come up repeatedly in review. Each differs from the correct version by a line or two, which is exactly what makes them hard to see for the person who wrote them.

Calling a DAO Straight from an Endpoint​

// Avoid
@Path("/api/foo/order/{orderId}")
@GET
public OrderEntity get(@Variable(name = "orderId") final String orderId) {
    final OrderDAO dao = DAOFactory.getTenantDatabaseDAO(OrderDAO.class);
    return dao.find(orderId);
}
// Prefer
@Path("/api/foo/order/{orderId}")
@GET
public OrderResponse get(@Variable(name = "orderId") final String orderId) throws BusinessErrorException {
    try {
        return useCase.execute(orderId);
    } catch (final OrderAppException e) {
        throw new BusinessErrorException(e.getMessage(), e);
    }
}

Referencing the infrastructure layer straight from the presentation layer skips the business rules that the two layers in between were carrying. Neither the business validity checks the service performs nor the conversion into a domain model take effect unless they are passed through.

Returning an Entity as the API Response​

// Avoid
public OrderEntity findOrder(final String orderId) {
    return dao.find(orderId);
}
// Prefer
public OrderResponse findOrder(final String orderId) throws OrderServiceException {
    final Order order = orderService.findByOrderId(orderId);
    return OrderResponse.fromDomainModel(order);
}

An entity is a class that mirrors the shape of a table. Send it outside and the internal column names and audit fields become visible to the client. Worse, the compatibility of the API breaks the moment the table definition changes.

Writing Business Rules in a Repository​

// Avoid
public class StandardOrderRepository implements OrderRepository {

    @Override
    public void save(final Order order) throws RepositoryException {
        if (order.getAmount().compareTo(BigDecimal.ZERO) < 0) {
            throw new RepositoryException("金額が不正です");
        }
        // Persistence
    }
}
// Prefer
public class StandardOrderService implements OrderService {

    @Override
    public void save(final Order order) throws OrderServiceException {
        if (order.getAmount().compareTo(BigDecimal.ZERO) < 0) {
            throw new OrderServiceException("金額は0以上である必要があります: amount=" + order.getAmount());
        }
        orderRepository.save(order);
    }
}

What a repository is responsible for is data access and the conversion between entities and domain models. Business rules written there disappear along with the repository the day it is swapped out.

The Domain Layer Referencing the Infrastructure Layer​

// Avoid
package jp.co.example.foo.domain.service;

import jp.co.intra_mart.mirage.ext.dao.DAOFactory;
import jp.co.intra_mart.mirage.ext.session.SessionTemplate;

public class StandardOrderService {

    public Order findByOrderId(final String orderId) {
        final OrderDAO dao = DAOFactory.getTenantDatabaseDAO(OrderDAO.class);
        // ...
    }
}
// Prefer
package jp.co.example.foo.infrastructure.service;

import jp.co.example.foo.domain.repository.OrderRepository;

public class StandardOrderService implements OrderService {

    private final OrderRepository orderRepository;
    // ...
}

At review time the import statements alone settle it. If jp.co.intra_mart.mirage.* shows up in a domain-layer class, its dependency already points outward. Put the implementation class in the infrastructure layer and leave only the interface in the domain layer.

Swallowing Exceptions​

// Avoid
try {
    orderRepository.save(order);
} catch (final RepositoryException e) {
    // Nothing
}
// Prefer
try {
    orderRepository.save(order);
} catch (final RepositoryException e) {
    LOGGER.error("Failed to save order: orderId=" + order.getOrderId(), e);
    throw new OrderServiceException("発注の保存に失敗しました", e);
}

Swallowing it leaves processing to continue with the update only partially applied. The caller reads that as success, so the inconsistency surfaces in the next operation, or in the following day's aggregation.

Putting the Transaction Boundary in an Endpoint​

// Avoid
@Path("/api/foo/order")
@POST
public void create(@Body final OrderRequest request) {
    SessionTemplate.execute(new SessionCallback<Void, RuntimeException>() {
        @Override
        public Void execute(final Session session) {
            orderService.register(request);
            return null;
        }
    });
}
// Prefer
@Override
public void register(final Order order) throws OrderServiceException {
    SessionTemplate.execute(s -> {
        try {
            orderRepository.save(order);
        } catch (final RepositoryException e) {
            throw new OrderServiceException("発注の登録に失敗しました: orderId=" + order.getOrderId(), e);
        }
        return null;
    });
}

The boundary is established by the service and repository implementations. Moving the boundary outward spreads the dependency on SessionTemplate to the presentation layer. Hiding the repository behind an interface stops meaning anything, and the boundary has to be rewritten when the same processing is called from a job.

Writing Business Exception Messages in English​

// Avoid
throw new OrderServiceException("Order not found: id=" + orderId);
// Prefer
throw new OrderServiceException("発注が見つかりません: orderId=" + orderId);

The basis for the split is who reads them. Business exception messages reach operators and users, so write them in Japanese with the variable values needed to identify the cause. Log messages, read only by developers, may stay in English.

Using new Instead of a Factory​

// Avoid
public class GetOrderUseCase {

    private final OrderService orderService = new StandardOrderService();
}
// Prefer
public class GetOrderUseCase {

    private final OrderService orderService;

    public GetOrderUseCase() {
        try {
            this.orderService = OrderServiceFactory.getInstance().getOrderService();
        } catch (final OrderServiceException e) {
            throw new RuntimeException("OrderService の初期化に失敗しました", e);
        }
    }
}

Writing new pins the caller to the default implementation. Neither swapping through ServiceLoaderUtil nor injecting a mock in tests takes effect any more.

Logging Input Validation Errors​

// Avoid
final List<String> errors = GetOrderValidator.validate(orderId);
if (!errors.isEmpty()) {
    LOGGER.error("Validation failed: " + errors);
    throw new ValidationException(errors);
}
// Prefer
final List<String> errors = GetOrderValidator.validate(orderId);
if (!errors.isEmpty()) {
    throw new ValidationException(errors);
}

Malformed input is the result of what a user did, not a system failure. Recording it as ERROR buries the failures that actually need attention. Authorization errors are left unrecorded for the same reason.

Setting Audit Fields by Hand​

// Avoid
entity.createUserCd = "admin";
entity.createDate = new Timestamp(System.currentTimeMillis());
entity.recordUserCd = "admin";
entity.recordDate = new Timestamp(System.currentTimeMillis());
dao.insert(entity);
// Prefer
dao.insert(entity);

On AbstractDAO, insert sets the four audit fields automatically, and update sets the two fields that record who updated the row and when. Setting them by hand records values unrelated to the logged-in user, which makes the update history impossible to follow. How audit fields are handled is covered in Creating Entities and DAOs.