Improve min and max logic (#3197)

* Improve min and max logic

* Fix import and update tests

* PR comments

* Change const to var, calculate toString once

Co-authored-by: Alex Browne <alex.browne@touchsurgery.com>

source/max.js

@@ -1,5 +1,5 @@
 import _curry2 from './internal/_curry2.js';
-
+import toString from './toString.js';
 
 /**
  * Returns the larger of its two arguments.
@@ -18,5 +18,24 @@ import _curry2 from './internal/_curry2.js';
  *      R.max(789, 123); //=> 789
  *      R.max('a', 'b'); //=> 'b'
  */
-var max = _curry2(function max(a, b) { return b > a ? b : a; });
+var max = _curry2(function max(a, b) {
+  if (a === b) { return b; }
+
+  function safeMax(x, y) {
+    if ((x > y) !== (y > x)) { return y > x ? y : x; }
+    return undefined;
+  }
+
+  var maxByValue = safeMax(a, b);
+  if (maxByValue !== undefined) { return maxByValue; }
+
+  var maxByType = safeMax(typeof a, typeof b);
+  if (maxByType !== undefined) { return maxByType === typeof a ? a : b; }
+
+  var stringA = toString(a);
+  var maxByStringValue = safeMax(stringA, toString(b));
+  if (maxByStringValue !== undefined) { return maxByStringValue === stringA ? a : b; }
+
+  return b;
+});
 export default max;

source/maxBy.js

@@ -1,5 +1,5 @@
 import _curry3 from './internal/_curry3.js';
-
+import max from './max.js';
 
 /**
  * Takes a function and two values, and returns whichever value produces the
@@ -26,6 +26,8 @@ import _curry3 from './internal/_curry3.js';
  *      R.reduce(R.maxBy(square), 0, []); //=> 0
  */
 var maxBy = _curry3(function maxBy(f, a, b) {
-  return f(b) > f(a) ? b : a;
+  var resultB = f(b);
+  return max(f(a), resultB) === resultB ? b : a;
 });
+
 export default maxBy;

source/min.js

@@ -1,5 +1,5 @@
 import _curry2 from './internal/_curry2.js';
-
+import toString from './toString.js';
 
 /**
  * Returns the smaller of its two arguments.
@@ -18,5 +18,24 @@ import _curry2 from './internal/_curry2.js';
  *      R.min(789, 123); //=> 123
  *      R.min('a', 'b'); //=> 'a'
  */
-var min = _curry2(function min(a, b) { return b < a ? b : a; });
+var min = _curry2(function min(a, b) {
+  if (a === b) { return a; }
+
+  function safeMin(x, y) {
+    if ((x < y) !== (y < x)) { return y < x ? y : x; }
+    return undefined;
+  }
+
+  var minByValue = safeMin(a, b);
+  if (minByValue !== undefined) { return minByValue; }
+
+  var minByType = safeMin(typeof a, typeof b);
+  if (minByType !== undefined) { return minByType === typeof a ? a : b; }
+
+  var stringA = toString(a);
+  var minByStringValue = safeMin(stringA, toString(b));
+  if (minByStringValue !== undefined) { return minByStringValue === stringA ? a : b; }
+
+  return a;
+});
 export default min;

source/minBy.js

@@ -1,5 +1,5 @@
 import _curry3 from './internal/_curry3.js';
-
+import min from './min.js';
 
 /**
  * Takes a function and two values, and returns whichever value produces the
@@ -26,6 +26,7 @@ import _curry3 from './internal/_curry3.js';
  *      R.reduce(R.minBy(square), Infinity, []); //=> Infinity
  */
 var minBy = _curry3(function minBy(f, a, b) {
-  return f(b) < f(a) ? b : a;
+  var resultB = f(b);
+  return min(f(a), resultB) === resultB ? b : a;
 });
 export default minBy;

test/max.js

@@ -19,4 +19,28 @@ describe('max', function() {
     eq(R.max('b', 'a'), 'b');
   });
 
+  it('returns the second argument if both arguments are equal according to the native JS strict equals operator', function() {
+    eq(R.max(7, 7), 7);
+    eq(R.max(undefined, undefined), undefined);
+  });
+
+  it('returns the alphabetically later type if neither argument is greater than the other', function() {
+    eq(R.max('a', 7), 'a');
+    eq(R.max('a', undefined), undefined);
+  });
+
+  it('returns the alphabetically later string coersion if no argument or type is greater than the other', function() {
+    const obj1 = { a: 1 };
+    const obj2 = { b: 1 };
+
+    eq(R.max(obj1, obj2), obj2);
+    eq(R.max(obj1, null), obj1);
+  });
+
+  it('returns the first argument if no other comparison attempts produce a result', function() {
+    const obj1 = { a: 1 };
+
+    eq(R.max(obj1, obj1), obj1);
+    eq(R.max(NaN, NaN), NaN);
+  });
 });

test/min.js

@@ -19,4 +19,28 @@ describe('min', function() {
     eq(R.min('b', 'a'), 'a');
   });
 
+  it('returns the first argument if both arguments are equal according to the native JS strict equals operator', function() {
+    eq(R.min(7, 7), 7);
+    eq(R.min(undefined, undefined), undefined);
+  });
+
+  it('returns the alphabetically earlier type if neither argument is smaller than the other', function() {
+    eq(R.min('a', 7), 7);
+    eq(R.min('a', undefined), 'a');
+  });
+
+  it('returns the alphabetically earlier string coersion if no argument or type is smaller than the other', function() {
+    const obj1 = { a: 1 };
+    const obj2 = { b: 1 };
+
+    eq(R.min(obj1, obj2), obj1);
+    eq(R.min(obj1, null), null);
+  });
+
+  it('returns the first argument if no other comparison attempts produce a result', function() {
+    const obj1 = { a: 1 };
+
+    eq(R.min(obj1, obj1), obj1);
+    eq(R.min(NaN, NaN), NaN);
+  });
 });