Провести code review OrderService на Kotlin

43. Провести code review OrderService на Kotlin

Условие задачи:
Дан сервис создания и отмены заказов на Kotlin.

Необходимо провести code review: найти проблемы в работе с null, коллекциями, денежными значениями, Singleton, корутинами, SQL и дизайном класса.

Код:

class OrderService {

    companion object {
        var INSTANCE: OrderService? = null

        fun getInstance() =
            INSTANCE ?: OrderService().also {
                INSTANCE = it
            }
    }

    public var orders: MutableList<Order> =
        mutableListOf()

    fun createOrder(
        userId: String,
        items: List<Item>?,
        promo: String
    ): Order {

        val order = Order(
            UUID.randomUUID().toString(),
            userId
        )

        var total = 0.0

        for (i in 0 ..< items!!.size - 1) {
            total += items[i].price * items[i].quantity
        }

        if (promo != null && promo != "") {
            if (promo.equals("SALE10")) {
                total = total - total * 0.1
            } else if (promo.equals("SALE20")) {
                total = total - total * 0.2
            }
        }

        order.total = total
        orders.add(order)

        GlobalScope.launch {
            Thread.sleep(1000)

            DB.exec(
                "INSERT INTO orders VALUES " +
                        "('${order.id}', ${order.total})"
            )
        }

        return order
    }

    fun cancel(id: String) {
        orders.remove(
            orders.find { it.id.equals(id) }
        )
    }
}

class Order(
    var id: String,
    var userId: String
) {
    var total: Double = 0.0
}

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

Подсказки

💡 items!! может привести к NullPointerException.
💡 Проверь границы цикла и обрабатывается ли последний элемент списка.
💡 Double не подходит для денежных расчётов.
💡 GlobalScope и Thread.sleep() плохо сочетаются с корутинами.
💡 Singleton через изменяемое поле INSTANCE не является потокобезопасным.

Решение

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

Singleton через INSTANCE не потокобезопасен;
публичный MutableList можно изменить извне;
items!! может привести к NullPointerException;
цикл не обрабатывает последний элемент;
promo объявлен как non-nullable, поэтому promo != null бессмысленно;
для денег используется Double;
для сравнения строк в Kotlin лучше использовать ==;
GlobalScope создаёт неконтролируемую корутину;
Thread.sleep() блокирует поток;
метод возвращает заказ до завершения записи в БД;
ошибки асинхронной записи никак не обрабатываются;
SQL строится через интерполяцию строки;
OrderService одновременно считает цену, хранит заказы и работает с БД;
cancel() некорректно работает, если заказ не найден.

Цикл лучше заменить обычным обходом элементов:

for (item in items) {
    total += item.price * item.quantity
}

Для денег использовать BigDecimal, а скидку определить через when:

fun createOrder(
    userId: String,
    items: List<Item>,
    promo: String?
): Order {

    require(userId.isNotBlank())
    require(items.isNotEmpty())

    var total = items.fold(BigDecimal.ZERO) { sum, item ->
        sum + item.price.multiply(
            item.quantity.toBigDecimal()
        )
    }

    total = when (promo) {
        "SALE10" -> total.multiply(BigDecimal("0.90"))
        "SALE20" -> total.multiply(BigDecimal("0.80"))
        else -> total
    }

    return Order(
        id = UUID.randomUUID().toString(),
        userId = userId,
        total = total
    )
}

Модель:

data class Order(
    val id: String,
    val userId: String,
    val total: BigDecimal
)

Отмену можно реализовать так:

fun cancel(id: String): Boolean {
    return orders.removeIf { it.id == id }
}

Вместо самописного Singleton:

companion object {
    var INSTANCE: OrderService? = null
}

в обычном Kotlin можно использовать object, а если класс управляется Spring — вообще не создавать Singleton вручную.

GlobalScope.launch также лучше убрать. Сохранение заказа должно выполняться через отдельный repository, а SQL — через параметры, а не конкатенацию строк.

Главные проблемы исходного кода — items!!, ошибка в диапазоне цикла, Double для денег, небезопасный Singleton, публичное изменяемое состояние и неконтролируемая асинхронная запись в БД.