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

pajatopmr

02/11/2019, 4:47 AM
Coroutines are incredibly complex, as I have learned over the past week or so. As part of a movie/tv app I am writing, I faced the need to fetch millions of lines of JSON data for a half dozen or so lists. This is my second take at a daily task to update these records in the app. Critique’s much appreciated:
The actual code is on Github at https://github.com/pajato/tmdb-lib fwiw
g

gildor

02/11/2019, 5:38 AM
1. I think you shouldn’t use
runBlocking
, but instead make function suspendable 2. It’s not clear for me, why do you need channel in this case 3. Instead of passing scope as argument, I would just make function suspend (as in item 1) and use
coroutineScope {}
block, because this function shouldn’t return until it finish job 4. I would convert fetchLines to suspend function +
withContext(IO)
, so function become non blocking and you can just can call it, without worry about dispatcher 5. Using MutableMap to return value doesn’t seems correct, better just return value
p

pajatopmr

02/11/2019, 6:09 AM
Cool, thanks for offering up a critique.
1) Making dailyExportTask() suspend causes an error: the invoking code must be a coroutine, so this suggestion just forces the runBlocking (or equivalent) to be moved.
2) Channel is for safe transfer of list back to the main thread.
g

gildor

02/11/2019, 6:13 AM
2) Channel is for safe transfer of list back to the main thread.
This is strange reason, suspend function is also completely trade safe for that (of course if you don’t touch global state)
1) Making dailyExportTask() suspend causes an error: the invoking code must be a coroutine,
Yes, because you use launch it outside of coroutine, I added a couple TODO with suggestion just expost this API as asyncronous suspend function rather than do blocking network call on singleton access, which is really smells
6. I see that you use a lot of nested functions, this fine, but for my eyes just adds a lot of noise and overcomplicated closures and harder to follow. I refactor everything to private top level functions, so you have this same encapsulation as before but imo it improved readability
p

pajatopmr

02/11/2019, 6:25 AM
It’ll take me a while to grok your changes, which I appreciate you suggesting, but it is very clear we don’t care for each other’s preferred style. 🙂
g

gildor

02/11/2019, 6:25 AM
So, in general, to your original comment “Coroutines are incredibly complex”, I see that your code was quite complex, you can see that in my version I have only 1 suspend nd function that used only to wrap blocking code and use of
map + awaitAll
patter to run multiple coroutines concurrently and await all the results It doesn’t look complicated for me Approach with Channel was much more complicated, it true, but there is no reason to use it
Yes, sure, i agree about style, but because I changed how functions are interact each other (no more global state, no more global closures, just pure functions that return result), It looked reasonable also move functions to top level
p

pajatopmr

02/11/2019, 6:32 AM
Was it clear that dailyExportTask() will be executed in a main (UI) thread so it needs to just kick off the downloads and update the list data later? The downloads (without coroutines) take ~12 seconds, unacceptable at startup. I haven’t figured out yet of your changes preserve this behavior.
g

gildor

02/11/2019, 6:33 AM
I see what you mean, and this is problem with
runBlocking
If you want async code, just use async request
p

pajatopmr

02/11/2019, 6:34 AM
Ok, I’ll keep studying your solution.
g

gildor

02/11/2019, 6:34 AM
Yes, my solution doesn’t handle issue with main thread, I can show how you can handle it if make your API suspendable
p

pajatopmr

02/11/2019, 6:35 AM
Go for it.
Updated commit, added info about related issues with suspend functions testing
4 Views