About handling data from an API call through RX and Retrofit

Question

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.

Answer 1

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());
  

• rupinderjeet: 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..?
• rupinderjeet: I will learn and implement lambda expressions soon. I have tried it using the setting in build.gradle. Thanks again, this is really helpful.
• Tim: 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"