Провести code review сервиса создания заказа

15. Провести code review сервиса создания заказа

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

Код:

@Service
public class OrderService {

    @Autowired
    public OrderRepository orderRepository;

    @Autowired
    public PaymentClient paymentClient;

    @Autowired
    public KafkaTemplate<String, String> kafkaTemplate;

    public String createOrder(CreateOrderRequest request) {

        Order order = new Order();
        order.setUserId(request.getUserId());
        order.setAmount(request.getAmount());
        order.setStatus("NEW");

        orderRepository.save(order);

        boolean paid = paymentClient.charge(
                request.getUserId(),
                request.getAmount()
        );

        if (paid) {
            order.setStatus("PAID");
            orderRepository.save(order);

            kafkaTemplate.send("orders-topic",
                    "Order paid: " + order.getId());
        } else {
            order.setStatus("FAILED");
            orderRepository.save(order);

            kafkaTemplate.send("orders-topic",
                    "Order failed: " + order.getId());
        }

        return "OK";
    }
}

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

Подсказки

💡 @Autowired на public-полях — плохая практика, лучше constructor injection.
💡 В коде есть несколько побочных эффектов: БД, внешний paymentClient, Kafka.
💡 Здесь возможна проблема dual write: запись в БД успешна, а отправка в Kafka — нет.
💡 Если paymentClient.charge(...) выполнится успешно, а обновление заказа упадёт, система станет неконсистентной.
💡 "NEW", "PAID", "FAILED" — магические строки, лучше enum.
💡 Нет валидации request.
💡 Возврат "OK" — слабый контракт метода.

Решение

В коде ок то, что логика читается довольно просто: создаём заказ, пытаемся списать деньги, обновляем статус, отправляем событие. Для маленького примера это понятно 👍

Но проблем здесь много.

Field injection и ещё public-поля ❌

@Autowired
public OrderRepository orderRepository;

зависимости не должны быть public
field injection хуже тестируется
лучше сделать private final и конструкторную инъекцию

Нет валидации входного запроса ⚠️

Нужно проверить хотя бы:

request != null
userId != null
amount != null
amount > 0

Магические строки статусов

order.setStatus("NEW");

Лучше использовать enum, например OrderStatus.NEW, PAID, FAILED.

Смешано слишком много ответственности

Сервис одновременно:

создаёт заказ
вызывает внешний платёжный сервис
обновляет статус
отправляет событие в Kafka

Это уже нарушение SRP.

Риск неконсистентности данных

Сценарий:

заказ сохранили как NEW
paymentClient.charge(...) успешно списал деньги
затем orderRepository.save(order) со статусом PAID упал

Тогда деньги списаны, а в БД заказ может остаться в статусе NEW ❌

Проблема с Kafka (dual write)

Сценарий:

статус в БД обновили
kafkaTemplate.send(...) не сработал

Тогда в БД одно состояние, а событие наружу не ушло.
Обычно такое решают через Transactional Outbox.

Нет обработки ошибок внешнего вызова

Если paymentClient.charge(...) бросит exception:

заказ уже сохранён как NEW
метод аварийно завершится
статус не будет обновлён

Нужно явно продумать, какой статус ставить в таком случае, например PAYMENT_ERROR.

Нет идемпотентности

Если клиент повторно вызовет createOrder из-за retry, можно создать дубль заказа и повторно списать деньги 💥

Возврат "OK" — плохой контракт

Лучше вернуть:

orderId
итоговый статус
DTO ответа

например CreateOrderResponse.

Исправленный вариант на базовом уровне может выглядеть так:

@Service
@RequiredArgsConstructor
public class OrderService {

    private final OrderRepository orderRepository;
    private final PaymentClient paymentClient;
    private final KafkaTemplate<String, String> kafkaTemplate;

    @Transactional
    public CreateOrderResponse createOrder(CreateOrderRequest request) {
        validate(request);

        Order order = new Order();
        order.setUserId(request.getUserId());
        order.setAmount(request.getAmount());
        order.setStatus(OrderStatus.NEW);

        orderRepository.save(order);

        boolean paid = paymentClient.charge(request.getUserId(), request.getAmount());

        if (paid) {
            order.setStatus(OrderStatus.PAID);
            kafkaTemplate.send("orders-topic", "Order paid: " + order.getId());
        } else {
            order.setStatus(OrderStatus.FAILED);
            kafkaTemplate.send("orders-topic", "Order failed: " + order.getId());
        }

        orderRepository.save(order);

        return new CreateOrderResponse(order.getId(), order.getStatus());
    }

    private void validate(CreateOrderRequest request) {
        if (request == null || request.getUserId() == null || request.getAmount() == null) {
            throw new IllegalArgumentException("Invalid request");
        }
        if (request.getAmount().signum() <= 0) {
            throw new IllegalArgumentException("Amount must be positive");
        }
    }
}

Но даже этот вариант не идеален, потому что внешний вызов paymentClient и Kafka всё ещё остаются рядом с транзакцией.

Более правильная архитектура такая:

OrderService создаёт заказ
отдельный компонент отвечает за оплату
событие в Kafka отправляется через outbox
статусы и переходы оформлены явно
платёжный вызов делается идемпотентно

Что я бы сказал на собеседовании 🎯

Основные проблемы здесь — field injection, отсутствие валидации, магические строки статусов, смешение ответственности и риск неконсистентности между БД, платёжным сервисом и Kafka.
Я бы перевёл сервис на constructor injection, ввёл enum статусов, нормальный response DTO, вынес платёжную логику и отправку событий, а для Kafka использовал outbox, чтобы избежать dual write.