Stop the switch madness!

aws · RomainMuller · Jun 20, 2019 · Jun 12, 2019 · Jun 13, 2019 · Jun 14, 2019
commit 5f9ca7a9783f8944596e0bbcba6d617ec626ed7c
diff --git a/packages/@aws-cdk/cdk/lib/duration.ts b/packages/@aws-cdk/cdk/lib/duration.ts
@@ -75,52 +75,22 @@ export class Duration {
   /**
    * @returns the value of this `Duration` expressed in Seconds.
    */
-  public toSeconds(_opts: TimeConversionOptions = {}): number {
-    switch (this.unit) {
-      case TimeUnit.Seconds:
-        return this.amount;
-      case TimeUnit.Minutes:
-        return this.amount * 60;
-      case TimeUnit.Hours:
-        return this.amount * 3600;
-      case TimeUnit.Days:
-        return this.amount * 86_400;
-      default:
-        throw new Error(`Unexpected time unit: ${this.unit}`);
-    }
+  public toSeconds(opts: TimeConversionOptions = {}): number {
+    return _ensureIntegral(this.amount * this.unit.inSeconds, opts);
   }
 
+  /**
+   * @returns the value of this `Duration` expressed in Minutes.
+   */
   public toMinutes(opts: TimeConversionOptions = {}): number {
-    switch (this.unit) {
-      case TimeUnit.Seconds:
-        return this.safeConversion(60, opts, TimeUnit.Seconds);
-      case TimeUnit.Minutes:
-        return this.amount;
-      case TimeUnit.Hours:
-        return this.amount * 60;
-      case TimeUnit.Days:
-        return this.amount * 1_440;
-      default:
-        throw new Error(`Unexpected time unit: ${this.unit}`);
-    }
+    return _ensureIntegral(this.amount * this.unit.inMinutes, opts);
   }
 
   /**
    * @returns the value of this `Duration` expressed in Days.
    */
   public toDays(opts: TimeConversionOptions = {}): number {
-    switch (this.unit) {
-      case TimeUnit.Seconds:
-        return this.safeConversion(86_400, opts, TimeUnit.Days);
-      case TimeUnit.Minutes:
-        return this.safeConversion(1_440, opts, TimeUnit.Days);
-      case TimeUnit.Hours:
-        return this.safeConversion(24, opts, TimeUnit.Days);
-      case TimeUnit.Days:
-        return this.amount;
-      default:
-        throw new Error(`Unexpected time unit: ${this.unit}`);
-    }
+    return _ensureIntegral(this.amount * this.unit.inDays, opts);
   }
 
   /**
@@ -146,24 +116,6 @@ export class Duration {
     return `${this.amount} ${this.unit}`;
   }
 
-  /**
-   * Divides the current duration by a certain amount and throws an exception if `opts` requires `integral` conversion
-   * and the requirement is not satisfied.
-   *
-   * @param divisor    the value to divide `this.amount` by.
-   * @param opts       conversion options instructing whether integral conversion is required or not.
-   * @param targetUnit the unit of time we are converting to, so it can be used in the error message when needed
-   *
-   * @returns the result of `this.amount / divisor`.
-   */
-  private safeConversion(divisor: number, { integral = true }: TimeConversionOptions, targetUnit: TimeUnit): number {
-    const remainder = this.amount % divisor;
-    if (integral && remainder !== 0) {
-      throw new Error(`Impossible intergal conversion of ${this} to ${targetUnit} was requested`);
-    }
-    return this.amount / divisor;
-  }
-
   private fractionDuration(symbol: string, modulus: number, next: (amount: number) => Duration): string {
     if (this.amount < modulus) {
       return `${this.amount}${symbol}`;
@@ -189,13 +141,6 @@ export interface TimeConversionOptions {
   readonly integral?: boolean;
 }
 
-const enum TimeUnit {
-  Seconds = 's',
-  Minutes = 'minutes',
-  Hours = 'hours',
-  Days = 'days'
-}
-
 /**
  * Helpful syntax sugar for turning an optional `Duration` into a count of seconds.
  *
@@ -206,3 +151,29 @@ const enum TimeUnit {
 export function toSeconds(duration: Duration | undefined): number | undefined {
   return duration && duration.toSeconds();
 }
+
+class TimeUnit {
+  public static readonly Seconds = new TimeUnit('seconds', 1);
+  public static readonly Minutes = new TimeUnit('minutes', 60);
+  public static readonly Hours = new TimeUnit('hours', 3_600);
+  public static readonly Days = new TimeUnit('days', 86_400);
+
+  public readonly inMinutes: number;
+  public readonly inDays: number;
+
+  private constructor(public readonly label: string, public readonly inSeconds: number) {
+    this.inMinutes = inSeconds / 60;
+    this.inDays = inSeconds / 86_400;
+  }
+
+  public toString() {
+    return this.label;
+  }
+}
+
+function _ensureIntegral(value: number, { integral = true }: TimeConversionOptions) {
+  if (!Number.isInteger(value) && integral) {
+    throw new Error(`Required integral time unit conversion, but value ${value} is not integral.`);
+  }
+  return value;
+}
diff --git a/packages/@aws-cdk/cdk/test/test.duration.ts b/packages/@aws-cdk/cdk/test/test.duration.ts
@@ -13,8 +13,8 @@ export = nodeunit.testCase({
 
     test.equal(duration.toSeconds(), 300);
     test.equal(duration.toMinutes(), 5);
-    test.throws(() => duration.toDays(), /Impossible intergal conversion of/);
-    test.equal(duration.toDays({ integral: false }), 300 / 86_400);
+    test.throws(() => duration.toDays(), /Required integral time unit conversion, but value/);
+    floatEqual(test, duration.toDays({ integral: false }), 300 / 86_400);
 
     test.equal(Duration.seconds(60 * 60 * 24).toDays(), 1);
 
@@ -26,8 +26,8 @@ export = nodeunit.testCase({
 
     test.equal(duration.toSeconds(), 300);
     test.equal(duration.toMinutes(), 5);
-    test.throws(() => duration.toDays(), /Impossible intergal conversion of/);
-    test.equal(duration.toDays({ integral: false }), 300 / 86_400);
+    test.throws(() => duration.toDays(), /Required integral time unit conversion, but value/);
+    floatEqual(test, duration.toDays({ integral: false }), 300 / 86_400);
 
     test.equal(Duration.minutes(60 * 24).toDays(), 1);
 
@@ -39,8 +39,8 @@ export = nodeunit.testCase({
 
     test.equal(duration.toSeconds(), 18_000);
     test.equal(duration.toMinutes(), 300);
-    test.throws(() => duration.toDays(), /Impossible intergal conversion of/);
-    test.equals(duration.toDays({ integral: false }), 5 / 24);
+    test.throws(() => duration.toDays(), /Required integral time unit conversion, but value/);
+    floatEqual(test, duration.toDays({ integral: false }), 5 / 24);
 
     test.equal(Duration.hours(24).toDays(), 1);
 
@@ -89,3 +89,11 @@ export = nodeunit.testCase({
     test.done();
   }
 });
+
+function floatEqual(test: nodeunit.Test, actual: number, expected: number) {
+  test.ok(
+    // Floats are subject to rounding errors up to Number.ESPILON
+    actual >= expected - Number.EPSILON && actual <= expected + Number.EPSILON,
+    `${actual} == ${expected}`,
+  );
+}