Skip to content

Commit df20205

Browse files
leebyronleebyron
committed
Improve min/max to be order-independent when handling null/undefined/NaN
closes immutable-js#233
1 parent f7f4d1c commit df20205

File tree

7 files changed

+147
-41
lines changed

7 files changed

+147
-41
lines changed

‎__tests__/minmax.ts

+65
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,17 @@
33

44
jest.autoMockOff();
55

6+
import jasmineCheck = require('jasmine-check');
7+
jasmineCheck.install();
8+
69
import I = require('immutable');
710
import Seq = I.Seq;
811

12+
var genHeterogeneousishArray = gen.oneOf([
13+
gen.array(gen.oneOf([gen.string, gen.undefined])),
14+
gen.array(gen.oneOf([gen.int, gen.NaN]))
15+
]);
16+
917
describe('max', () => {
1018

1119
it('returns max in a sequence', () => {
@@ -36,6 +44,36 @@ describe('max', () => {
3644
expect(family.maxBy<number>(p => p.age, (a, b) => b - a).name).toBe('Oakley');
3745
});
3846

47+
it('surfaces NaN, null, and undefined', () => {
48+
expect(
49+
I.is(NaN, I.Seq.of(1, 2, 3, 4, 5, NaN).max())
50+
).toBe(true);
51+
expect(
52+
I.is(NaN, I.Seq.of(NaN, 1, 2, 3, 4, 5).max())
53+
).toBe(true);
54+
expect(
55+
I.is(null, I.Seq.of('A', 'B', 'C', 'D', null).max())
56+
).toBe(true);
57+
expect(
58+
I.is(null, I.Seq.of(null, 'A', 'B', 'C', 'D').max())
59+
).toBe(true);
60+
});
61+
62+
it('null treated as 0 in default iterator', () => {
63+
expect(
64+
I.is(2, I.Seq.of(-1, -2, null, 1, 2).max())
65+
).toBe(true);
66+
});
67+
68+
check.it('is not dependent on order', [genHeterogeneousishArray], vals => {
69+
expect(
70+
I.is(
71+
I.Seq(shuffle(vals.slice())).max(),
72+
I.Seq(vals).max()
73+
)
74+
).toEqual(true);
75+
});
76+
3977
});
4078

4179
describe('min', () => {
@@ -68,4 +106,31 @@ describe('min', () => {
68106
expect(family.minBy<number>(p => p.age, (a, b) => b - a).name).toBe('Casey');
69107
});
70108

109+
check.it('is not dependent on order', [genHeterogeneousishArray], vals => {
110+
expect(
111+
I.is(
112+
I.Seq(shuffle(vals.slice())).min(),
113+
I.Seq(vals).min()
114+
)
115+
).toEqual(true);
116+
});
117+
71118
});
119+
120+
function shuffle(array) {
121+
var m = array.length, t, i;
122+
123+
// While there remain elements to shuffle…
124+
while (m) {
125+
126+
// Pick a remaining element…
127+
i = Math.floor(Math.random() * m--);
128+
129+
// And swap it with the current element.
130+
t = array[m];
131+
array[m] = array[i];
132+
array[i] = t;
133+
}
134+
135+
return array;
136+
}

‎dist/immutable.d.ts

+18-2
Original file line numberDiff line numberDiff line change
@@ -1919,7 +1919,15 @@ declare module 'immutable' {
19191919
* comparatively equivalent, the first one found will be returned.
19201920
*
19211921
* The `comparator` is used in the same way as `Iterable#sort`. If it is not
1922-
* provided, the default comparator is `a > b`.
1922+
* provided, the default comparator is `>`.
1923+
*
1924+
* When two values are considered equivalent, the first encountered will be
1925+
* returned. Otherwise, `max` will operate independent of the order of input
1926+
* as long as the comparator is commutative. The default comparator `>` is
1927+
* commutative *only* when types do not differ.
1928+
*
1929+
* If `comparator` returns 0 and either value is NaN, undefined, or null,
1930+
* that value will be returned.
19231931
*/
19241932
max(comparator?: (valueA: V, valueB: V) => number): V;
19251933

@@ -1940,7 +1948,15 @@ declare module 'immutable' {
19401948
* comparatively equivalent, the first one found will be returned.
19411949
*
19421950
* The `comparator` is used in the same way as `Iterable#sort`. If it is not
1943-
* provided, the default comparator is `a > b`.
1951+
* provided, the default comparator is `<`.
1952+
*
1953+
* When two values are considered equivalent, the first encountered will be
1954+
* returned. Otherwise, `min` will operate independent of the order of input
1955+
* as long as the comparator is commutative. The default comparator `<` is
1956+
* commutative *only* when types do not differ.
1957+
*
1958+
* If `comparator` returns 0 and either value is NaN, undefined, or null,
1959+
* that value will be returned.
19441960
*/
19451961
min(comparator?: (valueA: V, valueB: V) => number): V;
19461962

‎dist/immutable.js

+9-5
Original file line numberDiff line numberDiff line change
@@ -826,7 +826,7 @@ function quoteString(value) {
826826
return typeof value === 'string' ? JSON.stringify(value) : value;
827827
}
828828
function defaultNegComparator(a, b) {
829-
return a > b ? -1 : a < b ? 1 : 0;
829+
return a < b ? 1 : a > b ? -1 : 0;
830830
}
831831
function deepEqual(a, b) {
832832
if (a === b) {
@@ -2550,16 +2550,20 @@ function maxFactory(iterable, comparator, mapper) {
25502550
if (mapper) {
25512551
var entry = iterable.toSeq().map((function(v, k) {
25522552
return [v, mapper(v, k, iterable)];
2553-
})).reduce((function(max, next) {
2554-
return comparator(next[1], max[1]) > 0 ? next : max;
2553+
})).reduce((function(a, b) {
2554+
return _maxCompare(comparator, a[1], b[1]) ? b : a;
25552555
}));
25562556
return entry && entry[0];
25572557
} else {
2558-
return iterable.reduce((function(max, next) {
2559-
return comparator(next, max) > 0 ? next : max;
2558+
return iterable.reduce((function(a, b) {
2559+
return _maxCompare(comparator, a, b) ? b : a;
25602560
}));
25612561
}
25622562
}
2563+
function _maxCompare(comparator, a, b) {
2564+
var comp = comparator(b, a);
2565+
return (comp === 0 && b !== a && (b === undefined || b === null || b !== b)) || comp > 0;
2566+
}
25632567
function reify(iter, seq) {
25642568
return isSeq(iter) ? seq : iter.constructor(seq);
25652569
}

0 commit comments

Comments
 (0)