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>
1 Answer 1
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);
-
\$\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– Adam2018年08月30日 19:17:39 +00:00Commented Aug 30, 2018 at 19:17
Explore related questions
See similar questions with these tags.