Rightpoint / android-template

A `cookiecutter` template for Android projects
28 stars 4 forks source link

Coroutine Support #74

Open mattognibene opened 5 years ago

mattognibene commented 5 years ago
jonduran3000 commented 5 years ago

I have a number of notes and I figure I'll just add them as an overall comment as opposed to several smaller comments:

  1. Why use the approach of hiding all of the coroutine logic behind the use case? This is pretty much the same as using a callback except you are passing in a scope as well, so it's trying to have it both ways. You could just fully hide this implementation behind the data layer and callback and not pass in a scope. You lose a lot of control that way obviously but the most control you can have is to just expose the suspend function.
  2. One of the disadvantages of this requiring a callback (in the form of a higher-order function) is that we are now forcing the use of this custom Result class. I feel like there could a number of problems with this; the main one being that this clashes with kotlin.Result.
  3. I think calling it backgroundJob confuses a lot of coroutine terminology. The async function returns a Deferred object which is more like a future/promise.

My personal opinions on it are this you either lean into this having a suspend function that's exposed or hide everything behind the data layer and a custom callback. The latter option sort of defeats the purpose of this being called a CoroutineUseCase at all though.

Example 1:

interface CoroutineUseCase<T, R> {
    suspend fun executeCoroutine(params: T): R
}

Example 2:

abstract class CallbackUseCase<T, R> : BaseUseCase<T, R>() {
  fun executeCallback(params: T, callback: Callback<R>) {
    try {
      callback.onSuccess(execute(params))
    } catch (e: Exception) {
      callback.onFailure(e)
    }
  }
}
interface Callback<R> {
  fun onSuccess(result: R)
  fun onFailure(error: Throwable)
}

Edit: the second example will obviously need to add logic to handle threading.