https://kotlinlang.org logo
#codereview
Title
# codereview
i

ildar.i [Android]

04/12/2022, 12:08 PM
Could someone review our BaseViewModel on coroutines? It delegates all context handling to special object uiCaller, which enables composition of ViewModel logic
I don’t know why, but my text snippet attaches only as a binary. Probably because of size
c

christophsturm

04/12/2022, 12:10 PM
put it into a gist.
j

Joffrey

04/12/2022, 12:18 PM
Sorry I forgot to reply to this in the initial thread. The actual code doesn't have the problem I was assuming from the initial code snippet (the coroutine scope is not created on the spot but a bit more controlled). However it's still confusing to me what you're trying to achieve here. It seems like using the built-in scopes and
withContext
when needed would just be clearer than using callback-based methods (the whole point of coroutines is the direct style, and you're creating a callback-based wrapper on top, which is kinda the opposite of the intent). Why not define your methods as
suspend
functions instead of launching coroutines in a custom scope?
i

ildar.i [Android]

04/12/2022, 12:29 PM
The custom scope is so that i don't need to worry about scope on the call site. From the view I can simply call any VM method which will initiate a request via makeRequest. And the view will receive the result from a stateFlow, or error from errorSharedFlow
j

Joffrey

04/12/2022, 12:31 PM
You usually don't have to worry about scope on the call site anyway, you can use the built-in
lifecycleScope
or
viewModelScope
(depending on the place) without creating your own. This allows coroutines to be cancelled at the proper time in case they are not needed anymore
Another point, you're catching but not re-throwing
CancellationException
. In this particular case I don't see a way this could cause any harm, but in general it's a bad idea to not re-throw it because it could mess up with cancellation of your coroutines. Catching
Throwable
is rather bad practice too, because it caches things like
OutOfMemoryError
,
NoClassDefFoundError
and other unrecoverable stuff like this, which you probably shouldn't try to catch. If what you want is to ensure the loader stops, you can use a
finally
block instead.
i

ildar.i [Android]

04/12/2022, 12:43 PM
I will look into context switching, thank you (it left over from LiveData implementation). CancellationException is not rethrown becase we are at the root context plus we should call HIDE_LOADING regardless. Each call can be cancelled separately or all at once with
stop
. Custom scope so we can avoid implementation details at the call site and to have one line of code there instead of three. Thank you very much for your feedback 🙏
👍 1
j

Joffrey

04/12/2022, 12:45 PM
When I made the context switching comment, I missed the fact that you were calling the other user-provided lambda on the Main dispatcher. That said, I still believe you could just ensure you make your requests on IO dispatcher in your custom suspend function, and the caller then doesn't need to express anything (it will run on main by default if run in built-in scopes).
plus we should call HIDE_LOADING regardless
That's the role of
finally
blocks, you should probably use it to express this intent
Custom scope so we can avoid implementation details at the call site
Scopes are not implementation details, they are very high-level because they decide for how long the caller is interested in the result - you technically don't know it in your abstract implementation
It would be interesting to see an example of usage of this abstract implementation from another component
i

ildar.i [Android]

04/12/2022, 1:03 PM
I will show it in three hours, gtg now. Or you can look into my sandbox: https://github.com/ildar2/MySandbox
h

henrikhorbovyi

04/13/2022, 12:44 AM
I just have point about this separated states (statusStateFlow and errorSharedFlow). I wanted to know why don’t you merge them into a single state? It looks to me that it could cause the UI to have mutable states showing to the user at the same time. It could show a loading state and an error message or an empty state simultaneously, or it would be much harder to deal with many permutations of the two states. Just wanted to know what is your point of view about that 🙂
i

ildar.i [Android]

04/13/2022, 7:31 AM
Errors are events, so we use sharedFlow, Loading state is a state, so we use stateFlow. Errors are usually shown as a toast, rarely as a modal dialog or a snackbar
h

henrikhorbovyi

04/14/2022, 12:24 AM
hmm… make sense
3 Views