[Code health] Rename members of `LocalDataStore` classes

[gino-m]

trafficstars

To improve readability, recommending the following changes to LocalDataStore. Sharing initial thoughts here:

• surveyStore -> surveys
• userStore -> users
• etc.

Then, in each local data store, functions can exclude the qualifier (survey, locationOfInterest, etc), since they data store name already qualifies them. Also, we should be consist and use meaningful verbs to clarify what each fun does. For ex, in LocalSurveyStore:

• surveys() -> loadAll()
• getSurveyById() -> loadById()
• etc

Lastly, replace surveyDataStore alias with direct reference to localDataStore.surveys to remove ambiguity.

Example usage:

• localDataStore.surveys.loadAll()

@scolsen Wdyt?

[scolsen]

removing the redundancy sgtm -- I suggest we be more explicit about load ops though. Room basically has two types of operations:

• Load and stream changes on all updates
• Single-shot load (don't stream subsequent changes)

The first one can be further tweaked by using distinctUntilChanged. By default, the stream will emit a new item whenever the underlying table changes at all. Using distinctUntilChange alters the behavior so that it only emits when the dataset that's actually in the stream (e.g. filtered by id etc.) changes and not on all table changes.

wdyt about:

• loadBy<x> -- for single shot retrieval of a single row (e.g. Single<T>, Maybe, Suspend fun (for coroutines)).
• loadAll -- for single-shot retrieval of multiple rows in a table (e.g.Single< List<T>>).
• loadAllAndStream -- for load + stream on any table change (e.g. Flowable, Flow<> (coroutines)).
• loadAllAndStreamUpdates-- for load + stream only on relevant data changes (distinct until changed)

Along similar lines, I wanted to refactor the stores into a general interface of load operations -- but at the moment this isn't really feasible since loads require different sorts of arguments -- it might be more possible as we merge repositories and (e.g.) no longer need to pass both a survey and ID for LOIs, for instance (instead we can have a generic loadById)

[gino-m]

Sgtm! Not sure we need to distinguish between loadAllAndStream and loadAllAndStreamUpdates, since I don't think there are any cases where want to emit values on non-changes; all our uses would end up being loadAllAndStreamUpdates.

Along similar lines, I wanted to refactor the stores into a general interface of load operations

I think this is expected and acceptable, since we have different needs from each entity store. A common interface might also introduce methods that we don't need, and wouldn't offer any functional benefits since we never treat different entity stores polymorphically (e.g., load all the entities from store X). A clear, concise, and consistent naming convention will probably provide the most benefit here.

[scolsen]

sgtm! I like the policy of just streaming updates -- we'll have to make sure all our DAO streams have distinctUntilChanged (at least for coroutines, i forget off the top of my head if the RX variants work the same but I assume they do)

[gino-m]

sgtm! I like the policy of just streaming updates -- we'll have to make sure all our DAO streams have distinctUntilChanged (at least for coroutines, i forget off the top of my head if the RX variants work the same but I assume they do)

Room doesn't do that for us, at least when we're fetching the full entity rather than specific fields? If not, what does it mean for a table to be changed?

[scolsen]

At least for the coroutines Flow impl, Room will re-emit on any table state change (update/delete/insert):

Whenever any changes are made to the table, independent of which row is changed, the query will be re-triggered and the Flow will emit again. So if Frida gets updated, we will receive the latest info. However, this behaviour of the database also means that if we update an unrelated row, like Bandit, our Flow will emit again, with the same result: (Frida, 11, 3). Because SQLite database triggers only allow notifications at table level and not at row level, Room can’t know what exactly has changed in the table data, therefore it re-triggers the query defined in the DAO. In your code, use Flow operators like distinctUntilChanged, to ensure that you only get notified when the data you’re interested in has changed:

(https://medium.com/androiddevelopers/room-flow-273acffe5b57)

Unfortunately the RX Java article on Room doesn't include these details, but I wouldn't be surprised if it worked the same way for Flowable. So, if we only want re-emissions to happen when the row we're interested in is updated, we need to call distinctUntilChanged.

[gino-m]

Interesting, thanks for clarifying!

[gino-m]

Closing in favor of more conservative #1491.