Implement a Clock in JavaScript

Question

How idiomatic is my code?

var Clock = function(hour, minute) {
 this.hour = hour || 0;
 this.minute = minute || 0;
 this.plus = function(minutes) {
 computed_minutes = this.minute + minutes
 if (computed_minutes > 60) {
 this.minute = computed_minutes % 60
 this.hour += computed_minutes / 60
 } else {
 this.minute = computed_minutes
 }
 if (this.hour >= 24) {
 this.hour = this.hour - 24
 }
 this.hour = Math.round(this.hour)
 this.minute = Math.round(this.minute)
 return this;
 },
 this.minus = function(minutes) {
 computed_minutes = this.minute - minutes
 if (computed_minutes < 0) {
 this.minute = 60 + computed_minutes % 60
 this.hour -= (1 + Math.abs(computed_minutes / 60))
 } else {
 this.minute = computed_minutes;
 }
 if (this.hour < 0) {
 this.hour = 24 + this.hour
 }
 this.hour = Math.round(this.hour)
 this.minute = Math.round(this.minute)
 return this;
 },
 this.equals = function(other) {
 return this.hour == other.hour && this.minute == other.minute;
 }
}
Clock.at = function(hour, minute) {
 c = new Clock(hour, minute);
 return c;
}
Clock.prototype.toString = function() {
 function format(n) {
 return n >= 10 ? n : "0" + n;
 }
 return format(this.hour) + ":" + format(this.minute)
}
module.exports = Clock;

Answer 1

Some issues:

• Every instance of Clock gets its own instance of plus and minus rather than attaching these functions to the prototype.
• Clock.at sets a global variable c.
• Clock.prototype.toString creates the format function each time it is called; you can move this out of the function and into a different scope so it is only created once.
• plus and minus do a lot of checking that could be moved into a single set function and then this can also be called from the constructor (so that the same checks and rounding is also done in the constructor).
• 24 and 60 appear as magic constants. In this context it is easy to understand what they are but it would be better to assign them to named constants (HOURS_PER_DAY and MINUTES_PER_HOUR) that identify why those magic numbers are being used.

My suggestions:

module.export = (function(){
 var Clock = function( hours, minutes ) {
 this.set( hours, minutes )
 }
 Clock.at = function(hours, minutes) {
 return new Clock(hours, minutes);
 }
 Clock.HOURS_PER_DAY = 24;
 Clock.MINUTES_PER_HOUR = 60;
 Clock.prototype.set = function( hours, minutes ){
 var hrs = Math.round( hours || 0 ); 
 var mns = Math.round( minutes || 0 );
 this.hour = ( hrs + Math.floor( mns / Clock.MINUTES_PER_HOUR ) ) % Clock.HOURS_PER_DAY;
 if ( this.hour < 0 )
 {
 this.hour += Clock.HOURS_PER_DAY;
 }
 this.minute = mns % Clock.MINUTES_PER_HOUR;
 if ( this.minute < 0 )
 {
 this.minute += Clock.MINUTES_PER_HOUR;
 }
 return this;
 }
 Clock.prototype.plus = function(minutes) {
 return this.set( this.hour, this.minute + minutes )
 },
 Clock.prototype.minus = function(minutes) {
 return this.set( this.hour, this.minute - minutes );
 },
 Clock.prototype.equals = function(other) {
 return this.hour == other.hour && this.minute == other.minute;
 }
 function format(n) {
 return n >= 10 ? n : "0" + n;
 }
 Clock.prototype.toString = function() {
 return format(this.hour) + ":" + format(this.minute)
 }
 return Clock;
})();

• konijn: Some of this code seems worse than the original. Why introduce the variables hrs and mns?
• konijn: To be sure, your review itself is solid
• MT0: mns is used twice so is worth introducing a variable to make the code more efficient. They are also introduced so it is easier to review the code and see at a glance how the input is modified (i.e. 0 if undefined and rounded to the nearest integer); otherwise it would be buried in the middle of a long line of code.
• Ben: The format function could be written better as function format(n) { return ('0' + n).slice(-2); }
• MT0: @Ben jsperf.com/format-number-to-2-character-stringg - The example by the OP (and that I've used) is several orders of magnitude faster than slicing the string and is also more idiomatic.

Answer 2

From an idiomatic perspective;

• As MT0 mentioned, you should use more prototype
• Use lowerCamelCase, so computed_minutes -> computedMinutes
• In the JavaScript Date API you only have 1 addMinutes and then you can call it with positive or negative numbers to add or remove minutes. From an idiomatic perspective, you could considering doing the same. You would also be reducing some duplicated code

Other than your code is quite idiomatic.

• Chris Cirefice: +1 for suggesting mimicking Date object functionality. I really enjoy adding negative time to objects, it's so much simpler!