Add max() genSequence #3

sea1jxr · 2017-04-13T05:59:03Z

I normalized all the non-values (null, NaN, and undefined) to be undefined so I can return them as a part of Maybe.

MDN shows the behavior of Array.prototype.reduce throws an Error when the set is empty. I notice the reduce in this library doesn't do that, but I implemented it to get your thoughts on it.

I struggled to figure out where to put the method in the list of methods. I ended up putting it next to other methods that don't take parameters. That doesn't seem like a great choice but I didn't see a better pattern. I wonder if we should alphabetize them all instead?

coveralls · 2017-04-13T06:00:15Z

Coverage increased (+0.05%) to 99.743% when pulling 1d60c1d on sea1jxr:max into 6c4a8ee on Jason3S:master.

sea1jxr · 2017-04-14T17:11:56Z

@Jason3S Just pinging to see if you are going to be able to look at this soon

Jason3S · 2017-04-14T19:23:10Z

Thank you for the reminder.

Jason3S

Thank you for doing this.

Please see inline comments.

Please simplify it to do only one thing.

Optional: add an optional comparison function.

Jason3S · 2017-04-14T20:34:05Z

src/GenSequence.ts

+
+    return value;
+}
+


My vote in this case is to keep it simple. .max() should do only one thing.

export function max<T>(i: Iterable<T>): Maybe<T> { return reduce((p: T, c: T) => c > p ? c : p, undefined, i); }

Thoughts:

Empty lists: the max on an empty list is really just undefined and not an error. I can see why it might be tempting to throw an exception after looking at Array.reduce(). In the case of Array.reduce(), the exception is only thrown in the cases where no default is given.

Non-values: it is the job of the programmer to filter these out before calling max. This is very easy to do:
const seq = genSequence([undefined, null, NaN, 1, 3, 4]); const m = seq .filter(v => v != null && v === v) .max()

one nice addition might be to allow the user to use their own comparison function: max<T>(gt?: (a: T, b: T) => boolean). This is useful when you wan to find the "oldest" person or the "most expensive" item.

The C# linq expression exposes a transform (map basically), but it seems like it just gives you the equivelent of this:

const seq = genSequence([{name="joe", age=15}, {name="nancy", age=43}]); const m = seq .map(v => v.age) .max()

That leaves you with m === 43, but you don't know which object that is

What you proposed is better because you get the object back and know all the rest of the information about the object. Another (slightly cleaner?) way to do what you were proposing might be
max<T, U>(valueSelector: (T) => U): T
so you could do this

const seq = genSequence([{name="joe", age=15}, {name="nancy", age=43}]); const m = seq .max(v => v.age)

so m.age === 43, and m.name === "nancy".

The version you spec'd would look like this:

const seq = genSequence([{name="joe", age=15}, {name="nancy", age=43}]); const m = seq .max((c, p) => c.age > p.age)

And would have some strange side effects in that you could use the max() funciton to find the min value if you write the gt function backwards.

Thoughts?

Good point.

Your proposal is easy enough to read:

const seq = genSequence([{name="joe", age=15}, {name="nancy", age=43}]); const m = seq .max(v => v.age)

Yet, how would you compare a vector:

const seq = genSequence([{x=45, y=2}, {x=45, y=10}]); const m = seq .max((p, c) => c.x > p.x || (c.x == p.x && c.y > p.y))

Of course, if someone wants to do something that complicated, they can write their own reducer.

My vote is to go with your suggestion. It is less powerful, but easier to read and get correct. I think users of the function will find it more useful in the general case.

Hmm, ok I implemented it the very clean way you mentioned, and it looks great, however the tests expose that it works normally if you start with a real value, but if you start with a non value (NaN or undefined) first, the non value gets "stuck" and that is what you end up getting. I fear that folks will try it with non values in the sequence and get the impression that they are ignored, but then find themselves with a non value in the starting position in production and have it fail them.

it('test max starts with NaN always NaN', () => { const values = [NaN, 1, NaN, 2]; expect(genSequence(values).max()).to.be.NaN; }); it('test max NaN value', () => { const values = [1, NaN, 2]; expect(genSequence(values).max()).to.equal(2); });

Are you ok leaving this slightly misleading behavior? I can be ok with it.

Jason3S · 2017-04-14T20:42:19Z

src/GenSequence.ts

@@ -27,6 +27,7 @@ export interface Sequence<T> extends IterableLike<T> {
    any(fnFilter: (t: T)=> boolean): boolean;
    first(fnFilter?: (t: T)=> boolean, defaultValue?: T): Maybe<T>;
    first(fnFilter: (t: T)=> boolean, defaultValue: T): T;
+    max(): Maybe<T>;


When the list is small, the order doesn't really matter. As you pointed it out, as it grows, a logical order become necessary.

I would say group by type: Reducers, Filters, Mappers; and then alphabetically.

If I understand correctly that boils down to return type.

Reducers: Maybe
Filters: Sequence
Mappers: Sequence

Which is kind of what I was thinking as the first level grouping also.

It isn't really return time.

Reducers always end the sequence.
Mappers change the data being passed along.
Filters drop out unused values.

For example:

skip, filter, and take are a form of filter

any, max are reducers.

map, scan are mappers.

first is a combination of a filter and a reducer. But since it ends the sequence, I consider it to be a reducer.

I started to rearrange things based on this order, but after looking at it, I will send out a separate pull request that is just re-ordering so it is easier to review.

coveralls · 2017-04-22T23:56:40Z

Coverage increased (+0.05%) to 99.744% when pulling f6fcd41 on sea1jxr:max into 6c4a8ee on Jason3S:master.

coveralls · 2017-04-23T00:39:58Z

Coverage increased (+0.05%) to 99.744% when pulling d8259f3 on sea1jxr:max into 6c4a8ee on Jason3S:master.

coveralls · 2017-04-23T00:40:04Z

Coverage increased (+0.05%) to 99.744% when pulling d8259f3 on sea1jxr:max into 6c4a8ee on Jason3S:master.

sea1jxr · 2017-05-03T14:59:08Z

@Jason3S Just pinging to see if there are further changes needed.

Jason3S · 2017-05-04T08:40:53Z

Thank you for the reminder, I didn't see your changes. I'll take a look today.

Jason3S

Nicely done!

Jason3S · 2017-05-04T08:49:18Z

src/GenSequence.ts

@@ -282,11 +287,16 @@ export function first<T>(fn: (t: T) => boolean, defaultValue: T, i: Iterable<T>)
    return defaultValue;
 }

+export function max<T, U>(fn: (t: T) => U, i: Iterable<T>): Maybe<T>;
+export function max<T>(fn: Maybe<(t: T) => T>, i: Iterable<T>): Maybe<T> {
+    const selector = fn || ((v) => v);


nit: Let's try to avoid extra ().

const selector = fn || (v => v);

fixed by the previous comment fix.

Jason3S · 2017-05-04T08:54:00Z

src/GenSequence.ts

@@ -282,11 +287,16 @@ export function first<T>(fn: (t: T) => boolean, defaultValue: T, i: Iterable<T>)
    return defaultValue;
 }

+export function max<T, U>(fn: (t: T) => U, i: Iterable<T>): Maybe<T>;
+export function max<T>(fn: Maybe<(t: T) => T>, i: Iterable<T>): Maybe<T> {


How about?

export function max<T>(fn?: (t: T) => T, i: Iterable<T>): Maybe<T> {

or

export function max<T>(selector: (t: T) => T = (t => t), i: Iterable<T>): Maybe<T> {

Love the last one, and it fixes the issue below also :)

Jason3S · 2017-05-04T09:04:17Z

Thank you for the pull-request.

sea1jxr · 2017-05-07T04:40:23Z

@Jason3S The pull request has been updated with the changes you requested.

coveralls · 2017-05-07T04:41:04Z

Coverage increased (+0.05%) to 99.743% when pulling 3b20fd5 on sea1jxr:max into 6c4a8ee on Jason3S:master.

Jason3S · 2017-05-10T09:19:47Z

Very nice. Thank you. Nice and clean.

Jason3S · 2017-05-10T09:20:07Z

I'll publish later today.

sea1jxr · 2017-05-10T16:40:56Z

Thanks. You may want to wait and publish after min goes in.

sea1jxr added 2 commits April 12, 2017 22:19

Initial Implementation

0881821

Implemented as reduce

1d60c1d

Jason3S requested changes Apr 14, 2017

View reviewed changes

Add max with selector, and use the simple implementation of max

f6fcd41

sea1jxr added 2 commits April 22, 2017 17:37

Update the readme

0e020f4

tweak the readme

d8259f3

Jason3S requested changes May 4, 2017

View reviewed changes

update based on review comments

3b20fd5

Jason3S merged commit 438a3ae into streetsidesoftware:master May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add max() genSequence #3

Add max() genSequence #3

sea1jxr commented Apr 13, 2017

coveralls commented Apr 13, 2017 •

edited

Loading

sea1jxr commented Apr 14, 2017

Jason3S commented Apr 14, 2017

Jason3S left a comment •

edited

Loading

Jason3S Apr 14, 2017

sea1jxr Apr 14, 2017

Jason3S Apr 18, 2017

sea1jxr Apr 22, 2017

Jason3S Apr 14, 2017

sea1jxr Apr 14, 2017

Jason3S Apr 18, 2017

sea1jxr Apr 22, 2017

coveralls commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 23, 2017 •

edited

Loading

coveralls commented Apr 23, 2017 •

edited

Loading

sea1jxr commented May 3, 2017

Jason3S commented May 4, 2017

Jason3S left a comment

Jason3S May 4, 2017

sea1jxr May 7, 2017

Jason3S May 4, 2017

sea1jxr May 7, 2017

Jason3S commented May 4, 2017

sea1jxr commented May 7, 2017

coveralls commented May 7, 2017 •

edited

Loading

Jason3S commented May 10, 2017

Jason3S commented May 10, 2017

sea1jxr commented May 10, 2017

Add max() genSequence #3

Add max() genSequence #3

Conversation

sea1jxr commented Apr 13, 2017

coveralls commented Apr 13, 2017 • edited Loading

sea1jxr commented Apr 14, 2017

Jason3S commented Apr 14, 2017

Jason3S left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 22, 2017 • edited Loading

coveralls commented Apr 23, 2017 • edited Loading

coveralls commented Apr 23, 2017 • edited Loading

sea1jxr commented May 3, 2017

Jason3S commented May 4, 2017

Jason3S left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jason3S commented May 4, 2017

sea1jxr commented May 7, 2017

coveralls commented May 7, 2017 • edited Loading

Jason3S commented May 10, 2017

Jason3S commented May 10, 2017

sea1jxr commented May 10, 2017

coveralls commented Apr 13, 2017 •

edited

Loading

Jason3S left a comment •

edited

Loading

coveralls commented Apr 22, 2017 •

edited

Loading

coveralls commented Apr 23, 2017 •

edited

Loading

coveralls commented Apr 23, 2017 •

edited

Loading

coveralls commented May 7, 2017 •

edited

Loading