2
\$\begingroup\$

I've written a simple script that calculates the distance between two points in time and returns the duration.

Example: Start - 11:00, End - 11:45, Duration - 0.75

I wanted to make use of ES6 classes and I'd like feedback on the Duration class in particular. For example, should the validation of the start and end arguments go in the constructor? Or should that be done later?

class Duration {
  constructor(start, end) {
    const reg = RegExp('^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$');

    this.start = reg.test(start) ? start : '00:00';
    this.end = reg.test(end) ? end : '00:00';
  }

  _seconds(time = "00:00") {
    const split = time.split(':');

    return Number(split[0]) * 60 + Number(split[1]);
  }

  // See: https://stackoverflow.com/questions/11832914/round-to-at-most-2-decimal-places-only-if-necessary
  _round(float) {
    return Math.round((float + 0.0001) * 100) / 100;
  }

  get difference() {
    const {
      _round: round,
      _seconds: seconds,
      start,
      end
    } = this;

    return round((seconds(end) - seconds(start)) / 60);
  }

}

function calcDuration(e) {
  e.preventDefault();

  const start = document.getElementById('time-start');
  const end = document.getElementById('time-end');
  const target = document.getElementById('target');

  const duration = new Duration(start.value, end.value);

  target.innerHTML += `<tr><td>${duration.start}</td><td>${duration.end}</td><td>${duration.difference}</td></tr>`;

  start.focus();
}

document.getElementById('time-submit').addEventListener('click', calcDuration);
<link href="https://stackpath.bootstrapcdn.com/bootstrap/4.1.3/css/bootstrap.min.css" rel="stylesheet" />
<div class="container">
  <div class="row">
    <div class="col pt-5">
      <h1>Duration Calculator</h1>
      <p>Enter a start time and an end time and a conversion to a decimal will be given!</p>
    </div>
  </div>
</div>
<div class="container">
  <div class="row">
    <div class="col pt-4 pb-4">
      <form class="time mx-auto">
        <div class="form-row">
          <div class="col-auto">
            <label for="time-start">Start time:</label>
          </div>
          <div class="col-auto">
            <input type="time" id="time-start" class="form-control" name="time-start" required autofocus/>
          </div>
          <div class="col-auto">
            <label for="time-end">End time:</label>
          </div>
          <div class="col-auto">
            <input type="time" id="time-end" name="time-end" class="form-control" required/>
          </div>
          <div class="col-auto">
            <button id="time-submit" class="btn btn-primary" type="submit" value="Calculate">Calculate</button>
          </div>
        </div>
      </form>
    </div>
  </div>
</div>
<div class="container">
  <div class="row">
    <div class="col pt-2 pb-5">
      <table id="target" class="table">
        <thead>
          <tr>
            <th>Start</th>
            <th>End</th>
            <th>Duration</th>
          </tr>
        </thead>
      </table>
    </div>
  </div>
</div>

\$\endgroup\$
2
\$\begingroup\$

NOTE     I'll only be taking a look at your Duration class, not the UI-specific code.


Regex as a constant

You don't need to re-instantiate a new RegExp instance every time a Duration is instantiated. Move the regex initialization to the global context, or maybe as a static field of the class. This saves memory as it's not allocated for the same regex over and over whenever you call new Duration.

Also, let's be more descriptive in its naming. Since it's used for validation, let's name it exactly that.

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;
  constructor(start, end) {
    // ...
  }
  // ...
}

You also don't need to instantiate it with the RegExp function; you can just place it there as a literal as it's known run-time.

Helper methods

...shouldn't be methods at all, but functions instead. The _seconds and _round do not belong in the Duration class. They are pure utility functions that don't touch this at all: give them something as input, and something comes out as output.

If you put them in a separate file, they also become more reusable. Your round function, for example, has nothing to do with Duration and thus you could re-use it in any math-based code -- like a renderer in a 3D engine!

function seconds(time = "00:00") { /* ... */ }
function round(float) { /* ... */ }

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;
  constructor(start, end) { /* ... */ }
  get difference() { /* ... */ }
}

A good rule of thumb that a method should be a util function is that it doesn't use this. If you feel that the util function is still semantically tied to the class, you should place it as a static method.

This also make the get difference part simpler:

get difference() {
  const { start, end } = this;
  return round((seconds(end) - seconds(start)) / 60);
}

Validation

Constructor is a good place for validation in this case. If someone passes in some junk instead of valid timestamps, you'd want to catch that immediately as no operation would make sense if the start and end are not valid timestamps.

However, your handling of validation only hides errors under the rug. Say that someone, by mistake, tries to create a Duration like this:

new Duration("12:00", "1300")

Your validation will successfully test that 12:00 is fine, and assign that to start, but the 1300 will be silently treated as 00:00. And now my duration is actually (12:00, 00:00), but I wanted (12:00, 13:00). A silent bug. No error in console, but my results are weird. Difficult to debug!

That's why you should throw an exception if the validation fails. Feel free to scream at me, I want to know that I've messed up.

Here's the idea:

constructor(start, end) {
  if (!Duration.VALIDATION.test(start)) {
    throw new Error(`The "start" argument (${start}) is in wrong format.`)
  } else {
    this.start = start
  }
}

Notice what makes a good error message: it tells me exactly what I've screwed up. It would be even better if it went on and explained what is a valid argument, but I'll go ahead and assume that it's obvious in this case for brevity.

In order to avoid repeating, you can extract the validation as a method. The method would act as a no-op function in case everything is OK, but throw if something's wrong. It's a good candidate for a static method, since it's clearly tightly coupled with the Duration class.

static throwIfError(arg, argName) {
  if (!Duration.VALIDATION.test(arg)) {
    throw new Error(`The "${argName}" argument (${arg}) is in wrong format.`);
  }
  return arg;
}

constructor(start, end) {
  this.start = Duration.throwIfError(start, 'start');
  this.end = Duration.throwIfError(end, 'end');
}

The second argument is just for a prettier message. Now, when I try my example, new Date("12:00", "1300"), I get an error message saying this:

Error: The "end" argument (1300) is in wrong format.

Complete code

My full refactored version of the original code is below, along with an example of usage.

function seconds(time = "00:00") {
  const split = time.split(':');
  return Number(split[0]) * 60 + Number(split[1]);
}

// See: https://stackoverflow.com/questions/11832914/round-to-at-most-2-decimal-places-only-if-necessary
function round(float) {
  return Math.round((float + 0.0001) * 100) / 100;
}

class Duration {
  static VALIDATION = /^([0-9]|0[0-9]|1[0-9]|2[0-3]):[0-5][0-9]$/;

  static throwIfError(arg, argName) {
    if (!Duration.VALIDATION.test(arg)) {
      throw new Error(`The "${argName}" argument (${arg}) is in wrong format.`);
    }
    return arg;
  }

  constructor(start, end) {
    this.start = Duration.throwIfError(start, 'start');
    this.end = Duration.throwIfError(end, 'end');
  }

  get difference() {
    const { start, end } = this;
    return round((seconds(end) - seconds(start)) / 60);
  }

}

console.log("From 12:00 to 13:00 => ");
console.log(new Duration("12:00", "13:00").difference);
console.log("From 12:00 to 1300 (!) => ");
console.log(new Duration("12:00", "1300").difference);

\$\endgroup\$
1
  • \$\begingroup\$ I like your emphasis on using static class members over globally scoped variables/constants. If you're using classical objects, this is the best way to go. \$\endgroup\$ – Adam Aug 30 '18 at 19:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.