Could someone review our BaseViewModel on coroutin...

# codereview

[ildar.i [Android]]

Could someone review our BaseViewModel on coroutines? It delegates all context handling to special object uiCaller, which enables composition of ViewModel logic

BaseViewModel

[ildar.i [Android]]

I don’t know why, but my text snippet attaches only as a binary. Probably because of size

[christophsturm]

put it into a gist.

[ildar.i [Android]]

https://gist.github.com/ildar2/7db73ed0b721d4cc441f3e5e2d8a526b

[Joffrey]

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?

[ildar.i [Android]]

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

[Joffrey]

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

[Joffrey]

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.

[ildar.i [Android]]

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 🙏

[Joffrey]

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).

[Joffrey]

plus we should call HIDE_LOADING regardless

That's the role of finally blocks, you should probably use it to express this intent

[Joffrey]

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

[Joffrey]

It would be interesting to see an example of usage of this abstract implementation from another component

[ildar.i [Android]]

I will show it in three hours, gtg now.
Or you can look into my sandbox: https://github.com/ildar2/MySandbox

[henrikhorbovyi]

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 🙂

[ildar.i [Android]]

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

[henrikhorbovyi]

hmm… make sense