badoo / Chateau

Chateau is a framework for adding (or improving) chat functionality in any Android app
MIT License
664 stars 93 forks source link

Problem with separating. #5

Open bkodirov opened 8 years ago

bkodirov commented 8 years ago
  1. As I understand in your case BARF is domain+Entity layer. This layer should be fully platform independent and should be java library module in your project. It may know about RX. That is ok. In your case domain layer knows about the platform. You use RxAndroid in it.
  2. Your ChateauCore (data+use case ) layer doesn't have to know about UI things like recycler view. And it should be an Android library in the perfect world.
kingamajick commented 8 years ago

Hi @behzodbek

Thanks for your feedback. This problems have both been fixed in our internal copy of Chateau and we will be pushing this to GitHub soonish.

With regards to issue 1, the ScheduleOn has been removed as we now leave it to the DataSources to decide which Scheduler to user, which is configured at instantiation of the DataSource.

With regards to issue 2, this appears to be an erronious import from earlier iterations of the project.

I’ll leave this issue open until that push and close it then.

Cheers

bkodirov commented 8 years ago

@kingamajick It would be more flexible if your Use case take 2 implementation of Executor interface.

  1. ExecutionThread
  2. PostExecutionThread And do something like in the use case yourSubscriber.subscribeOn(Schedulers.from(threadExecutor)) .observeOn(postExecutionThread.getScheduler()); How to implement execution and postExecution thread up to data layer(Because data layer knows about platform). P.S.Usualy DataSource does I/O operations and it is time consuming operation. Accounting into this fact we can run DataSource always in the separated thread. Btw. DataSource should not know about threads etc. It would be great make this class tight cohesive. I think use case has to deal with threads. At least Usecase defines your business logic.
kingamajick commented 8 years ago

We initially looked at using this approach, and it works ok in fairly simplistic cases, but has some issues.

In our internal applications we end up with caching that looks like the following:

Memory Cache -> Disk (DB) Cache -> Network

If the data is in the memory cache, there is no reason to switch to another scheduler to complete the operation as it can be completed on the main thread. If the data needs to be retrieved from the bottom two layers, it does need to move to a different scheduler. By allowing the DataSources to be configured to use different schedulers, I believe allows for much more flexibility.

The other problem I see with putting the thread switching in the UseCases is it makes assumptions about how the layer below works internally. We assume that it needs to work on a background thread, yes this will likely be the case most of the time, but not always.

I hope this examples why we made the choice we did.

bkodirov commented 8 years ago

Thank you. I got you decision. Btw. If you think that context switching takes more resources you just can use one single thread in the UseCase. P.S. Dealing with schedulers in the DataSource makes it complex to write UnitTests(IMHO)

bkodirov commented 8 years ago

Btw. Looking forward to your push.