Conversation
sergeykamilyevich
left a comment
sergeykamilyevich
left a comment
There was a problem hiding this comment.
привет! оставил пару комментариев
|
|
||
| class CatsViewModel(private val catsService: CatsService) : ViewModel() { | ||
| private val _uiModelFlow = MutableStateFlow<Result>(Result.Loading) | ||
| val uiModelFlow: StateFlow<Result> = _uiModelFlow |
There was a problem hiding this comment.
лучше в конце добавлять .asStateFlow() чтобы нельзя было скастить к MutableStateFlow
| sealed interface Result { | ||
| data class Success(val uiModel: CatsUiModel) : Result | ||
| data class Error(val message: String?) : Result | ||
| object Loading : Result |
There was a problem hiding this comment.
минорно, для удобства лучше сделать data object
There was a problem hiding this comment.
Только ради генерации toString()? Ну да, хуже не будет.) Но для этого надо котлин поднять в проекте, так что пожалуй не буду
| private val _uiModelFlow = MutableStateFlow<Result>(Result.Loading) | ||
| val uiModelFlow: StateFlow<Result> = _uiModelFlow | ||
| private val exceptionHandler = CoroutineExceptionHandler { _, e -> | ||
| when (e) { |
There was a problem hiding this comment.
в exceptionHandler лучше не обрабатывать ошибки, а только делать логирование, что и указано в условиях задачи, сюда должны попасть неперехваченные в try-catch ошибки
There was a problem hiding this comment.
Тогда стэйт ошибки не дойдет никогда до ui. Либо внутри async ловить ошибки, тогда они не дойдут до exceptionHandler-а, чтобы там залогироваться. Либо давать им уходить в exceptionHandler и если там только логировать, то стэйт не обновиться.
| viewModelScope.coroutineContext.cancelChildren() | ||
| viewModelScope.launch(exceptionHandler) { | ||
| _uiModelFlow.emit(Result.Loading) | ||
| val factJob = async(Dispatchers.IO) { |
There was a problem hiding this comment.
хорошая практика передавать Dispatchers через конструктор вьюмодели, чтобы его можно было подменять (например в тестах)
There was a problem hiding this comment.
Не спорю, не было такой задачи
| null | ||
| } | ||
| } catch (e: Throwable) { | ||
| ensureActive() |
There was a problem hiding this comment.
тут эта проверка не имеет особого смысла, лучше в блоке when сделать проброс CancellationException дальше:
if (e is CancellationException) throw e
There was a problem hiding this comment.
"ensureActive()" короче чем "if (e is CancellationException) throw e" и делает примерно тоже самое
There was a problem hiding this comment.
ensureActive кидает новый эксепшн с новым стек трейсом, а throw e пробрасывает дальше исходный
There was a problem hiding this comment.
Так это новый CancellationException, какой там стэктрэйс нужен?
There was a problem hiding this comment.
перехватывать и подменять CancellationException в данном случае совершенно неоправданная практика, ты потеряешь оригинальное сообщение, причину и место выброса
There was a problem hiding this comment.
Ок, хотя я не до конца понимаю, где эта инфа может понадобиться
| } else { | ||
| null | ||
| } | ||
| } catch (e: Throwable) { |
There was a problem hiding this comment.
нет обработки SocketTimeoutException
There was a problem hiding this comment.
А там четко не сказано, что это надо для картинок и что вообще должно происходить, когда ошибка только в одном из запросов. Какое условие такой и результат
| } | ||
|
|
||
| scope.launch { | ||
| val fact = factJob.await() |
There was a problem hiding this comment.
тут не выполнено требование одновременной отмены запросов, и launch и оба async запущены на одной SupervisorJob и отмена одной загрузки никак не повлияет на другую
There was a problem hiding this comment.
Там нет такого требования там написано: "Отменятся запросы должны одновременно". Там не написано отменять оба запроса в случае ошибки в любом из них. Отменяю я запросы при закрытии экрана вместе с гибелью скоупа. Это происходит одновременно
2 participants
No description provided.