Провести code review сервиса конвертации валюты

42. Провести code review сервиса конвертации валюты

Условие задачи:
Дан Spring-сервис, который конвертирует сумму из рублей в другую валюту, списывает деньги с рублёвого счёта, зачисляет их на валютный счёт и отправляет уведомление.

Необходимо провести code review, обратить внимание на стилистику, corner cases, дизайн классов и SOLID.

Код:

@Component
@RequiredArgsConstructor
public class Converter {

    private final ConversationService cs;
    private final AccountRepository accountRepository;
    private final NotificationService notificationService;

    /**
     * Конвертирует sumInRub в валюту currencyCode
     * по курсу на текущий момент и переводит
     * с рублевого счета на валютный
     *
     * @param sumInRub сумма перевода в рублях
     * @param currencyCode валюта перевода
     * @param accountNumberRub номер рублевого счета
     * @param accountNumber номер валютного счета
     */
    @Transactional
    public void convertFromRubbles(
            double sumInRub,
            String currencyCode,
            String accountNumberRub,
            String accountNumberCurr
    ) {
        Double rate = cs.getRate("RUB", currencyCode);
        Double convertedSum = sumInRub * rate;

        accountRepository.decrease(
                accountNumberRub,
                sumInRub
        );

        accountRepository.increase(
                accountNumberCurr,
                convertedSum
        );

        notificationService.send("Перевод успешен");
    }
}

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

Подсказки

💡 Для денежных значений лучше использовать BigDecimal.
💡 Проверь входные параметры, существование счетов и достаточность средств.
💡 Подумай, стоит ли выполнять внешний вызов курса внутри DB-транзакции.
💡 Уведомление отправляется ещё до успешного commit.
💡 При конкурентных переводах уменьшение баланса должно выполняться атомарно.

Решение

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

для денег и курса используются double / Double;
нет проверки sumInRub > 0;
не проверяются currencyCode и номера счетов;
не обрабатывается отсутствие курса;
не проверяется наличие достаточного остатка;
метод получения курса выполняется внутри открытой транзакции;
уведомление отправляется до успешного commit;
ошибка notificationService может откатить перевод, хотя уведомление не является частью изменения баланса;
возможна race condition при одновременных списаниях;
неясно, что означает rate: умножать сумму на курс или делить — это должен определять контракт getRate();
название convertFromRubbles() содержит опечатку, корректнее convertFromRubles();
имя cs неинформативно;
в JavaDoc указан accountNumber, а параметр называется accountNumberCurr;
класс Converter одновременно получает курс, выполняет перевод и отправляет уведомление.

Для денежных операций лучше использовать BigDecimal:

BigDecimal convertedAmount =
        amount.multiply(rate)
                .setScale(2, RoundingMode.HALF_UP);

Базовый вариант сервиса:

@Service
@RequiredArgsConstructor
public class CurrencyConversionService {

    private final ConversationService conversionService;
    private final AccountRepository accountRepository;

    @Transactional
    public void convertFromRubles(
            BigDecimal amount,
            String currencyCode,
            String rubAccount,
            String currencyAccount
    ) {
        if (amount == null
                || amount.compareTo(BigDecimal.ZERO) <= 0) {
            throw new IllegalArgumentException(
                    "Amount must be positive"
            );
        }

        BigDecimal rate =
                conversionService.getRate(
                        "RUB",
                        currencyCode
                );

        BigDecimal convertedAmount =
                amount.multiply(rate)
                        .setScale(
                                2,
                                RoundingMode.HALF_UP
                        );

        int decreased =
                accountRepository.decreaseIfEnough(
                        rubAccount,
                        amount
                );

        if (decreased != 1) {
            throw new IllegalStateException(
                    "Insufficient funds"
            );
        }

        accountRepository.increase(
                currencyAccount,
                convertedAmount
        );
    }
}

Списание желательно выполнять одной атомарной операцией на уровне БД:

UPDATE account
SET balance = balance - :amount
WHERE account_number = :accountNumber
  AND balance >= :amount;

Если изменена одна строка — средств хватило. Если 0 строк — счёт отсутствует или денег недостаточно.

Также получение курса желательно выполнять до короткой DB-транзакции, если ConversationService обращается во внешний сервис.

Уведомление:

notificationService.send("Перевод успешен");

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

Главные исправления: BigDecimal, валидация, атомарное списание, корректная граница транзакции и отправка уведомления только после успешного перевода.