Ревью Kotlin-кода: null-safety и идиомы

Ревью UserProfileRepository

Что делает код

Репозиторий отдаёт профиль пользователя. Сначала он ищет его в Room. Если там пусто, берёт профиль из сети, в фоне сохраняет в базу, оставляет только оплаченные заказы, считает сумму покупок и кладёт результат в поле-кеш. Кроме того, он умеет обновить запись в базе из сети и определить premium-статус по порогу суммы.

Замысел читается без пояснений. Этапы «кеш → сеть → маппинг → сохранение» хорошо видны, так что с этим всё в порядке.

🔴 Критично: баги и краши

К1. Блокирующий сетевой вызов внутри suspend без смены диспетчера

fetchUserSync внутри вызывает Call.execute(), а это блокирующий вызов. viewModelScope по умолчанию работает на Dispatchers.Main.immediate, поэтому при открытии экрана будет NetworkOnMainThreadException. Если StrictMode выключен, вместо исключения получите фриз UI. Модификатор suspend сам по себе не уводит код с главного потока.

Вторая проблема: execute() не реагирует на отмену корутины. Пользователь ушёл с экрана, а запрос продолжает висеть.

Было:

val response = api.fetchUserSync(userId) // синхронный вызов Retrofit-клиента

Стало. Вариант А, если обёртку можно заставить отдавать Call<UserDto>. Этот вариант предпочтительнее:

import retrofit2.awaitResponse

private suspend fun fetchUser(userId: String): UserDto {
    val response = api.fetchUserCall(userId).awaitResponse() // отменяет Call при отмене корутины
    return response.requireBody()
}

Вариант Б, если обёртку пока трогать нельзя:

private suspend fun fetchUser(userId: String): UserDto {
    val response = runInterruptible(ioDispatcher) { api.fetchUserSync(userId) }
    return response.requireBody()
}

Почему лучше: сеть уходит с главного потока, а отмена корутины действительно прерывает запрос. В варианте Б это происходит через interrupt потока, OkHttp на него реагирует.

ioDispatcher я предлагаю инжектить через Hilt-квалификатор, а не хардкодить Dispatchers.IO. Тогда в тестах его можно подменить на StandardTestDispatcher.

❓ Вопрос: dao.getUser и dao.insert объявлены как suspend? Если нет, Room бросит IllegalStateException при обращении с главного потока, и их тоже нужно увести на ioDispatcher.

К2. response.body()!!: главный источник крашей, которого вы боитесь

body() возвращает null сразу в нескольких случаях:

• любой не-2xx ответ. На 401, 404, 500 тело лежит в errorBody(), а body() равен null;
• ответ 204 No Content;
• execute() может ещё и бросить IOException (нет сети, таймаут). Он пролетит мимо !! и уронит корутину в ViewModel, если там нет try/catch.

Отдельный риск связан с тем, что обёртка написана на Java. Её возвращаемый тип для Kotlin является platform type Response<UserDto>!. Компилятор не заставит вас проверять null. Если старая Java-обёртка в духе того времени глотает исключения и возвращает null, NPE вылетит уже на .body().

Было:

val response = api.fetchUserSync(userId)
val dto = response.body()!!

Стало:

/** Бросается, когда сервер вернул 2xx, но данные непригодны. Наследуем IOException — это сетевая ошибка по смыслу. */
class MalformedResponseException(message: String) : IOException(message)

private fun <T> Response<T>.requireBody(): T {
    if (!isSuccessful) throw HttpException(this)               // retrofit2.HttpException, несёт code()
    return body() ?: throw MalformedResponseException("Empty body, HTTP ${code()}")
}

Если обёртка на Java может вернуть null, зафиксируйте это явным типом на границе:

val response: Response<UserDto>? = api.fetchUserSync(userId)
    ?: throw IOException("fetchUserSync returned null for $userId")

Почему лучше: вместо безымянного NPE получается типизированная ошибка с кодом ответа. ViewModel может её поймать и показать пользователю, а не падать.

К3. dto.displayName!! и dto.orders!!: краш на частичном ответе

Бэкенд может не прислать поле. Причины бывают разные: новая версия API, удалённый аккаунт, баг сервера. Во всех этих случаях !! роняет приложение.

Отдельно про dto.id. Судя по коду, поле non-null, но если у вас Gson, это ничего не гарантирует. Gson создаёт объекты через Unsafe, обходит конструктор и спокойно кладёт null в non-null поле Kotlin. Упадёт это позже, в конструкторе UserProfile, с сообщением «Parameter specified as non-null is null», и искать причину будет неудобно.

❓ Вопрос: какой конвертер у Retrofit: Gson, Moshi или kotlinx.serialization? И как объявлены поля UserDto? Если Gson, то nullable-поля в DTO — это правильное решение, и валидировать их нужно при маппинге, как ниже. С Moshi codegen или kotlinx.serialization non-null поля честно проверяются при парсинге.

Было:

val avatarUrl: String? = dto.avatarUrl
val displayName = dto.displayName!!
// ...
for (o in dto.orders!!) {

Стало. Вся валидация собрана в одном месте, на границе DTO → domain:

private fun UserDto.toProfile(): UserProfile {
    val profileId = id ?: throw MalformedResponseException("id is null")
    val name = displayName
        ?: throw MalformedResponseException("displayName is null for user $profileId")
    val paidOrders = orders.orEmpty()
        .filter { it.status == ORDER_STATUS_PAID }
        .map { it.toDomain() }

    return UserProfile(
        id = profileId,
        name = name,
        avatarUrl = avatarUrl,
        orders = paidOrders,
        totalSpent = paidOrders.sumOf { it.amount },
    )
}

Почему лучше: для каждого поля принято осознанное решение, а сообщение об ошибке сразу говорит, какое поле пришло пустым.

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

• avatarUrl опционален по смыслу, поэтому String? в домене правильно. Лишнюю промежуточную переменную я убрал.
• displayName: я сделал его обязательным. Если продукт допускает фолбэк, можно написать displayName ?: DEFAULT_NAME и не показывать ошибку.
• orders: orEmpty() удобен, но у него есть подвох. Если null означает «сервер не смог загрузить заказы», а не «заказов нет», пользователь молча потеряет premium-статус. Если это ваш случай, тоже бросайте MalformedResponseException.

requireNotNull(x) { "..." } здесь тоже подошёл бы. Я выбрал своё исключение, потому что requireNotNull бросает IllegalArgumentException, а это сигнал «ошибка программиста». Кривой ответ сервера — это ошибка ввода-вывода, и её удобно ловить вместе с остальными сетевыми через catch (e: IOException).

Обратите внимание на порядок операций. В оригинале DTO сохраняется в базу до проверки orders!!. Если маппинг упадёт, в Room останется битая запись, и при следующем открытии экрана она придёт из кеша. В исправленной версии сначала маппинг, потом сохранение.

К4. externalScope.launch без обработки ошибок

Сначала хорошая новость: сам паттерн правильный. Google рекомендует именно его для работы, которая должна пережить экран. Утечки здесь нет, если externalScope является application-scope синглтоном.

Проблема в другом. Исключение внутри launch (например, SQLiteFullException, SQLiteConstraintException, ошибка в toEntity()) никто не ловит. Что будет дальше, зависит от того, как собран scope:

• без CoroutineExceptionHandler исключение уйдёт в uncaught handler потока, и упадёт всё приложение. Причём не в том месте, где вы ожидаете;
• без SupervisorJob одно падение отменит весь scope, и все следующие launch в нём молча не будут выполняться. Вот это по ощущениям и похоже на «утечку»: корутины бесследно пропадают.

Было:

externalScope.launch {
    dao.insert(dto.toEntity())
}

Стало:

private fun saveInBackground(dto: UserDto) {
    externalScope.launch {
        try {
            dao.insert(dto.toEntity())
        } catch (e: SQLiteException) {
            Log.e(TAG, "Failed to cache user ${dto.id}", e) // кеш не критичен — не роняем приложение
        }
    }
}

Сам scope в Hilt-модуле:

@Provides
@Singleton
@ApplicationScope
fun provideApplicationScope(): CoroutineScope = CoroutineScope(
    SupervisorJob() + Dispatchers.Default +
        CoroutineExceptionHandler { _, e -> Log.e("AppScope", "Unhandled", e) }
)

Почему лучше: сбой записи в кеш остаётся локальной проблемой и не роняет приложение и соседние фоновые задачи.

❓ Вопрос: как сейчас провайдится externalScope? Если это CoroutineScope(Dispatchers.IO) без SupervisorJob, то это ровно описанный выше сценарий. Если scope создаётся на каждый экземпляр репозитория и нигде не отменяется, тогда это настоящая утечка. Покажите модуль, и я скажу точно.

К5. refreshInBackground глотает CancellationException

catch (e: Exception) ловит и CancellationException. Когда пользователь уходит с экрана во время pull-to-refresh, отмена корутины будет:

1. записана в лог как «refresh failed», то есть превратится в ложные ошибки в логах и крашлитике;
2. проглочена, из-за чего нарушится structured concurrency. Родитель не узнает, что корутина отменилась, и код после withContext продолжит выполняться.

Есть и ещё две проблемы. Первая: execute() внутри withContext(IO) по-прежнему нельзя отменить (см. К1). Вторая: метод ничего не возвращает, поэтому ViewModel не знает, что обновление не удалось, и не может показать ошибку и корректно спрятать индикатор.

Было:

suspend fun refreshInBackground(userId: String) {
    withContext(Dispatchers.IO) {
        try {
            val fresh = api.fetchUserSync(userId).body()!!
            dao.insert(fresh.toEntity())
        } catch (e: Exception) {
            Log.e("UserProfileRepository", "refresh failed", e)
        }
    }
}

Стало:

/** @return true, если данные обновлены; false — при сетевой ошибке или кривом ответе. */
suspend fun refresh(userId: String): Boolean = try {
    val dto = fetchUser(userId)
    val profile = dto.toProfile()
    dao.insert(dto.toEntity())
    cachedProfile = profile
    true
} catch (e: IOException) {          // нет сети, таймаут, MalformedResponseException
    Log.w(TAG, "Refresh failed for $userId", e)
    false
} catch (e: HttpException) {        // 4xx/5xx
    Log.w(TAG, "Refresh failed for $userId: HTTP ${e.code()}", e)
    false
}

Почему лучше: ловим только ожидаемые ошибки, поэтому отмена проходит насквозь без ручного rethrow, а вызывающий код знает результат.

Ещё несколько деталей:

• withContext(Dispatchers.IO) больше не нужен: смену диспетчера инкапсулирует fetchUser, а suspend-DAO Room сам уводит на свой пул.
• Я переименовал метод в refresh. Суффикс InBackground у suspend-функции вводит в заблуждение: вызывающий код ждёт её завершения.
• Если вам всё же нужен широкий catch (e: Exception), первой веткой обязательно ставьте catch (e: CancellationException) { throw e }. По той же причине не используйте runCatching в корутинах: он тоже глотает отмену.

🟡 Важно: надёжность и поддерживаемость

В1. Pull-to-refresh и два пути получения профиля, возможно, расходятся

У loadProfile есть две особенности:

• если запись в Room есть, сеть никогда не вызывается. Если pull-to-refresh вызывает loadProfile, пользователь навсегда видит старые данные;
• refreshInBackground обновляет базу, но не cachedProfile и ничего не возвращает. После него ViewModel должна отдельно вызвать loadProfile.

Кроме того, в сетевой ветке фильтрация по PAID и подсчёт totalSpent делаются в репозитории, а в локальной ветке всё отдано local.toDomain().

❓ Вопросы:

1. Какой метод вызывается по pull-to-refresh?
2. Что хранит UserEntity: все заказы или только оплаченные? Считает ли UserEntity.toDomain() поле totalSpent так же?

Если toEntity() сохраняет все заказы, а toDomain() их не фильтрует, premium-статус будет разным в зависимости от того, пришёл профиль из кеша или из сети. Тогда это уже баг из категории «критично».

Направление на будущее, не блокер для релиза: сделать Room единственным источником правды. DAO отдаёт Flow<UserEntity?>, refresh() только пишет в базу, а экран подписан на Flow. Тогда обе проблемы исчезают сами собой.

В2. Публичный изменяемый cachedProfile

Писать в var cachedProfile может любой внешний код. Кроме того, поле читается и пишется из разных потоков: refresh работает на IO. Внутри репозитория оно нигде не читается.

Было:

var cachedProfile: UserProfile? = null

Стало:

@Volatile
var cachedProfile: UserProfile? = null
    private set

Почему лучше: состояние меняется только через методы репозитория, а @Volatile гарантирует, что читатель в другом потоке увидит свежее значение.

lateinit здесь не подходит. «Профиль ещё не загружен» — законное состояние, поэтому nullable выбран правильно.

❓ Кто читает cachedProfile снаружи? Если это ViewModel, чтобы переживать пересоздание экрана, лучше MutableStateFlow<UserProfile?> с публичным StateFlow: тогда UI подписывается, а не опрашивает поле.

В3. Double для денег и магические значения

Для маркетплейса Double в сумме заказов — это накопление ошибок округления (0.1 + 0.2 != 0.3). Сумма 9999.9999999 против порога 10000.0 — это реальный пограничный баг premium-статуса.

Было:

if (o.status == "PAID") {
// ...
if (profile.totalSpent > 10000.0) {

Стало:

private companion object {
    const val TAG = "UserProfileRepository"
    const val ORDER_STATUS_PAID = "PAID"
    const val PREMIUM_THRESHOLD = 10_000.0 // TODO: перейти на Long в копейках
}

Почему лучше: порог и статус видны в одном месте, их легко найти и поменять, а опечатка в строке "PAID" превращается в ошибку компиляции.

Правильное решение — хранить суммы в минимальных единицах (Long, копейки) или в BigDecimal. Но оно затрагивает DTO, Entity и API, так что обсудите его с командой отдельно. Для статуса со временем лучше подойдёт enum class OrderStatus с маппингом в DTO-слое.

В4. Ручные циклы вместо операций коллекций

Здесь коллекционные операции действительно читаются проще. К тому же MutableList больше не утекает в доменную модель.

Было:

val orders = mutableListOf<Order>()
for (o in dto.orders!!) {
    if (o.status == "PAID") {
        orders.add(o.toDomain())
    }
}

var totalAmount = 0.0
for (o in orders) {
    totalAmount = totalAmount + o.amount
}

Стало:

val paidOrders = orders.orEmpty()
    .filter { it.status == ORDER_STATUS_PAID }
    .map { it.toDomain() }
val totalSpent = paidOrders.sumOf { it.amount }

Почему лучше: четыре строки описывают намерение («оплаченные заказы и их сумма») вместо механики, и исчезают два var.

В5. isPremium: вложенные if возвращают true/false

Двенадцать строк здесь выражают одно булево выражение.

Было:

fun isPremium(profile: UserProfile?): Boolean {
    if (profile != null) {
        if (profile.totalSpent > 10000.0) {
            return true
        } else {
            return false
        }
    } else {
        return false
    }
}

Стало:

fun isPremium(profile: UserProfile?): Boolean =
    profile != null && profile.totalSpent > PREMIUM_THRESHOLD

Почему лучше: smart cast после profile != null && работает сам, и правило читается одной строкой.

Про when, раз вы спрашивали: здесь он хуже булева выражения, вот как это выглядело бы:

fun isPremium(profile: UserProfile?): Boolean = when {
    profile == null -> false
    else -> profile.totalSpent > PREMIUM_THRESHOLD
}

when выигрывает, когда веток три и больше или когда вы разбираете sealed-иерархию или enum. Например, будущий OrderStatus удобно маппить так: when (status) { PAID -> ...; CANCELLED -> ...; PENDING -> ... }.

⚪ Вкусовщина: на ваше усмотрение, не блокирует релиз

Т1. Двойной вызов local.toDomain(): лишний маппинг и два разных объекта в кеше и в результате.

// было
cachedProfile = local.toDomain()
return local.toDomain()
// стало
return local.toDomain().also { cachedProfile = it }

Так объект в кеше и объект в результате — один и тот же экземпляр.

Т2. isPremium — это правило домена, а не доступа к данным. Можно перенести его в модель:

// было: repository.isPremium(profile)
// стало, в UserProfile:
val isPremium: Boolean get() = totalSpent > PREMIUM_THRESHOLD
// вызов: profile?.isPremium == true

Так правило живёт рядом с данными, и его не нужно искать в репозитории.

Т3. totalAmount = totalAmount + o.amount → totalAmount += o.amount. После В4 эта строка исчезает, упоминаю её для полноты.

Т4. Trailing comma в именованных аргументах UserProfile(...): при добавлении поля дифф в git будет на одну строку короче.

Итоговая версия класса

class UserProfileRepository @Inject constructor(
    private val api: UserApi,
    private val dao: UserDao,
    @ApplicationScope private val externalScope: CoroutineScope,
    @IoDispatcher private val ioDispatcher: CoroutineDispatcher,
) {

    @Volatile
    var cachedProfile: UserProfile? = null
        private set

    /**
     * @throws IOException нет сети, таймаут или некорректный ответ
     * @throws HttpException сервер вернул не-2xx
     */
    suspend fun loadProfile(userId: String): UserProfile {
        val local = dao.getUser(userId)
        if (local != null) {
            return local.toDomain().also { cachedProfile = it }
        }

        val dto = fetchUser(userId)
        val profile = dto.toProfile()   // сначала валидируем, потом сохраняем
        saveInBackground(dto)

        cachedProfile = profile
        return profile
    }

    /** @return true, если данные обновлены. */
    suspend fun refresh(userId: String): Boolean = try {
        val dto = fetchUser(userId)
        val profile = dto.toProfile()
        dao.insert(dto.toEntity())
        cachedProfile = profile
        true
    } catch (e: IOException) {
        Log.w(TAG, "Refresh failed for $userId", e)
        false
    } catch (e: HttpException) {
        Log.w(TAG, "Refresh failed for $userId: HTTP ${e.code()}", e)
        false
    }

    fun isPremium(profile: UserProfile?): Boolean =
        profile != null && profile.totalSpent > PREMIUM_THRESHOLD

    private suspend fun fetchUser(userId: String): UserDto {
        val response = runInterruptible(ioDispatcher) { api.fetchUserSync(userId) }
        if (!response.isSuccessful) throw HttpException(response)
        return response.body()
            ?: throw MalformedResponseException("Empty body for $userId, HTTP ${response.code()}")
    }

    private fun saveInBackground(dto: UserDto) {
        externalScope.launch {
            try {
                dao.insert(dto.toEntity())
            } catch (e: SQLiteException) {
                Log.e(TAG, "Failed to cache user ${dto.id}", e)
            }
        }
    }

    private fun UserDto.toProfile(): UserProfile {
        val profileId = id ?: throw MalformedResponseException("id is null")
        val name = displayName
            ?: throw MalformedResponseException("displayName is null for user $profileId")
        val paidOrders = orders.orEmpty()
            .filter { it.status == ORDER_STATUS_PAID }
            .map { it.toDomain() }

        return UserProfile(
            id = profileId,
            name = name,
            avatarUrl = avatarUrl,
            orders = paidOrders,
            totalSpent = paidOrders.sumOf { it.amount },
        )
    }

    private companion object {
        const val TAG = "UserProfileRepository"
        const val ORDER_STATUS_PAID = "PAID"
        const val PREMIUM_THRESHOLD = 10_000.0
    }
}

class MalformedResponseException(message: String) : IOException(message)

Одно важное замечание: loadProfile теперь документированно бросает исключения. В ViewModel вызов нужно обернуть в try/catch (IOException) и catch (HttpException), иначе исключение из viewModelScope.launch уронит приложение так же, как раньше !!. Если хотите, чтобы компилятор заставлял обрабатывать ошибку, можно возвращать sealed interface ProfileResult { Success(profile); Failure(cause) }. Скажите, и я покажу этот вариант вместе с кодом ViewModel.

Итог

Общая оценка. Структура и замысел хорошие, но в текущем виде код не готов к проду. Есть как минимум три гарантированных сценария краша: сеть на главном потоке, !! на не-2xx ответе и необработанное исключение в externalScope. Всё это типичные следы механической миграции с Java, и чинится всё за день.

Три улучшения с наибольшим эффектом:

1. Увести сетевой вызов с главного потока и убрать body()!! (К1, К2). Это закрывает самые частые реальные краши: плохая сеть и ошибки сервера.
2. Собрать валидацию DTO в одну функцию toProfile() и сохранять данные в базу только после неё (К3). Это убирает NPE на частичных ответах и не даёт битым данным попасть в кеш.
3. Правильно собрать externalScope (SupervisorJob + обработка ошибок) и перестать глотать CancellationException (К4, К5). Это закрывает ваш второй страх: корутины перестают молча умирать и ронять приложение.

Что уже хорошо:

• Вы используете внедрённый externalScope вместо GlobalScope. Это правильный и рекомендованный паттерн, многие ошибаются именно здесь.
• avatarUrl честно объявлен как String?: опциональность выражена в типе, а не спрятана.
• UserProfile собирается с именованными аргументами, поэтому конструктор читается и его сложно сломать перестановкой полей.
• Слои разделены (toDomain(), toEntity(), DTO отдельно от домена), и это сильно облегчило ревью.
• Вы пришли на ревью до релиза и с правильным списком опасений. Оба ваших страха оказались обоснованными.

Открытые вопросы, от ответов на которые зависит окончательный вердикт:

1. Какой JSON-конвертер у Retrofit и как объявлены поля UserDto?
2. Как в Hilt провайдится externalScope?
3. Являются ли методы DAO suspend-функциями?
4. Какой метод вызывается по pull-to-refresh?
5. Что хранит UserEntity и считает ли её toDomain() поле totalSpent так же, как сетевая ветка?