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

Operations Aggregate, Average and Sum with selector #657

Merged
merged 1 commit into from
Dec 27, 2013

Conversation

akarnokd
Copy link
Member

Issue #653

Remarks:

  • I know we can combine ops to get one of the new aggregate variant, but I think it might be worth having a direct version which avoids nesting several layers of Observables, Observers and Subscriptions.
  • The averageInteger and sumInteger (and the other types) are handy if we want to use chained operation invocations (with less overhead):
Observable.from("a", "bb", "ccc").sumInteger(s -> s.length())
    .toBlockingObservable().single();

instead of

Observable.sumIntegers(Observable.from("a", "bb", "ccc").map(s -> s.length()))
    .toBlockingObservable().single();

@akarnokd akarnokd mentioned this pull request Dec 23, 2013
25 tasks
@cloudbees-pull-request-builder

RxJava-pull-requests #590 SUCCESS
This pull request looks good

@benjchristensen
Copy link
Member

This establishes (or strengthens) a precedent of adding a large number of helper methods for very specific cases to the already massive Observable so I'd like to explore one of two routes:

  1. Move mathematical operators to a separate Observable in rx.observable.NumericalObservable or something like that.
  2. Move them into a contrib module like https://github.com/Netflix/RxJava/blob/master/rxjava-contrib/rxjava-string/src/main/java/rx/observables/StringObservable.java

@akarnokd
Copy link
Member Author

I'd go for NumericalObservable on this one, as it is primarily useful out-of-box whereas Futures may go into contrib.

* @return an Observable that aggregates the source values with the given accumulator
* function and projects the final result via the resultselector
*/
public <U, V> Observable<V> aggregate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is equivalent to:

reduce(seed, accumulator).map(resultSelector)

Why the need for a separate operator and implementation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we can combine ops to get one of the new aggregate variant, but I think it might be worth having a direct version which avoids nesting several layers of Observables, Observers and Subscriptions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nesting several layers of Observables, Observers and Subscriptions.

The entire library is based on composition. If we were seeking to avoid that we wouldn't be using a functional style so unless there is a strong performance reason (that isn't a bug with one of the operators themselves) I don't see that as a reason to add a helper operator.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay.

@benjchristensen benjchristensen merged commit 14b701d into ReactiveX:master Dec 27, 2013
@akarnokd akarnokd deleted the AggregatorsWithSelector branch January 13, 2014 09:57
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
jihoonson pushed a commit to jihoonson/RxJava that referenced this pull request Mar 6, 2020
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

Successfully merging this pull request may close these issues.

3 participants