android / architecture-components-samples

Samples for Android Architecture Components.
https://d.android.com/arch
Apache License 2.0
23.42k stars 8.29k forks source link

In the example project BasicRxJavaSample, Activity includes business logic! #214

Open arifnadeem7 opened 6 years ago

arifnadeem7 commented 6 years ago

I see this code in UserActivity.java

mDisposable.add(mViewModel.getUserName()
                .subscribeOn(Schedulers.io())
                .observeOn(AndroidSchedulers.mainThread())
                .subscribe(userName -> mUserName.setText(userName),
                        throwable -> Log.e(TAG, "Unable to update username", throwable)));

Why should the Activity manage Disposable? Also subscribeOn, observeOn and the actual subscribe method should go in ViewModel, shouldn't they?

What is the best way to circumvent this?

ivotai commented 6 years ago

ViewModel has two advantage: use onCleared method to disposeļ¼Œsurvive after lifecycleOwner recreating , still hold the data. This example does not override onClearedmethod and send request every time lifecycleOwner onStart . It even shouldn't use ViewModel.

arifnadeem7 commented 6 years ago

Yes ViewModels look like an unnecessary overhead in this particular example, they do too little and they should manage Disposable which right now they aren't!

beilo commented 6 years ago
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())

It is for the viewModel to be able to use junit4 directly in the test module, and try not to import android modules.

arifnadeem7 commented 6 years ago

AndroidSchedulers.mainThread() is not from Android SDK, it is part of RxAndroid library, you can write your own scheduler and remove AndroidScheduler.

beilo commented 6 years ago

but, using observeOn(AndroidSchedulers.mainThread()) in Java is wrong, and I think the switching thread does not have much to do with the model

arifnadeem7 commented 6 years ago

I agree, we can compose this thread switch in the view and then let the ViewModel do the actual subscribe().