[Code health] Consistent naming of Rx streams

[gino-m]

@scolsen listened to this podcast after our chat. I agree with most of the suggestions there. Key takeaways (mixed with my own observations):

• There's no single convention used by everyone, but there are some prevailing trends.
• The getFoo() pattern is dated, a relic of JavaBeans naming conventions.
• Consistent naming >> relying on return types as documentation:
• Doesn't require developers to look at declaration to understand semantics.
• Prevents signatures from collide if we add an override that returns a different Rx type.

Some ideas/suggestions:

• Use foo() for immutable synchronous accessors (e.g. User user()).. There's also precedent for this in JDK methods like values().
• fooOnce() for Rx types that return a single value and complete (usually Single<>).
• fooStream() for Rx types that are expected to not complete while the application is in a normal state. fooOnceAndStream()` for Rx types that return a single value (the latest?) ASAP and then continue to stream updates while the application is in a normal state.

A similar approach is also described here.

Wdyt? No need to change existing uses now, but new code could use the convention, and we can update existing code once V0 is ready.

@navinsivakumar FYI.

[scolsen]

I like this proposal! If I understand correctly, here's a few examples of how these names should map to RX types:

• If we return an Observable<Foo> that does not complete, the reference/method should be named fooStream
• If we return a Single<Foo>, Maybe<Foo>, or an Observable.just(foo) that emits one value and completes, the reference/method should be named fooOnce
• If we return a BehaviorSubject<Foo>, the reference/method should be named fooOnceAndStream

What about Completable<Foo>, which emits no items but can either complete or error? Will fooOnce suffice for this case, or should it be distinguished, e.g. as fooComplete or fooEmpty? I prefer fooEmpty since Maybe.empty() and Observable.empty() are both instances of this case.

I don't think we have an instance of this case yet, but we could also consider Observables that return N items then complete, for example, Observable.range(1, 5). How about fooMany for this case?

Adopting the original set and the additional names mentioned in this post, we'd have:

• foo
• fooStream
• fooOnce
• fooOnceAndStream
• fooEmpty
• fooMany

[gino-m]

Yep, that's the gist of it, except I [think I] also agree with the above linked article where it says:

Using these naming conventions with Observables and Flowables improve code clarity, but they should not be used with other Rx types like Single, Completable, and Maybe.

...since the "Once" and "Stream" concepts seem to apply more to the former two types but not the latter three. Perhaps a more realistic example would help illuminate things:

• User currentUser()
• Observable<User> userStream()
• Observable<User> userOnceAndStream()
• Single<User> loadUserSingle(String id)
• Maybe<User> loadUserMaybe(String id)
• Completable authenticateUserCompletable(User user)

Two observations / questions:

1. The method names appear inconsistent and therefore less readable without the get prefix, since some are verbs and some are nouns. Do you think feel the same?
2. Do the asynchronous one-time actions that return Single, Maybe, or Completable really require disambiguation? Also, how likely would a signature collision be (e.g. why would we ever have a method w/the same name w/the same signature that returns either a Single or a Maybe)?

Assuming the answers to the above are "yes", "no", and "unlikely", respectively, we'd get:

• User getCurrentUser()
• Observable<User> getUserStream()
• Observable<User> getUserOnceAndStream()
• Single<User> loadUser(String id)
• Maybe<User> loadUser(String id)
• Completable authenticateUser(User user)

These read more like natural language imo. Do you think they're unambiguous enough?

[scolsen]

I don't mind either style, so I'm totally fine with the second proposal:

• User getCurrentUser()
• Observable<User> getUserStream()
• Observable<User> getUserOnceAndStream()
• Single<User> loadUser(String id)
• Maybe<User> loadUser(String id)
• Completable authenticateUser(User user)

It seems very clear to me.

So more than likely we'll never actually need the Once suffix, since that would equate to an Observable that returns a single value then errors or completes, in which case we should just use a Single or Maybe as I think those cover the possible usage -- except for maybe some random case in which we need a flowable that emits once with backpressure (but why? lol)?

I would still like to have some convention for streams that emit a finite number of items N and are expected to complete--if we wind up needing it. I'm fine with waiting to adopt that until it actually comes up in practice, though, since thus far it seems to be a non-issue.

[gino-m]

I think Once might get used on private helper methods that return Observable instead of Single or Maybe for convenience, but probably not elsewhere.

I think streams that emit N items will be rare. More often, I think we'll end up with a Single<List<>>. Agree we can cross that bridge if/when we get to it. Closing this issue for now and opening another one for moving best practices to Wiki.

[gino-m]

Actually we still need to apply this to existing code. Reopening.

[gino-m]

One other thing; we should also take care when naming fields referencing Subjects backing these streams. It may be tempting (as I did in my previous deleted comment) to name them according to the method convention:

• PublishSubject<User> userStream;
• BehaviorSubject<User> userOnceAndStream;

But recall that the "Once" part is a function of the streams' behavior, not their underlying type. The first stream could easily be set up to emit a value immediately when the method is called. For that reason, it might be better to just name them for the fact that they're subjects:

• PublishSubject<User> userSubject; or
• BehaviorSubject<User> userSubject;

This also makes it clear that values can be emitted via those fields by calling userSubject.onNext().

Completable, Maybe, and Single will rarely be used as fields, but I assume we would follow the same convention as for method names (method getUser() returns field user).

[scolsen]

Should we also adopt a policy that all subjects should be class local variables and last the duration of the lifetime of the encapsulating class?

If possible, and this seems to be reasonable for all the cases thus far, I think we should avoid returning subjects. They've primarily been useful as bridges of sorts between imperative and reactive code or between user interactions and reactive business logic. If we can ensure subjects only fill this role of liaison in their encapsulating classes and aren't used outside of the class, we can be extremely confident about when their reactive methods onNext etc. are called. This makes it easy to reason about their usage in a particular case, and I think it's beneficial to keep such "bridging" code as simple as possible.

[gino-m]

Yes, I think the point of the Subject is a bridge between imperative and functional reactive worlds. Subject is just a specific implementation of an Observable that make it "mutable". We shouldn't be exposing these any more than we should expose ArrayList and not List. Not sure if we need an explicit rule for this or if it's implicit in that we should only expose Observable, Single, etc. streams.

[gino-m]

Hi @scolsen! Reviving this discussion and adding one observation (no pun intended 😹). I've noticed that we (mainly I) have applied functional and imperative styles inconsistently throughout Ground, meaning that in some cases logic in the method is executed immediately when invoked, and in others it's executed on subscribe of the returned observable. This has already led to bugs which I've addressed as they arose on a case-by-case basis, but I suspect there are many more cases like this one, for example:

https://github.com/google/ground-android/blob/7a02a819e7d6cc1f90f2f8cc19c3d4f5550a1db0/gnd/src/main/java/com/google/android/gnd/repository/DataRepository.java#L202-L217

This may not be something we need to address via naming convention, though; should we just say that all functions that return streams should never executed logic in their body, but always in observables in the stream?

@shobhitagarwal1612 @deepakbansal FYI

[scolsen]

Ah! Great point to raise! Definitely something we should address. I think the right approach is actually to just fix all the function bodies so that we don't mix eager/lazy eval rather than use a naming convention. Generally speaking I'm also a fan of eliminating as much of the imperative code that we can (right now we do have quite the amalgam of functional/imperative style). wdyt?

[gino-m]

Agreed! Filed #204 to track.

[gino-m]

In the Icebox until we can figure out whether to migrate to Flow.

[gino-m]

Moving to Coroutines so this is obsolete.