Fix and improve Duration#normalize (#1494)

src/duration.js

@@ -10,7 +10,6 @@ import {
   isUndefined,
   normalizeObject,
   roundTo,
-  signedFloor,
 } from "./impl/util.js";
 import Settings from "./settings.js";
 import DateTime from "./datetime.js";
@@ -127,27 +126,46 @@ function clone(dur, alts, clear = false) {
   return new Duration(conf);
 }
 
-// this is needed since in some test cases it would return 0.9999999999999999 instead of 1
-function removePrecisionIssue(a) {
-  return Math.trunc(a * 1e3) / 1e3;
-}
-
-// NB: mutates parameters
-function convert(matrix, fromMap, fromUnit, toMap, toUnit) {
-  const conv = matrix[toUnit][fromUnit];
-  const raw = fromMap[fromUnit] / conv;
-  const added = signedFloor(raw);
-
-  toMap[toUnit] = removePrecisionIssue(toMap[toUnit] + added);
-  fromMap[fromUnit] = removePrecisionIssue(fromMap[fromUnit] - added * conv);
+function durationToMillis(matrix, vals) {
+  let sum = vals.milliseconds ?? 0;
+  for (const unit of reverseUnits.slice(1)) {
+    if (vals[unit]) {
+      sum += vals[unit] * matrix[unit]["milliseconds"];
+    }
+  }
+  return sum;
 }
 
 // NB: mutates parameters
 function normalizeValues(matrix, vals) {
+  // the logic below assumes the overall value of the duration is positive
+  // if this is not the case, factor is used to make it so
+  const factor = durationToMillis(matrix, vals) < 0 ? -1 : 1;
+
   reverseUnits.reduce((previous, current) => {
     if (!isUndefined(vals[current])) {
       if (previous) {
-        convert(matrix, vals, previous, vals, current);
+        const previousVal = vals[previous] * factor;
+        const conv = matrix[current][previous];
+
+        // if (previousVal < 0):
+        // lower order unit is negative (e.g. { years: 2, days: -2 })
+        // normalize this by reducing the higher order unit by the appropriate amount
+        // and increasing the lower order unit
+        // this can never make the higher order unit negative, because this function only operates
+        // on positive durations, so the amount of time represented by the lower order unit cannot
+        // be larger than the higher order unit
+        // else:
+        // lower order unit is positive (e.g. { years: 2, days: 450 } or { years: -2, days: 450 })
+        // in this case we attempt to convert as much as possible from the lower order unit into
+        // the higher order one
+        //
+        // Math.floor takes care of both of these cases, rounding away from 0
+        // if previousVal < 0 it makes the absolute value larger
+        // if previousVal >= it makes the absolute value smaller
+        const rollUp = Math.floor(previousVal / conv);
+        vals[current] += rollUp * factor;
+        vals[previous] -= rollUp * conv * factor;
       }
       return current;
     } else {
@@ -581,13 +599,7 @@ export default class Duration {
   toMillis() {
     if (!this.isValid) return NaN;
 
-    let sum = this.values.milliseconds ?? 0;
-    for (let unit of reverseUnits.slice(1)) {
-      if (this.values[unit]) {
-        sum += this.values[unit] * this.matrix[unit]["milliseconds"];
-      }
-    }
-    return sum;
+    return durationToMillis(this.matrix, this.values);
   }
 
   /**
@@ -697,18 +709,22 @@ export default class Duration {
 
   /**
    * Reduce this Duration to its canonical representation in its current units.
+   * Assuming the overall value of the Duration is positive, this means:
+   * - excessive values for lower-order units are converted to higher order units (if possible, see first and second example)
+   * - negative lower-order units are converted to higher order units (there must be such a higher order unit, otherwise
+   *   the overall value would be negative, see second example)
+   *
+   * If the overall value is negative, the result of this method is equivalent to `this.negate().normalize().negate()`.
    * @example Duration.fromObject({ years: 2, days: 5000 }).normalize().toObject() //=> { years: 15, days: 255 }
+   * @example Duration.fromObject({ days: 5000 }).normalize().toObject() //=> { days: 5000 }
    * @example Duration.fromObject({ hours: 12, minutes: -45 }).normalize().toObject() //=> { hours: 11, minutes: 15 }
    * @return {Duration}
    */
   normalize() {
     if (!this.isValid) return this;
     const vals = this.toObject();
-    if (this.valueOf() >= 0) {
-      normalizeValues(this.matrix, vals);
-      return clone(this, { values: vals }, true);
-    }
-    return this.negate().normalize().negate();
+    normalizeValues(this.matrix, vals);
+    return clone(this, { values: vals }, true);
   }
 
   /**
@@ -758,16 +774,12 @@ export default class Duration {
           own += vals[k];
         }
 
+        // only keep the integer part for now in the hopes of putting any decimal part
+        // into a smaller unit later
         const i = Math.trunc(own);
         built[k] = i;
         accumulated[k] = (own * 1000 - i * 1000) / 1000;
 
-        // plus anything further down the chain that should be rolled up in to this
-        for (const down in vals) {
-          if (orderedUnits.indexOf(down) > orderedUnits.indexOf(k)) {
-            convert(this.matrix, vals, down, built, k);
-          }
-        }
         // otherwise, keep it in the wings to boil it later
       } else if (isNumber(vals[k])) {
         accumulated[k] = vals[k];
@@ -783,7 +795,8 @@ export default class Duration {
       }
     }
 
-    return clone(this, { values: built }, true).normalize();
+    normalizeValues(this.matrix, built);
+    return clone(this, { values: built }, true);
   }
 
   /**

src/impl/util.js

@@ -124,10 +124,6 @@ export function parseMillis(fraction) {
   }
 }
 
-export function signedFloor(number) {
-  return number > 0 ? Math.floor(number) : Math.ceil(number);
-}
-
 export function roundTo(number, digits, towardZero = false) {
   const factor = 10 ** digits,
     rounder = towardZero ? Math.trunc : Math.round;