Comments (7)
Just came across this in Google Rx MVVM example:
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.
from ground-android.
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.
from ground-android.
Note: This appears to be related to #67.
from ground-android.
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).
from ground-android.
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 doOn
s 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.
from ground-android.
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!
from ground-android.
Latest guidance is in our Wiki; let's identify these as we go, closing for now.
from ground-android.
Related Issues (20)
- [Draw area task] Location card obscures location lock button HOT 7
- [Capture location] Capture location button label truncated on some devices
- [Nav drawer] Survey title truncated at one line
- Incomplete draft surveys are visible in the app to the survey organizer
- Simplify data collection entry point HOT 1
- [Proposal / Proof of Concept] Protocol Buffers as a Firestore schema HOT 3
- [Code health] ktfmt-gradle and AS ktfmt plugin produce different results
- [Code health] Complete survey proto definitions HOT 1
- [Bug] - hasOtherOption nullcheck
- Add audit (creation/modified) info to survey metadata
- [Migration: protobuf for Firestore] Introduce new schema to app HOT 3
- Delete code which uses old schema
- [Geometry task] Prompt user to add point or draw perimeter when both allowed by organizer HOT 1
- [Code health] Add proto linter to `checkCode` Gradle target
- [Capture location] Altitude and accuracy probably not stored in Firebase
- [Capture location task] Location lock not on when first opened
- [Draw or walk perimeter] Instructions not shown on first use
- Upgrade Kotlin to 2.0 or higher
- Non-unique IDs for tasks in different jobs cause tasks to be overwritten in local DB
- Vulnerability training
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from ground-android.