android10 / Android-CleanArchitecture

This is a sample app that is part of a series of blog posts I have written about how to architect an android application using Uncle Bob's clean architecture approach.
Apache License 2.0
15.51k stars 3.32k forks source link

Composing Use Case observables #121

Open kiwiandroiddev opened 8 years ago

kiwiandroiddev commented 8 years ago

Hi guys, great project, at my company we have based an app on this template and the principles in the accompanying blog post.

For some more advanced features in the app, we ran into an issue that is very similar to another one mentioned by @amatkivskiy (https://github.com/android10/Android-CleanArchitecture/issues/63). We would like to be able to compose and combine use cases together to form new use cases.

As mentioned, RxJava provides a huge toolbox of operators for composing observables which would be perfect. However, the Use Case interface in this project accepts Subscribers and doesn't expose Observables directly.

This makes it awkward to compose and combine use cases with RxJava operators. The approach I went with was first to use Subjects to adapt the subscriber interface into an observable, then to create a new custom observable. This works but isn't great as there's a lot of nesting and boilerplate.

I was wondering what the reasoning behind the design choice of having Use Cases accept Subscribers rather than returning Observables is? Is it so that Use Cases hide the knowledge of which scheduler is used to subscribe on/observe on? What would be the downsides of having Use Cases return a single Observable?

I'm still new to using Rx in an Android app architecture, so I'm keen to hear some discussion around this and learn something. Thanks again for contributing this to the community :)

FranRiadigos commented 8 years ago

Hi @kiwiandroiddev,

I will try to give you an example on how I understand Use Cases. I think it's intended to be one Use Case for one specific process.

Lets say a flight destination is a Use Case. If your are going to take a flight toward a Country, you cannot take another flight at same time to another, unless you divide yourself right? But if you need it, you can flight toward the Country2, making a transfer/transit when you arrive at Country1 by another flight toward the Country2. Maybe you need to buy something from Country1 before to land to Country2.

Use Cases, usually should deal with Repositories, and Repository calls can be chained because they expose Observables. There is no reason to not have multiple repositories in one Use Case.

Repositories retrieve data, but a Use Case performs a specific operation. Think in Use Cases like that information provided by your Product Team, or by the Team that define requirements, generally using UML.

This is my approach, I hope it can be useful for you.

Best.

amatkivskiy commented 8 years ago

@kuassivi Thanks for explanation. But I think that @kiwiandroiddev ment the use case when you want to use for example RxLifecycle. It requires access to the Observable.

FranRiadigos commented 8 years ago

Ok, but I think there is no limit to use RxLifecycle inside a Use Case through a Repository, even passing a Provider. That could also decouple the lifecycle logic from your Activity. It is just my point of view.

lenguyenthanh commented 8 years ago

I have a question about Use Case too. We now encapsulate Observable in an UseCase class. So we need to make some actions like transform data from Domain model to View model on main thread. And it is not really good. We'd better to do transform data on background thread. How do you guys think about it?

Trikke commented 8 years ago

@kuassivi small note : RxLifecycle shouldn't be used inside a Use Case, you'd be breaking boundaries. RxLifecycle can be used in the Presenter to compose and subscribe to the Use Case.

@lenguyenthanh Domain model to View model transformations should happen in the Presenter. You can still do scheduling there.

lenguyenthanh commented 8 years ago

@Trikke thx for answering my question. But Use Case don't expose Observable class for us so how we can do scheduling there?

FranRiadigos commented 8 years ago

@lenguyenthanh If I'm not wrong, model transformations are performed in the map() function of an Observable. That function is performing on the Thread configured in the subscribeOn() function, which is automatically set in the buildObservable() method of the Use Case. To test it, put a breakpoint inside the map() method of your Observer, then look for the current thread name with Thread.currentThread().getName().

lenguyenthanh commented 8 years ago

@kuassivi It should be like that. But in this example it looks like this:

private void showUsersCollectionInView(Collection<User> usersCollection) {
    final Collection<UserModel> userModelsCollection =
        this.userModelDataMapper.transform(usersCollection);
    this.viewListView.renderUserList(userModelsCollection);
  }

  private void getUserList() {
    this.getUserListUseCase.execute(new UserListSubscriber());
  }

  private final class UserListSubscriber extends DefaultSubscriber<List<User>> {

    @Override public void onCompleted() {
     // bla bla
    }

    @Override public void onError(Throwable e) {
      // bla bla
    }

    @Override public void onNext(List<User> users) {
      UserListPresenter.this.showUsersCollectionInView(users);
    }
  }

You can see that, transforming process is on onNext() which is on main thread.

FranRiadigos commented 8 years ago

@Trikke agree, putting .compose(activity.<Long>bindToLifecycle()) inside a Use Case is going to break its boundaries, anyway I don't think Use Cases must expose Observables.

@lenguyenthanh yep. The transformation should be inside the UseCase to work with the same thread. Something like:

@Override public Observable buildUseCaseObservable() {
    return this.userRepository.users().map(users -> this.dataMapper.transform(users));
  }
lenguyenthanh commented 8 years ago

@kuassivi Agree, but (but again :smile: ) UseCase should not know about presentation layer so it normally can't have dataMapper instance. So how do you solve this problem? Using DI or any other way?

Trikke commented 8 years ago

@lenguyenthanh Ah yes, i noticed. This example has a different setup for Use Case subscription than the one i'm using. Sorry, i forgot that. But are Domain to View Model transformations that expensive? What kind of expensive objects are you mapping, or are we talking about thousands of objects being mapped at the same time? Usually this is just some data mapping. Maybe just map them in the subscriber in the Presenter, and do some performance testing to see if this is really an issue.

@kuassivi Transformations to View Models can't be inside the Use Case, you'd be breaking boundaries.

FranRiadigos commented 8 years ago

No mention, because this probably can be a long discussion.

If you are pulling new data when scrolling down a list, something usual, you may need to map every time your retrieved data, so that can be expensive.

I don't think I'm breaking boundaries in this case, although I see there are a lot of boundaries. A mapping process in a Observable is common, and I'm not specifying what kind of mapper is in the implementation.

If you have a mapping process to be performed for that Use Case, then provide it, otherwise not.

Trikke commented 8 years ago

You are breaking boundaries in this case because the Use Case is Domain-specific and the mapper maps Domain Entities to View Models. Because that mapper knows about View-specific logic, it must reside in that layer and cannot be used by the Use Case.

For @lenguyenthanh , there are a few solutions:

I'm using the 2nd approach already, mainly because in my sentiment the data flow doesn't stop at the Use Case. It should continue flowing through to the Presenter as there is View logic that consumes or reacts to this data. (ie : map stuff, show buttons on certain conditions)

FranRiadigos commented 8 years ago

Sure, but as I said ... I'm not specifying what kind of mapper is in the implementation. of the Use Case. The mapper can map from one domain to another different domain right? In this case, will it break boundaries?

lenguyenthanh commented 8 years ago

@Trikke thanks for your solutions. I prefer 2nd option too, it's more flexible. I'm thinking about adding transformer object to Use Case objects but it doesn't look good. Could you please give me an example of how you implement it?

Trikke commented 8 years ago

@kuassivi We were talking about a Domain Entity to View Model mapper, which @lenguyenthanh was asking questions about. I'm not sure what you mean by "I'm not specifying what kind of mapper is in the implementation". A mapper just maps data from one kind to another, But where this mapper resides depends greatly on what data we are talking about.

The "Domain" is a layer in which al your business logic must reside. Usually Use Cases and other classes that describe the "core business" of your app. There is just one "Domain" layer. The "View" or "Repository" are not other "Domains", but actual separate layers. So when data crosses a layer, we have to make sure an inner layer knows nothing of the outer layer. Since the "Domain" layer is an inner layer opposite to the "View" layer, we have to take care in not exposing the View Models in the "Domain" layer.

@lenguyenthanh in my implementation, the Observables in a Use Case are just exposed, but do contain all business rules. The Presenter can chain on that, but only for view logic.

I'll give an example here of something simplified and made up. Please don't look at this code for compile correctness, i quickly typed it by hand.

in Use Case (there is no specific business logic for getting a User, so we just return one from the Repo)

public GetUserDetailsUseCase(String userId, UserRepository userRepository) {
        this.userId = userId;
        this.userRepository = userRepository;
    }
public Observable<UserEntity> get() {
        return userRepository.getUser(userId).compose(this.<UserEntity>applySchedulers());
    }

in Presenter (on button press, we load a user, map it to the view model, and let the view render that)

public loadUserOnButtonPress() {
    getUserUseCase.get()
    .map(this.structureToModelMapper.transformUser)
    .subscribe(new Subscriber(){
        @Override public void onNext(UserModel user) {
                getView().showUser(user);
        }
    });
    }
lenguyenthanh commented 8 years ago

@Trikke I get your ideal, you made my day. Thank you so much.

android10 commented 8 years ago

Kudos @Trikke :+1:

spirosoik commented 8 years ago

@Trikke so you are following a little bit different kind of building a UseCase. I mean according to this repo your implementation is a little bit different as you break the abstraction buildUse....

amatkivskiy commented 8 years ago

@spirosoik No, he just changed the method name and return type. Otherwise the method is the same as buildUseCase.

FranRiadigos commented 8 years ago

I finally realized it could be interesting to expose the Observable, so following the suggestion from @Trikke I refactored the UseCase abstract class to this one. I hope this helps someone.

public abstract class UseCase<T>  {

     private final ThreadExecutor threadExecutor;
     private final PostExecutionThread postExecutionThread;

     protected UseCase(ThreadExecutor threadExecutor,
                                    PostExecutionThread postExecutionThread) {
        this.threadExecutor = threadExecutor;
        this.postExecutionThread = postExecutionThread;
    }

    private Observable.Transformer<T, T> applySchedulers() {
        return observable -> observable
                          .subscribeOn(Schedulers.from(threadExecutor))
                          .observeOn(postExecutionThread.getScheduler());
    }

    protected abstract Observable<T> buildUseCaseObservable();

    final public Observable<T> get() {
        return buildUseCaseObservable().compose(applySchedulers());
    }

    final public Subscription execute(Subscriber<T> subscriber) {
        return get().subscribe(subscriber);
    }
}
public class GetUserDetails extends UseCase<User> {

  private final int userId;
  private final UserRepository userRepository;

  @Inject
  public GetUserDetails(int userId, UserRepository userRepository,
                                      ThreadExecutor threadExecutor, 
                                      PostExecutionThread postExecutionThread) {
    super(threadExecutor, postExecutionThread);
    this.userId = userId;
    this.userRepository = userRepository;
  }

  @Override protected Observable<User> buildUseCaseObservable() {
    return this.userRepository.user(this.userId);
  }
}

I haven't provided the unsubscribe()method because I prefer to handle them with a CompositeSubscription in the Presenter.

Trikke commented 8 years ago

@spirosoik I'm just using a different implementation, and passing the data flow into the Presenter. No boundaries are broken. The Presenter ultimately handles the Subscription. It's quite easy to then map data to an Adapter or a paging mechanism. This video also talks about this and they show ways they have implemented this.

kiwiandroiddev commented 8 years ago

Thanks @Trikke and @kuassivi, that has explained a lot. From that I gather there's nothing wrong with exposing Observables from the domain layer, and I think we'll move towards that in our project to get that extra flexibility in the presentation layer.

What's still not completely clear to me is the thinking behind encapsulating Observables in the domain layer in the original implementation. As @Trikke says data flow doesn't stop at the domain layer and surely you would get the biggest payoff from a reactive architecture by applying it all the way down the stack. I'm curious as there must be some advantage to this approach?

android10 commented 8 years ago

@kiwiandroiddev right! As an app scales, there are new requirements and surely refactors must by applied. This is, let's say a pet project for learning purpose and in this specific use case, not a lot of data composition is required at the presentation layer. With that being said, this is the main purpose for these sort of discussions: to receive feedback and input in order to improve the codebase :smile:

kiwiandroiddev commented 8 years ago

@android10 That makes sense :) This project and these kind of discussions are great for advancing the state of art of Android app development I think, so thanks again for the contribution.

rshah commented 8 years ago

Is it should be ok or considered bad practice, if we mix the implementation?

I mean some UseCase expose it's observable and other hide it. Because not all UseCase need to be chained or composed with other observable.

Trikke commented 8 years ago

@rshah , no don't start mixing. You stick with either system. And it's generally a better idea to have them composable, even if you don't "need" to do so for some use cases.

zoopolitic commented 8 years ago

@Trikke What about exposing types ? In your example UseCase knows about UserEntity which is not part of domain, but part of data. If response from api is different from model domain is know about then UseCase should now about this type, but I think response from server should not be a part of domain

Trikke commented 8 years ago

@zoopolitic Indeed, an api response wouldn't be part of the domain. Usually that would reside in the Data Layer. And you'd use a Mapper to map it from a UserApiResponse to a UserEntity.

zoopolitic commented 8 years ago

@Trikke But your Use case return Observable< T >, so if api returns UserApiResponse - then UseCase will return Observable< UserApiResponse > and this response is part of data, but domain knows about it it this case

Trikke commented 8 years ago

@zoopolitic I never said to pass UserApiResponse with the Observable chain. That's why i implied you'd use a Mapper to make UserApiResponse to UserEntity. The same applies to reading stuff out of a database or another data source. Your DB could return a UserRowStructure. You don't pass that around, and again you use a Mapper to map it to a UserEntity.

You convert data from some external form, such as an external service, database or even filesystem, to the internal form used by the use cases.

zoopolitic commented 8 years ago

@Trikke Thanks, I've already figured out. Now I do mapping in gateways so Use Cases work only with domain data and stay isolated from another layers

spirosoik commented 8 years ago

@Trikke quick question are you using a mapper library for convenience? we do the mapping by hand. I haven't tried a library for this but there are some nice libs for this. Not sure if they use reflection or compile time generation.

for example http://mapstruct.org/ is working on compile-time and is a code generator for mappers. If you used something give us your impressions please

Trikke commented 8 years ago

@zoopolitic : glad to help, and yes that's what you should be doing. Your domain data is holy; what comes in from an external source should always be converted to your own data.

@spirosoik , unfortunately i haven't tried any mapping libs yet. The larger project i maintain has a legacy api with support for Android, iOS and Windows Apps with different data for each platform merged together, it's a mess. So i've got all my mappers written by hand because i know the intricacies of that horrible legacy api. And other personal projects are usually so small that i never bothered checking a mapping lib. But i will check out the example you posted! Maybe it does things just right for me :)

spirosoik commented 8 years ago

@Trikke thanks for the answer.

@android10 I have the same question for you:

quick question are you using a mapper library for convenience? we do the mapping by hand. I haven't tried a library for this but there are some nice libs. Not sure if they use reflection or compile time generation.

for example http://mapstruct.org/ is working on compile-time and is a code generator for mappers. If you used something give us your impressions please

lenguyenthanh commented 8 years ago

IMHO, mapping by hand with some libraries like AutoValue (with builder) is the best option now. Using mapping library is quite over engineer.

Trikke commented 8 years ago

@lenguyenthanh : i think mappers have their uses, it depends on preference and scope. For me, my domain is immutable with AutoValue and external sources are hand mapped (see above reasons).

FranRiadigos commented 8 years ago

But your Use case return Observable< T >, so if api returns UserApiResponse - then UseCase will return Observable< UserApiResponse >

@zoopolitic

Not necessary like that, in my code sample, you can see the UseCase class implies a parameterized type

public abstract class UseCase<T>{}

so both abstract Observable<T> buildUseCaseObservable() and Observable<T> get() must be composed with the same type. So, unless you specify a type from the Data Layer, it should return the expected one, the Domain type, that @Trikke mentioned

use a Mapper to make UserApiResponse to UserEntity

@Trikke

Maybe doing the transformation previously inside the Repository. (And also because the UseCase belongs to the Domain Layer, so you shouldn't be able to return a Data type, only Domain types)

@spirosoik Have you tried https://projectlombok.org to generate builders for your mappers?

spirosoik commented 8 years ago

@kuassivi no I didn't try. Have you already used that?

We are experimenting with mapstruct

FranRiadigos commented 8 years ago

Nope, I didn't yet, but I'll try to test something like

public Foo mapToFoo(Bar bar) {
     new Foo.Builder(bar).build();
}

Both object should have the same fields like in mapstruct, but this library looks very interesting as well.

StuStirling commented 8 years ago

@kuassivi I like your implementation of UseCase so Observable is exposed however, the Schedulers are always added so there is no way to get the Observable beforehand.

Say for example you want to subscribe and observe on the background thread, how would we go about this? In my scenario, I have a method the returns a domain entity of a specific id. I do not want to have to implement the whole Subscriber pattern every time I want to retrieve an entity of specific id. I have found a few different options but unsure which are valid and which break best practices.

  1. Use the get() on UseCase and then overwrite the .subscribeOn and .observeOn methods on the returned Observable. Not sure if that would work or is a valid solution.
  2. Provide a matching UseCase but use the @Named parameter and supply a different PostExecutionThread. This seems like code overkill.
  3. use .toBlocking() and then first() on the Observable that returns the item from my repository.

UPDATE

.toBlocking didn't work because I am using .defer on the Observable so must subscribe.

I added a getNoSchedulers() which just returns the Observable without .compose. That way it runs in the thread that calls the subscribe. This gives me the functionality I wanted but it doesn't look pretty.

FranRiadigos commented 8 years ago

@StuStirling, in fact that implementation was taken from a previous comment from @Trikke If you want to subscribe and observe both on the background then the ThreadExecutor and the PostExecutionThread must be the same. My current implementation is a bit different right now, I use a common Interface with a @Name as you mentioned to pass the Thread Executors in the constructor. I would recommend you then the 2 option.

jemshit commented 6 years ago

Finally, i was not sure why Observable is not exposed from UseCase to Presenter, since i want to do sth like this:

myUsecase.doOnSubscribe(view.showLoading())
    .doOnTerminate(view.hideLoading())
    .subscribe(...);

Also you have to use subscribeWith(Observer<>) in UseCase and you won't be able to use lambda (for onNext, onError...) in Presenter.

Also as in this approach, Reactive Data Layer, stream from Repository to Presentation layer never stops (it is infinite), exposing Observable seems better approach.

I was wondering what was the important reason (big deal) giving Subscriber to UseCase and not exposing Observable to outside. From what i understand from @Trikke and @android10 useCase code in this repo is just one way of doing it and no big reason is behind it. Maybe it should be stated in Readme.

spirosoik commented 6 years ago

@jemshit it's up to you to decide when you want to have observable and where. what @android10 suggests in this repo is the clean architecture implementation in android. I don't think that there is a specific rule which you will violate if you expose Observable also in Presenter. also @Trikke adapts his implementation based on their team's needs.

I can say that it's really valid your concern not only about the example you mentioned but also it's reasonable for keeping in the back-ground thread the data transformation (mapping) for performance reasons too.