Исправить ошибки в OrderService

23. Исправить ошибки в OrderService

Условие задачи:
Дан сервис изменения статуса заказа. Нужно провести code review и исправить ошибки: корректно получать и сохранять заказ, обрабатывать входные данные и статусы, а также правильно отправлять уведомления при завершении или отмене заказа.

Код:

public class OrderService {

    @Autowired
    private OrderRepository orderRepository;

    private final NotificationService notificationService;

    @Autowired
    public OrderService(NotificationService notificationService) {
        this.notificationService = notificationService;
    }

    public void updateOrderStatus(Long orderId, String newStatus) {
        Order order = orderRepository.findById(orderId);

        if (newStatus.equals(""COMPLETED"")) {
            order.setStatus(""COMPLETED"");
            notificationService.notify(
                    order.getUserId(),
                    ""Your order is completed""
            );
        } else if (newStatus.equals(""CANCELLED"")) {
            order.setStatus(""CANCELLED"");
            notificationService.notify(
                    order.getUserId(),
                    ""Your order is cancelled""
            );
        } else if (newStatus.equals(""PENDING"")) {
            order.setStatus(""PENDING"");
            orderRepository.save(order);
        } else if (newStatus.equals(""IN_PROGRESS"")) {
            order.setStatus(""IN_PROGRESS"");
            orderRepository.save(order);
        } else {
            throw new IllegalArgumentException(
                    ""Unsupported status: "" + newStatus
            );
        }
    }
}

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

Подсказки

💡 Используй один способ Dependency Injection.
💡 Проверь результат findById().
💡 Статусы лучше представить через enum.
💡 В некоторых ветках изменённый заказ вообще не сохраняется.
💡 Уведомление желательно отправлять только после успешного изменения заказа.

Решение

Проблемы:

смешаны field injection и constructor injection;
если OrderRepository — Spring Data repository, findById() возвращает Optional<Order>;
нет проверки orderId и newStatus;
при newStatus == null возникнет NullPointerException;
статусы представлены строками;
для COMPLETED и CANCELLED заказ не сохраняется;
логика сохранения дублируется по веткам;
нет транзакции;
уведомление отправляется до гарантированного commit;
не проверяются допустимые переходы статусов, например CANCELLED → COMPLETED;
если класс не зарегистрирован другим способом, ему нужен @Service.

Как лучше исправить

Сделать constructor injection, преобразовать строку в enum, один раз изменить и сохранить статус:

@Service
@RequiredArgsConstructor
public class OrderService {

    private final OrderRepository orderRepository;
    private final NotificationService notificationService;

    @Transactional
    public void updateOrderStatus(
            Long orderId,
            String newStatus
    ) {
        if (orderId == null) {
            throw new IllegalArgumentException(
                    "orderId must not be null"
            );
        }

        if (newStatus == null || newStatus.isBlank()) {
            throw new IllegalArgumentException(
                    "newStatus must not be blank"
            );
        }

        Order order = orderRepository.findById(orderId)
                .orElseThrow(() ->
                        new EntityNotFoundException(
                                "Order not found: " + orderId
                        )
                );

        OrderStatus status;

        try {
            status = OrderStatus.valueOf(
                    newStatus.trim().toUpperCase(Locale.ROOT)
            );
        } catch (IllegalArgumentException e) {
            throw new IllegalArgumentException(
                    "Unsupported status: " + newStatus
            );
        }

        if (order.getStatus() == status) {
            return;
        }

        order.setStatus(status);
        orderRepository.save(order);

        if (status == OrderStatus.COMPLETED) {
            notificationService.notify(
                    order.getUserId(),
                    "Your order is completed"
            );
        } else if (status == OrderStatus.CANCELLED) {
            notificationService.notify(
                    order.getUserId(),
                    "Your order is cancelled"
            );
        }
    }
}

public enum OrderStatus {
    PENDING,
    IN_PROGRESS,
    COMPLETED,
    CANCELLED
}

В реальном сервисе уведомление лучше отправлять после успешного commit, например через @TransactionalEventListener(AFTER_COMMIT) или outbox.

Если у заказа есть бизнес-правила переходов между статусами, их также стоит проверять отдельно, а не разрешать произвольную замену статуса.