[Code health] Verify usage of Rx side effects

[scolsen]

There are some instances in the code in which we rely on doOnNext, doOnSuccess and other RX side effect operators.

In general, we should either avoid side effects entirely or ensure their usage is carefully prescribed.

Personally, I like treating anything that does not have to do with the direct manipulation of data (emitted items) as a side effect. For example, logging and UI changes would both be considered side effects from this perspective. I think this approach enables us to maintain clear separation between the actual 'transformation'/ data-manipulating code and 'side-effecting' code like logging.

For our purposes, we only need to treat logging as a side-effect, since display related code is handled by fragments (and ideally live-data binding). The majority of cases in which we manipulate some piece of data in a doOn function can be expressed using transformations instead.

@gino-m

[gino-m]

In general, we should either avoid side effects entirely or ensure their usage is carefully prescribed.

Agree 100%. We should work towards this as we continue to implement functionality for Alpha/MVP version. Since it's not blocking, perhaps we can roll into ongoing work, and be mindful of this going forward.

Personally, I like treating anything that does not have to do with the direct manipulation of data (emitted items) as a side effect. For example, logging and UI changes would both be considered side effects from this perspective.

Not sure about the UI part. The sole purpose of some Observables is for the UI to watch and react to changes in the view model state. If that's the case, it's hard to explain why some UI components would react to the stream, while others would be altered by side-effects.

[gino-m]

Note: This appears to be related to #67.

[scolsen]

Not sure about the UI part. The sole purpose of some Observables is for the UI to watch and react to changes in the view model state. If that's the case, it's hard to explain why some UI components would react to the stream, while others would be altered by side-effects.

You're right! Android's architecture and binding/livedata stuff effectively makes handling UI a non-issue for us and is the better approach.

So really, the only time we might want to use side effects are for logging purposes.

I also think we should avoid using side effects for kicking-off data manipulations that are external to the given stream. For example, in the EditRecord VM there are currently some side effects on stream S that, when called, set the contents of a property P on the VM--we can instead express this property's value as a stream transformation on A (and eventually as live data)--this it will still trigger the desired changes at the same time as the side-effecting set up (as values are emitted downstream).

[gino-m]

Just came across this in Google Rx MVVM example:

https://github.com/googlesamples/android-architecture/blob/2e20b4a5deffb354f5ffa01c7f4448fd37acb6c3/todoapp/app/src/main/java/com/example/android/architecture/blueprints/todoapp/tasks/TasksViewModel.java#L77-L84

They're using side effects for loading state indicator as well as errors. Arguably more readable than using a wrapper class, esp. since there's no downside to doing so in this particular case afaict.

[scolsen]

The state-indicator handling is pretty nifty! I'm sure there are a lot of places where we could similarly fire off a subject on subscribe.

I agree that the example is very readable--it also seems to adhere to the idea of keeping non-emissions altering code in do operators--this is what I was driving at in the OP on this issue.

IIRC (which I may well not), at the moment there are a couple of places where we tuck away some object field changes (like set mutable live data) or other 'side-effects' into a mapped function on the stream--really it should be separated out into a separate call in a doOnNext operator, to keep the logic that alters the stream emissions separate from logic that only uses those emissions to do something else (but doesn't actually change the stream).

More concretely, we should refactor cases like:

Int setAndToInt(String  x) {
  this.stringLiveData = x;
  return x.toInt;
}
someStream.map(this::setAndToInt)

to something like this instead:

someStream
  .doOnNext(x -> this.stringLiveData = x)
  .map(x -> x.toInt);

That keeps our mapping functions "pure", i.e. free of side-effects, where side effect is more or less any code that amounts to void, like setting a field on an object, logging, etc. Moving these sorts of operations out into doOns makes it clearer precisely what's going on when you look at a stream, whereas tucking this logic into mapping functions forces readers to look at the implementation to see when and where the side-effects happen.

This may be a non-issue. At this point I only have a vague memory of seeing these cases in the codebase, so they may not actually exist, lol.

At any rate, I think dropping our Result wrapper was the right move.

[gino-m]

I agree. Let's keep this in mind going forward.

I'm not 100% convinced we should remove Resource in favor of side-effects, but we can cross that bridge if and when we get to it.

Thanks for the comments!

[gino-m]

Latest guidance is in our Wiki; let's identify these as we go, closing for now.