RXAndroid better way to write nested subscriptions

Question

I'm working with an RXBle library for Android. In order to read from a BLE peripheral (BLE device) I end up setting up a number of subscribes, all within one another, like this:

    Disposable scanSubscription = null;

    public void startScan() {
        scanSubscription = rxBleClient
            .scanBleDevices(settings, filter)
            .take(1)
            .subscribe(
                scanResult -> connectDevice(scanResult.getBleDevice()),
                throwable -> {}
            );
    }

    public void connectDevice(RxBleDevice device) {
        device
            .establishConnection(false)
            .subscribe(
                connection -> requestMtu(connection),
                throwable -> {}
            );
    }

    public void requestMtu(final RxBleConnection connection) {
        connection
            .requestMtu(185)
            .subscribe(
                mtu -> readCharacteristic(connection),
                throwable -> {}
            );
    }

    public void readCharacteristic(final RxBleConnection connection) {
        /* this one eventually scanSubscription.dispose!!!!! */
    }

Essentially, I have a series of functions that perform an action on a thing, and then subscribe to the resulting Single (I think I'm using the terminology correctly)

My question is, what is a better way to write this?

Even more importantly, what happens to the nested subscribes that I never unsubscribe from? Note that in the first function I'm calling a take(1)...eventually I just dispose the top-level disposable that is calling all of this.

idunnololz · Accepted Answer · 2018-10-09T00:34:41.600

1

No. That is not the RX way. I recommend giving this a quick read.

The tl;dr is that you want to lay your code out so that it flows like a stream. One of the things that Rx tries to address is to eliminate nested callbacks. This "stream" approach does that.

Now onto what you can do to make it better. Here is one approach to rewriting your code so that it's a stream instead of a group of nested callbacks:

Disposable scanSubscription = null;

public void doThing() {
    scanSubscription = rxBleClient
        .scanBleDevices(settings, filter)
        .take(1)
        .map(scanResult -> scanResult.establishConnection(false))
        .map(connection -> connection.requestMtu(185))
        .map(mtu -> <do thing>)
        .subscribe(/* do things with final value */)
}

Here is another approach:

Disposable scanSubscription = null;

public void doThing() {
    scanSubscription = rxBleClient
        .scanBleDevices(settings, filter)
        .take(1)
        .flatMap(scanResult -> scanResult.establishConnection(false))
        .flatMap(connection -> connection.requestMtu(185).map(result -> Pair(connection, result)))
        .flatMap(result -> {
            Connection c = result.first;
            Mtu mtu = result.second;
            //do thing
        })
        .subscribe(/* do things with final value */)
}

edited Oct 09 '18 at 00:34

answered Oct 08 '18 at 23:35

idunnololz

8,058
5
30
46

@idunnoloz thanks for the response. The part I got a little confused about was how I could access `connection` from the part in the code where you said "do things". Any advice? I need to read from the connection once the MTU has changed. – wheresmycookie Oct 09 '18 at 00:19
1

@wheresmycookie Depending on how "clean" you want to be you can either have `map()` return a pair like so: `.map(connection -> new Pair(connection, connection.requestMtu(185)))`. Or you can create a result class with the fields you want to pass down the stream and return that instead. – idunnololz Oct 09 '18 at 00:25
1

@wheresmycookie I updated my answer to give you another way you can tackle the problem if you need to pass the Connection down the stream. – idunnololz Oct 09 '18 at 00:34
Ah, great! That's what I was looking for! One more thing and then I'm content to upvote this solution: suppose every few minutes I wanted to call `doThing()` which in your example would be removing the current subscribe and "replacing" it with a fresh version of itself. (I know it's a weird request but I have to reset the scanning occasionally due to constraints in the OS). Would it be sufficient to just call `dispose()` and then `doThing()` again in a delayed loop? – wheresmycookie Oct 09 '18 at 00:34
1

@wheresmycookie it depends. `dispose()` will set the interrupt flag for all of the threads that are running within your stream so you need to make sure that your code can handle interrupts and errors correctly. But assuming that you do handle interrupts gracefully then yes you can just do that. You do not need to do this with a "delayed loop" unless you NEED to ensure execution has stopped. As soon as `dispose()` is called, your consumer will stop receiving results. – idunnololz Oct 09 '18 at 00:43
this has been really useful. Looking forward to learning more about Rx.. I just cut about 100 lines out of my pre-Rx code. Thanks! – wheresmycookie Oct 09 '18 at 00:48

RXAndroid better way to write nested subscriptions

1 Answers1