Any recommendation or preferences for following sn...

# codereview

[miqbaldc]

Any recommendation or preferences for following snippet?

preconditions or throw for this case, maybe any references?

1️⃣

suspend fun updatePushToken(newPushToken: String): Result<String> = invoke {
    require(newPushToken.isNotEmpty()) { "Push token is required to not empty" }

    kotlin.runCatching { repository.agentUpdatePushToken(newPushToken = newPushToken) }
}

or

2️⃣

suspend fun updatePushToken(newPushToken: String): Result<String> = invoke {
        
        if (newPushToken.isEmpty()) {
            throw IllegalArgumentException("Push token is required to not empty")
        }
        
        kotlin.runCatching { repository.agentUpdatePushToken(newPushToken = newPushToken) }
    }

[Joffrey]

Option one personally, if your question is only about the first part.

However, I'd rather not use runCatching in this context, and certainly not fully qualified like this. Why do you want to catch OutOfMemoryError , NoClassDefFoundError and other such throwables when calling agentUpdatePushToken, but you're fine throwing IAE 2 lines above this?

[miqbaldc]

we’re currently use this in UseCase (class), here’s the full snippet:

class AgentPushTokenNotificationUseCase @Inject constructor(
    private val repository: AccountRepository, // remote or local switch in the implementation detail
    dispatchers: CoroutineDispatchers,
) {

    suspend fun updatePushToken(newPushToken: String): Result<String> = withContext(dispatchers) {
        require(newPushToken.isNotEmpty()) { "Push token is required to not empty" }

        kotlin.runCatching { repository.agentUpdatePushToken(newPushToken = newPushToken) }
    }
}

[Joffrey]

What's invoke here? Your class doesn't seem to extend any function type.

Also, I don't believe this answers my points about runCatching 🤔

[miqbaldc]

The runCatching part intention was to consider as a tolerable failures (such any kind of throwable) in our case (and we transform it into a Result.failure(…)), and in a UI Layer, we can shows some sort of events/effect (e.g: showing a dialog in this case)

[miqbaldc]

What’s invoke here? Your class doesn’t seem to extend any function type.

shot, lemme update it first, done updated

[Joffrey]

I think you should catch a smaller subset of exceptions for this purpose, and convert them into a more suited result type. This kotlin.Result type was not meant to represent business errors. Please take a look at this:
https://stackoverflow.com/questions/70847513/when-and-how-to-use-result-in-kotlin

[miqbaldc]

If it in domain/business-usecase (or model), we use a specific sealed class to determine business-error. Does it makes senses to use runCatching or an operator such as Flow.catch for error handling in the consumer side (e.g: UI Layer)?

[ephemient]

runCatching will catch Error which is almost always a mistake. it will also catch CancellationException which will break coroutine cancellation and catch InterruptedException which will break thread interruption.

[ephemient]

Result is meant for use in continuations/flows; the failure definitely gets unwrapped and thrown later

[ephemient]

for your UI layer, you really should consider exactly which exceptions you can handle

[miqbaldc]

Any example/references for how to write error handling in a domain layer?

[miqbaldc]

Here’s my one cents:

sealed class AgentNotificationsResult {
    object EmptyNewPushToken : AgentNotificationsResult()
    data class FailUpdatePushToken(val e: Exception) : AgentNotificationsResult()
    object SuccessUpdatePushToken : AgentNotificationsResult()
}

class AgentNotificationsUseCase @Inject constructor(
    private val repository: AccountRepository,
    private val dispatchers: CoroutineDispatchers = Default,
) {

    suspend fun updatePushToken(newPushToken: String): AgentNotificationsResult = withContext(dispatchers) {
        if (newPushToken.isEmpty()) {
            AgentNotificationsResult.EmptyNewPushToken
        }

        try {
            repository.agentUpdatePushToken(newPushToken = newPushToken)
            AgentNotificationsResult.SuccessUpdatePushToken
        } catch (e: Exception) {
            AgentNotificationsResult.FailUpdatePushToken(e = e)
        }
    }
}

Not sure for the try-catch part that catching general Exception is considered as “business error”, as it’ll catch retrofit2.HttpException or SocketTimeoutException from :data layer.

There’s an article mentioning about transforming the :data exception to a `sealed class`/`inteface` as well in here:

https://miro.medium.com/max/1400/1*0HepdkVU-3zhoXBD2R0aLw.png▾

(the :base has a specific result type and exception in this case)

[miqbaldc]

message has been deleted

[Matteo Mirk]

require() will throw an IAE so they’re equivalent. I would choose [1]