Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Design Review: Instance Concat/Merge/Flatten/Zip #687

Closed
benjchristensen opened this issue Dec 24, 2013 · 7 comments
Closed

API Design Review: Instance Concat/Merge/Flatten/Zip #687

benjchristensen opened this issue Dec 24, 2013 · 7 comments

Comments

@benjchristensen
Copy link
Member

An idea that has floated for the past year (including #295) is to add instance method for flatten. During API review this came up again along with instance methods for each of the combination operators: concat, merge/flatten, zip.

Because of how Java types work and without targeted extension methods we can not only make these apply to Observable<Observable<T>> and be type safe.

We could however do things like ...

If we have an `Observable<Observable>:

  • observable.flatten().cast(MyType.class)
  • observable.flattenTo(MyType.class)

If we have an Observable<T> it would basically just pass-thru without applying the flattening since it's already flat.

/cc @headinthebox and @jhusain

@zsxwing
Copy link
Member

zsxwing commented Dec 25, 2013

Another issue about the instance implementation, is the conflict of the static method signature and the instance one. For example, public static <T> Observable<T> concat(Observable<? extends Observable<? extends T>> observables) and public Observable<T> concat(Observable<? extends T> that).

@samuelgruetter
Copy link
Contributor

Given that there's import static: With such a change, you get

  • The ability to write `o.merge()` instead of `merge(o)`, `o1.zip(o2)` instead of `zip(o1, o2)`, `o1.concat(o2)` instead of `concat(o1, o2)`, which is just code cosmetics
    

But the price is high:

  • Lost typesafety
    
  • Several ways to do the same thing, bloats the API
    
  • If the programmer thinks an expression is an Observable of Observables, but in fact it's only an Observable, instead of getting an error, the case "just pass-thru without applying the flattening since it's already flat" applies. This is a source of confusion.
    

As you can see, I don't like the idea ;-)

@abersnaze
Copy link
Contributor

Would it be worth while to move all of the static operators to a different class (maybe something like ObservableOfObservables.java and ObservableOfNumbers.java) so that they won't clash with the instance revisions?

@benjchristensen
Copy link
Member Author

the conflict of the static method signature and the instance one.

It would not accept another Observable to flatten/merge/concat with, it is only for operating on the current Observable.

The intent of the instance methods is not for use cases such as o1.zip(o2). That is of little value and in fact confusing.

It is to flatten/merge/zip an Observable<Observable<T>>.

For example: o.flatten() or o.merge()

Thus the instance signature would be Observable<T> flatten() without arguments. (or Observable<T> concat(), Observable<T> merge() etc)

just code cosmetics

One could argue that about the entire Rx chaining pattern. Why have instance methods at all if code cosmetics was not a concern?

the case "just pass-thru without applying the flattening since it's already flat" applies. This is a source of confusion.

It offers support for interesting use cases where an Observable<Object> can return a mixture of T and Observable<T> and then be flattened into a single Observable.

Flatten specifically is considered for recursive flattening such that it could work on Observable<T>, Observable<Observable<T>> or even Observable<Observable<Observable<T>>> and a mixture of them.

Whether that's confusing or not is in the eye of the beholder ... I can argue that switch is not easy to understand and it is perfectly type-safe and does exactly what it says it will do. The flatMap operator is hard to understand for many (particularly those new to the functional style) yet flatten makes perfect sense.

Perhaps the "confusing" part could be reduced by only having a flatten instance method and leaving concat/merge/zip as static only. The flatten operator could be considered the recursive merge that will flatten whatever it is given whether it is already T or Observable<T> or Observable<Observable<T>>. For most people map().flatten() makes more sense to them than flatMap/mergeMap/concatMap.

Lost typesafety

It's little different than when Observable<Map<String, Object>> gets used to move data around, or Observable<Object>. The flatten() operator would not be something that can accidentally be used and break the static typing of any other operator by hitting an overload such as concat(Object, Object) so it's a developers choice to use it or not.

Several ways to do the same thing, bloats the API

It's doing something that static methods don't permit, particularly if we allow flatten() to be recursive.

I'm not convinced either way on whether this is a good or bad thing, but the following is preferable code:

o.map().filter().map().flatten().take()

than breaking the order of the chain into this:

flatten(o.map().filter().map()).take()

@samuelgruetter
Copy link
Contributor

@benjchristensen now it makes more sense ;-) Recursive flattening of heterogeneous Observables sounds interesting...

@benjchristensen
Copy link
Member Author

A reminder that this and #295 are related.

@benjchristensen
Copy link
Member Author

We now have mergeWith, concatWith, ambWith, zipWith, etc so closing this out. The flatten method is handled by #295.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants