Сделать рефакторинг сервиса создания заказа

15. Сделать рефакторинг сервиса создания заказа

Условие задачи:
Дан OrderService, который создаёт заказ, проверяет наличие товаров, уменьшает остатки и применяет промокод.

Необходимо исправить ошибки, улучшить валидацию, читаемость и корректность работы при конкурентных заказах.

Код:

@Service
public class OrderService {

    @Autowired
    private OrderRepository orderRepository;

    @Autowired
    private ProductRepository productRepository;

    public Order createOrder(String userId, List<Product> products, String promoCode) {
        if (products.size() > 0) {
            double total = 0;

            for (int i = 0; i <= products.size(); i++) {
                Integer stock = productRepository.findStockById(products.get(i).getId());

                if (stock == null || stock <= 0) {
                    throw new IllegalStateException(
                            "Product out of stock: " + products.getId()
                    );
                }
            }

            for (int i = 0; i <= products.size(); i++) {
                total += products.get(i).getPrice();
                productRepository.updateStockById(products.get(i).getId(), 1);
            }

            if (promoCode == "WELCOME10") {
                total = total * 0.9;
            }

            Order order = new Order();
            order.setUserId(userId);
            order.setTotal(total);
            order.setStatus("new");

            System.out.println("Created order for user: " + userId);

            return orderRepository.save(order);
        }

        return null;
    }
}

Спойлеры к решению

Подсказки

💡 i <= products.size() приводит к IndexOutOfBoundsException.
💡 products.getId() не компилируется.
💡 Строки нельзя сравнивать через ==.
💡 Нужна проверка userId, products и элементов списка.
💡 Для денег лучше BigDecimal.
💡 Проверка остатка и его последующее изменение должны быть атомарными.

Решение

Основные проблемы:

field injection вместо constructor injection;
products == null не проверяется;
условие i <= products.size() ошибочно;
products.getId() не существует;
promoCode == "WELCOME10" сравнивает ссылки;
return null — плохой контракт;
double нежелателен для денег;
"new" лучше заменить на enum;
System.out.println() лучше заменить логгером;
нет транзакции;
схема SELECT stock → UPDATE stock допускает overselling при параллельных заказах;
если Product пришёл от клиента, нельзя доверять его price — цену следует получать из БД.

Для остатков лучше сделать атомарное обновление:

public interface ProductRepository
        extends JpaRepository<Product, Long> {

    @Modifying
    @Query("""
            update Product p
               set p.stock = p.stock - 1
             where p.id = :productId
               and p.stock > 0
            """)
    int decreaseStock(@Param("productId") Long productId);
}

Сервис:

@Service
public class OrderService {

    private static final String WELCOME10 = "WELCOME10";
    private static final BigDecimal DISCOUNT =
            new BigDecimal("0.90");

    private final OrderRepository orderRepository;
    private final ProductRepository productRepository;

    public OrderService(
            OrderRepository orderRepository,
            ProductRepository productRepository
    ) {
        this.orderRepository = orderRepository;
        this.productRepository = productRepository;
    }

    @Transactional
    public Order createOrder(
            String userId,
            List<Product> products,
            String promoCode
    ) {
        validate(userId, products);

        BigDecimal total = BigDecimal.ZERO;

        for (Product requestedProduct : products) {
            Long productId = requestedProduct.getId();

            Product product = productRepository.findById(productId)
                    .orElseThrow(() ->
                            new IllegalArgumentException(
                                    "Product not found: " + productId
                            )
                    );

            if (productRepository.decreaseStock(productId) != 1) {
                throw new IllegalStateException(
                        "Product out of stock: " + productId
                );
            }

            total = total.add(product.getPrice());
        }

        if (WELCOME10.equals(promoCode)) {
            total = total.multiply(DISCOUNT);
        }

        Order order = new Order();
        order.setUserId(userId);
        order.setTotal(total);
        order.setStatus(OrderStatus.NEW);

        return orderRepository.save(order);
    }

    private void validate(
            String userId,
            List<Product> products
    ) {
        if (userId == null || userId.isBlank()) {
            throw new IllegalArgumentException(
                    "userId must not be blank"
            );
        }

        if (products == null || products.isEmpty()) {
            throw new IllegalArgumentException(
                    "products must not be empty"
            );
        }

        if (products.stream().anyMatch(
                p -> p == null || p.getId() == null
        )) {
            throw new IllegalArgumentException(
                    "product id must not be null"
            );
        }
    }
}

@Transactional здесь нужен, чтобы уменьшение остатков всех товаров и сохранение заказа были одной операцией: если один товар закончился или save() упал, предыдущие изменения остатков откатятся.

При этом сама конкурентная корректность обеспечивается не @Transactional, а условным атомарным:

UPDATE product
SET stock = stock - 1
WHERE id = ?
  AND stock > 0

Это не позволяет двум параллельным заказам списать один последний экземпляр товара.