CODEWITHSUNDEEP
androidkotlinkotlin-coroutines
sonrohancue
sonrohancue

Reputation: 341

How can I refactor this Kotlin method with coroutines properly firing off?

Take this method

override suspend fun login(email: String, password: String): BaseResult<Unit> {
    coroutineScope {
        try {
            fetchMobileConfig()
        } catch (e: Exception) {
            // Unhandled because it seems like a fire and forget.
        }
    }
    

    val loginRequest = LoginRequest(
        email = email,
        password = password
    )

    return when (val response = loginNetwork.login(loginRequest)) {
        is NetworkResponse.Success -> {
            coroutineScope {
                launch {
                    getUserData()
                }
            }

            callSomeFinishingFunctionThatIsSuspending()

            BaseResult.Ok(Unit)
        }
        is NetworkResponse.Error.Unauthorised -> {
            BaseResult.Error(response.exception)
        }
    }
}

I encountered this method and wanted to refactor it. I have some ideas, but want to see if there is anything better that I had not considered. It actually seems to work, but feels a bit chaotic.

The method is called from a ViewModel with viewModelScope{}.

The fetchMobileConfig() and getUserData() functions are kind of odd, but they are currently fire and forget methods it seems. Basically if they fail, the login should not fail.

The BaseResult returned is used to inform the ViewModel of the status. If you are curious how the login method is called in loginNetwork, it looks like this:

override suspend fun login(loginRequest: LoginRequest): NetworkResponse<SessionToken> {
        return performRequest {
            loginService.login(loginRequest)
        }
    }

I am happy with how this is working currently.

What is a nicer way to write this repository level function at the top? I am open to another structure entirely with it.

Upvotes: 1

Views: 223

Answers (1)

Tenfour04
Tenfour04

Reputation: 93882

The coroutineScope { } function is pointless if you are launching fewer than two coroutines inside it, so both of your uses of it can be removed with no change in functionality. In the second case, where you launch a single coroutine, you can remove the inner launch as well so the result of the suspend function call is waited for.

If you want your coroutine to continue even if getUserData() fails, you should also wrap it in runCatching. In your current code, the coroutineScope block is going to rethrow any exceptions encountered in its child coroutine that you launched.

If you use try/catch without using the return value or doing anything in the catch block, you could use runCatching for conciseness.

It's kind of unnecessarily long-winded to use named parameters when the variables you're passing already have matching names.

The above changes give:

override suspend fun login(email: String, password: String): BaseResult<Unit> {
    runCatching { 
        fetchMobileConfig()
    }

    val loginRequest = LoginRequest(email, password)

    return when (val response = loginNetwork.login(loginRequest)) {
        is NetworkResponse.Success -> {
            runCatching {
                getUserData()
            }
            callSomeFinishingFunctionThatIsSuspending()
            BaseResult.Ok(Unit)
        }
        is NetworkResponse.Error.Unauthorised -> {
            BaseResult.Error(response.exception)
        }
    }
}

Upvotes: 1

Related Questions