Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using Observable.create without checking for isSubscribed and without an obvious reason #10

Open
akoufa opened this issue Jul 4, 2016 · 2 comments

Comments

@akoufa
Copy link

akoufa commented Jul 4, 2016

I have noticed that throughout the whole project and as an example in the PreferencesHelper.class you are using the Observable.create method without checking for isSubscribed. It is maybe not a problem in your exact usage pattern but as suggested from reowned Android Devs the usage of Observable.create should be avoided for various reasons. I think you can replace the Observable.create in most places with either Observable.just or Observable.fromCallable. There you get most of the thinks required from the Observable contract for free.

@ivacf
Copy link
Contributor

ivacf commented Jul 5, 2016

Yeah, that is right. We no longer use Observable.create() in our most recent projects and we usually use Observable.defer() or Observable.fromCallable() instead. However, we haven't changed it in this project yet. I'll leave this issue open se we can refactor it in the future.
More info about the topic here: https://artemzin.com/blog/rxjava-defer-execution-of-function-via-fromcallable/

@therajanmaurya
Copy link

therajanmaurya commented Jul 12, 2016

Hi @ivacf

In my case, I am getting data from the retrofit call and need to save in database, I was saving all the stuff in the background thread.

here the DataManager

public Observable<Page<Client>> getAllClients(boolean paged, int offset, int limit) {
        return mBaseApiManager.getClientsApi().getAllClients(paged, offset, limit)
                .concatMap(new Func1<Page<Client>, Observable<? extends Page<Client>>>() {
                               @Override
                               public Observable<? extends Page<Client>> call(Page<Client>
                                                                                      clientPage) {
                                   //Saving Clients in Database
                                   mDatabaseHelperClient.saveAllClients(clientPage);
                                   return Observable.just(clientPage);
                               }
                           });
    }

and DatabaseHelper methods

public Observable<Void> saveAllClients(final Page<Client> clientPage) {
        AsyncTask.THREAD_POOL_EXECUTOR.execute(new Runnable() {
            @Override
            public void run() {

                for (Client client : clientPage.getPageItems()) {

                    ClientDate clientDate = new ClientDate(client.getId(), 0,
                            client.getActivationDate().get(0),
                            client.getActivationDate().get(1),
                            client.getActivationDate().get(2));
                    client.setClientDate(clientDate);
                    client.save();
                }
            }
        });
        return null;
    }

Now I have made changes to my DataManager and DatabaseHelper

public Observable<Page<Client>> getAllClients(boolean paged, int offset, int limit) {
        return mBaseApiManager.getClientsApi().getAllClients(paged, offset, limit)
                .concatMap(new Func1<Page<Client>, Observable<? extends Page<Client>>>() {
                               @Override
                               public Observable<? extends Page<Client>> call(Page<Client>
                                                                                      clientPage) {
                                   //Saving Clients in Database
                                   return mDatabaseHelperClient.saveAllClients(clientPage);
                               }
                           });
    }

and DatabaseHelper

public Observable<Page<Client>> saveAllClients(final Page<Client> clientPage) {
        return Observable.defer(new Func0<Observable<Page<Client>>>() {
            @Override
            public Observable<Page<Client>> call() {

                for (Client client : clientPage.getPageItems()) {

                    if (client.getActivationDate().size() != 0) {
                        ClientDate clientDate = new ClientDate(client.getId(), 0,
                                client.getActivationDate().get(0),
                                client.getActivationDate().get(1),
                                client.getActivationDate().get(2));
                        client.setClientDate(clientDate);
                    }
                    //Using DBFlow for saving clients
                    client.save();
                }
                return Observable.just(clientPage);
            }
        });
    }

First I want to ask
my first approach is wrong or not because it's making concurrentmodificationexception, if the user is scrolling the list very fastly ?

In Second approach I am saving Clients into Database and then giving back data to datamanager to give to the presenter.

Is it good as compare to First ?

and Please tell me if I have implemented Observable.defer in a wrong way.

Here the architecture I have Implemented in which there is two mode Online and Offline.
In Online mode data will be loaded from the server and saved in database.
and User have option to switch the mode. If user will switch to offline mode data will loaded from the database.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants