cesarferreira / rxpaper Goto Github PK

RxPaper contains 2 static fields to hold the instances of the current RxPaper and custom book you'd be using.

private static RxPaper mRxPaper;
private static String sCustomBook;

There are 2 issues with this setup which both relate to having multiple custom books (or preferably, instances of RxPaper)

Problem 1:

RxPaper.book("custom-book1").read(...)
RxPaper.book("custom-book2").read(...)
RxPaper.book("custom-book1").read(...)

This will instantiate 3 instances on RxPaper (3x overriding the same static field). So if i use 2 custom books i will create a lot of useless instances.

Problem 2:

I can not have multiple classes which each have a separate RxPaper with a custom book because of the above. The last class create will determine which custom book the will all share.

https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java#L50

https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java#L89

https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java#L104

An exception is an exception, and should come onError.

public Observable<List<String>> getAllKeys() {

    return Observable.create(new Observable.OnSubscribe<List<String>>() {
        @Override
        public void call(Subscriber<? super List<String>> subscriber) {

            if (!subscriber.isUnsubscribed()) {

                try {
                    List<String> keys;
                    if (hasBook()) {
                        keys = Paper.book(mCustomBook).getAllKeys();
                    } else {
                        keys = Paper.book().getAllKeys();
                    }
                    subscriber.onNext(keys);
                } catch (Exception e) {
                    subscriber.onError(new UnableToPerformOperationException("Can't collect all keys"));
                }

                subscriber.onCompleted();
            }
        }
    });
}

private static boolean hasBook() {
return !TextUtils.isEmpty(mCustomBook);
}

In my implementation I work with multiple books.
Problem: I don't know at runtime if a book has entries or not.
If I call "getAllKeys()" at a time where a book is empty I get all keys from default book.

I will overcome this problem by not using default book. However, I suggest to just return zero keys if a custom book is empty and not fall back to default book keys.

What is the need to pass a context to RxPaper.with(..) method, which initilalizes Paper.init(...) for every call?, In Paper docs states that Paper.init(context); must be called only once in Application.onCreate(...). I'm temporary workarounding it by creating another initializer, but I think the context is not necesary in any way.

https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java#L76

In case no value is found value will be null, which is not Observable.just friendly. Consider Observable.empty or Observable.error instead.

This is not really an issue, more of a question. As most people advocate against using Observable.create unless really needed. link

What are your opinions on this and why do you use Observable.create in RxPaper?

Being the #1 cause of leaks on Android, retaining a Context is rarely recommended. It is still required by the main observable, but it's never used, becoming a pointless pain point: https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java

Could you please release a new version with that change, and deprecating the old use?

Any plans to support RxJava2?

As I see in the code
https://github.com/cesarferreira/RxPaper/blob/master/rxpaper/src/main/java/com/cesarferreira/rxpaper/RxPaper.java#L43
write is performed immediately as RxPaper.with(ctx).write(key, value) is called. This should be doing in onSuscribe instead so disk i/o could be postponed and scheduled in the bg thread using Rx.