2
\$\begingroup\$

I started using Retrofit and RxJava recently and I love it. I am working on an Android project and I am following MVP Repository pattern as specified in Android Architecture Components. I'm still learning though.

Whenever I have to make an API call, I do it like this in the Presenter:

fruitsRepository
  .getFeaturedFruits()
  .subscribeOn(Schedulers.io())
  .observeOn(AndroidSchedulers.mainThread())
  .subscribe(new Observer<UniversalResponse<Fruit>>() {

    @Override
    public void onSubscribe(Disposable d) {
        compositeDisposable.add(d);
    }

    @Override
    public void onNext(UniversalResponse<Fruit> universalResponse) {

        if (view.isLost()) return;
        view.setProgressVisibility(false);

        if (universalResponse.isSessionExpired()) {

            view.onSessionExpired();
            return;
        }

        if (universalResponse.isError()) {

            if (universalResponse.isServerError()) {
                view.showErrorViewerPage(universalResponse.getServerErrorMessage());
            }

            view.showFeaturedFruitsError(universalResponse.getMessage());
            return;
        }

        List<Fruit> fruits = universalResponse.getItems();
        if (fruits != null && fruits.size() > 0) {

            view.showFeaturedFruits(fruits);
        }
        else {
            view.showNoFeaturedFruits();
        }
    }

    @Override
    public void onError(Throwable e) {

        if (view.isLost()) return;
        view.setProgressVisibility(false);

        view.showFeaturedFruitsError(e.getMessage());
    }

    @Override
    public void onComplete() {

        if (view.isLost()) return;
        view.setProgressVisibility(false);
    }
});
  1. fruitsRepository is a Repository that can get data from local or remote source.
  2. .getFeaturedFruits() calls retrofit internally if remote source is selected
  3. compositeDisposable is CompositeDisposable
  4. UniversalResponse is a class having httpStatusCode, isSuccess and List<T>
  5. view is an implementation of UI from FeaturedFruitsContract.View

I am looking for some improvements to this. Can I reduce this somehow? If I have to make 5 different API calls, my presenter gets really bigger and repetitive. I was thinking about making an adapter for Observer I am using to subscribe.

\$\endgroup\$
4
\$\begingroup\$

Let me start by saying I am by no means an expert in Rx. I have no authoritative references to back up my suggested improvements. They come from the little experience I have, as well as some statements in articles from memory. Do no consider my suggestions as "A is bad and B is good so always use B" and only take them into consideration if you agree they are actual improvements.

  • It makes little sense for a retrofit api call to be represented as an Observable. An api call either succeeds with 1 item (the response) or it errs. Rx has a great match for this: Single. There might be exceptions to this but I do not know of any. If anyone does, please let me know. I would suggest you change the method signature to Single<List<Fruit>> getFeaturedFruits()
  • The scheduler on which an operation should run should be decided by the component that does the actual operation. subscribeOn should therefore be in the fruitsRepository.getFeaturedFruits. It might look like this

    Single<List<Fruit>> getFeaturedFruits() {
        fruitsApi.getFeaturedFruits()
                .subscribeOn(Schedulers.io())
    }
    
  • In case you have an observable that only emits 1 item and then completes, which is the case if you have a retrofit api call wrapped in an obserable, it makes little sense to implement onComplete in the subscriber if you already implemented onNext and onError. onComplete is more commonly used when you have a hot observable can emit multiple items and you don't know how many there might be. If you use a Single instead, you don't even have to think about this because there is only success or error, no next and complete.
  • You have if (view.isLost() return; in every callback. This seems like you might not be disposing of the observables properly. I don't know what .isLost() is doing, but consider instead having the view tell the presenter when it is "lost" and have the presenter clean up the compositeDisposable when that happens.
  • Use lambdas. Especially when using Rx where you use anonymous classes all over the place, they will greatly reduce the lines of code and (debatable) readability of the Rx chain. In recent versions of the android sdk the lambda notation is supported natively, otherwise you can use "retrolambda". This will let you reduce

    .subscribe(new Observer<UniversalResponse<Fruit>>() {
    
        @Override
        public void onSubscribe(Disposable d) {
        }
    
        @Override
        public void onNext(UniversalResponse<Fruit> universalResponse) {
        }
    
        @Override
        public void onError(Throwable e) {
        }
    
    
        @Override
        public void onComplete() {
        }
    }
    

    to something like

    .subscribe(disposable -> {}, universalReponse -> {}, e -> {}, {})
    

    If you were to use a Single and just handle the success and error callbacks, you don't have void onSubscribe(Disposable d) from where you can add d to the compositeSubscription anymore. However, the .subscribe() call returns a disposable that you can handle the same way:

    compositeDisposable.add(getFeaturedFruits().blabla.subscribe());
    
\$\endgroup\$
  • \$\begingroup\$ Thanks for the answer. This helped me in improving a lot of things, I didn't know about the use-cases of Single and having no onComplete() and onNext() will reduce my code. I didn't get 4th point, isLost() checks if context is null and whether view fragment is added or not using isAdded(). Are you saying if I dispose Disposable, the observer will not receive any callbacks in onNext() etc..? \$\endgroup\$ – rupinderjeet May 16 '18 at 12:47
  • \$\begingroup\$ I will learn and implement lambda expressions soon. I have tried it using the setting in build.gradle. Thanks again, this is really helpful. \$\endgroup\$ – rupinderjeet May 16 '18 at 12:50
  • \$\begingroup\$ Are you saying if I dispose Disposable, the observer will not receive any callbacks yes. It is like saying "whatever you are doing, I don't need it anymore" \$\endgroup\$ – Tim Castelijns May 16 '18 at 12:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.