current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Sign up ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

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;
share|improve this question

2 Answers 2

up vote 3 down vote accepted

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;
})();
share|improve this answer
    
Some of this code seems worse than the original. Why introduce the variables hrs and mns? – konijn Nov 17 '14 at 15:00
1  
To be sure, your review itself is solid – konijn Nov 17 '14 at 15:34
    
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. – MT0 Nov 17 '14 at 21:06
    
The format function could be written better as function format(n) { return ('0' + n).slice(-2); } – Ben Nov 19 '14 at 14:45
1  
@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. – MT0 Nov 19 '14 at 15:16
up vote 2 down vote

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.

share|improve this answer
    
+1 for suggesting mimicking Date object functionality. I really enjoy adding negative time to objects, it's so much simpler! – Chris Cirefice Nov 17 '14 at 15:47

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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