-
Notifications
You must be signed in to change notification settings - Fork 40
Added an aggregate operator without a seed #3
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
base: master
Are you sure you want to change the base?
Conversation
675e728 to
dbcd01e
Compare
Added range_of_keys method to lookup_range so that keys can also be iterated
|
Hi. Been a bit busy. I am planning to take a closer look at this the coming week. |
CppLinq/cpplinq.hpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Predicate => Selector is fine (correct even) but in general template parameters are prefixed with 'T'. Any special reason to remove it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A miss, sorry. That parameter should be TKeySelector.
|
So a few comments. Overall I think it looks good and should be mergable. Have you done code coverage runs to see the code-coverage of the new code? I guess I really should add some proper CI... |
|
Sorry to inject my two cents late in the game. This is similar to LINQ Aggregate? That's with/without a seed? For instance, without, like gathering some std::string elements? Or with, like doing a summation, multiplication, or division, and injecting either 0 or 1, for example? |
|
@mwpowellhtx |
|
@mrange |
|
@zwvista I understand, without seed is what you did. What is the "default seed"? I think users need/want the ability to decide for themselves what that seed should be. I know I do: i.e. in the couple of instances I explained above; contrast multiply/divide with add/subtract, etc, etc. Zero (0) would not work for multiply, where as One (1) is probably inappropriate for add. Make sense? |
|
@zwvista I just got through putting the C++ transform numerics range algorithm, as a matter of fact, along similar lines. Have a look at, transform as a frame of reference. The output type is the seed, and the binary operator accepts an Output and and Input, and returns an Output. Along the same lines as LINQ Aggregate. |
|
@mwpowellhtx And the C++ algorithm function that does the same work as Aggregate in LINQ is not transform. It's accumulate, if I'm not mistaken. The accumulate function in C++ always comes with a seed(init) value. We don't have an accumulate without seed in the standard library. But that doesn't mean an accumulate without seed is useless. |
|
@zwvista That's not the point. I use addition or multiplication as illustrations only. The point is that the end-user should be telling aggregate what the seed should be, not the other way around. I'll grant you there is one version of LINQ Aggregate that does not specify seed. Fair enough. However, there is another version that does, and should, accept a seed. Hope this helps. Moving on... |
|
@mwpowellhtx In an aggregate with a seed, the return type is same as that of the seed. Without a seed, there is still no problem in deciding the return type of the algorithm. Just take the type of the element of the sequence, and it's done. There is no such algorithm/overload in the standard library of C++ but as you know, there is one in LINQ. So is there in F# and Java 8. Obviously Human Interface is the approach taken in the API design in LINQ and cpplinq. That is, to provide as many algorithms/overloads as possible to facilitate the use of the library. Surely you can do without an aggregate without a seed, but you can also do without many of the APIs here.The aggregate without a seed is shorter and will be useful in some context. That's the key point of the Human Interface. That's also my two cents. |
|
@zwvista If you're going to provide an aggregate extension, along the same lines as LINQ, and not dissimilar to the C++ range-based transform function, then I think you provide both: one with seed, one without. I tend to prefer with seed, because it's not up to the API to dictate what that result should look like, that's up to the end-user to decide that. @mrange As repository moderator, will defer to you on this one. I've stated my peace and will leave it at that. I've said my peace. What you do with it is up to you. Thank you... |
|
@mwpowellhtx |
…perator - the aggregate without a seed
|
In order to simplify the interface, I've removed the reduce operator and replaced it with a third overload of the aggregate operator. So now the aggregate in cpplinq is just like its LINQ counterpart. |
|
Ah @zwvista then it is I who have misunderstood. |
I've implemented the following operators
aggregate without a seed
last
element_at
distinct with a key selector
start_with
to_lookup with a value selector