oldergod / android-architecture

MVI architecture Implementation of the ToDo app.
Apache License 2.0
669 stars 70 forks source link

Reduce code duplication on ViewModels #16

Closed feresr closed 6 years ago

feresr commented 6 years ago

I would like to submit a PR restructuring the code to avoid the duplication of boiler plate code on MviViewModels. But I would like you get your approval before I get my hands dirty.

My approach would be converting the MviViewModel interface into an abstract class like this:

abstract public class MviViewModel<I extends MviIntent, S extends MviViewState> extends ViewModel {

    private PublishSubject<I> mIntentsSubject = PublishSubject.create();
    private Observable<S> mStatesObservable;

    protected abstract Observable<S> compose(PublishSubject<I> s);

    public void processIntents(Observable<I> intents) {
        intents.subscribe(mIntentsSubject);
    }

    public Observable<S> states() {
        if (mStatesObservable == null) mStatesObservable = compose(mIntentsSubject);
        return mStatesObservable;
    }
}

With this we can avoid repeating the following code: (which is identical for every single ViewModel)

 private PublishSubject<I> mIntentsSubject;
   private Observable<S> mStatesObservable;

    public CustomViewModel(@NonNull TaskDetailActionProcessorHolder actionProcessorHolder) {
        mIntentsSubject = PublishSubject.create();
        mStatesObservable = compose();
    }

@Override
    public void processIntents(Observable<TaskDetailIntent> intents) {
        intents.subscribe(mIntentsSubject);
    }

    @Override
    public Observable<TaskDetailViewState> states() {
        return mStatesObservable;
    }

Creating a new ViewModel is much more straightforward. Can you see any disadvantages to this approach? Please let me know!

oldergod commented 6 years ago

I have thought about this a lot actually and always refrained myself from refactoring the duplicated code.

For someone who'd want to learn what the architecture is about, I think it'd be easier to do so as of now. This repository's main purpose is for people to have a feel of what MVI can look like. Wouldn't it make more difficult to understand with still more abstraction?

feresr commented 6 years ago

You make a valid point, being familiar with MVI makes it hard to say which approach is more friendly to newcomers. Specially because I suspect that people looking at this architecture would be rather experienced in Android. In any case, I took a quick look to the other architectures in googlesamples/android-architecture and they seem to go for the first approach (less abstraction, more code).

Example: In todo-mvp-kotlin each fragment is calling:

    override fun onResume() {
        super.onResume()
        presenter.start()
    }

This could be esaily done in a 'BaseFragment' class to prevent users forgetting to call .start() on their presenters, but this is not the case.

So, you are right, I'd say we don't implement the suggested changes for now. Feel free to close this issue and thanks for the prompt reply.

oldergod commented 6 years ago

Thank you for the input!