Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unused unsubscribe method on CompletableUseCase #32

Open
gh-23378 opened this issue Dec 28, 2017 · 3 comments
Open

Unused unsubscribe method on CompletableUseCase #32

gh-23378 opened this issue Dec 28, 2017 · 3 comments

Comments

@gh-23378
Copy link

gh-23378 commented Dec 28, 2017

It looks like we're just creating an empty disposable in this class and disposing of it in unsubscribe without ever changing its value. In the SingleUseCase class, we pass an observer into the execute method and capture a reference to the subscription by subscribing inside of thhat method. I would have expected the same to happen in this class, or if the idea is that we subscribe manually from our presenter, shouldn't we also unsubscribe manually and remove this unsubscribe method altogether?

abstract class CompletableUseCase<in Params> protected constructor(
        private val threadExecutor: ThreadExecutor,
        private val postExecutionThread: PostExecutionThread) {
    // Here we initialize subscription as an empty disposable
    private val subscription = Disposables.empty()
    protected abstract fun buildUseCaseObservable(params: Params): Completable

    // I would have expected an observer to be passed into execute so that we can
    // subscribe in this method and obtain a reference to the subscription 
    fun execute(params: Params): Completable {
        return this.buildUseCaseObservable(params)
                .subscribeOn(Schedulers.from(threadExecutor))
                .observeOn(postExecutionThread.scheduler)
    }

    // Now we unsubscribe from our empty disposable
    fun unsubscribe() {
        if (!subscription.isDisposed) {
            subscription.dispose()
        }
    }
}
@gh-23378 gh-23378 changed the title Broken unsubscribe method on CompletableUseCase Unused unsubscribe method on CompletableUseCase Dec 28, 2017
@ghost
Copy link

ghost commented Feb 19, 2018

Any example for CompletableUseCase?

@qwertyfinger
Copy link

@DevKate In this case it's most probably a mistake.
I think CompletableUseCase should either hold the exact same logic as SingleUseCase:

fun execute(
        completableObserver: DisposableCompletableObserver, 
        params: Params
) {
        disposables.add(
            buildUseCaseObservable(params)
                .subscribeOn(Schedulers.from(threadExecutor))
                .observeOn(postExecutionThread.scheduler)
                .subscribeWith(completableObserver)
        )
    }

or return a Completable from execute() function and be stripped of subscription variable along with unsubcribe() function.
But in this case it's better to also have the same logic in SingleUseCase.

The choice between these two depends on whether you need to have access to a cold Observable in your Presenters (e.g. to combine them or apply some additional logic). I prefer using the latter in any case.
See this thread for more information.

@qwertyfinger
Copy link

@852dev Example usage? Saving some data to the database, for example. Anything where we don't need any actual data passed back to us, only the success/error status.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants