current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 6 down vote favorite

I was working on a timer code in pure JavaScript and would like to know any pointers to improve:

Features

  • Start/ Stop/ Reset on click of a button.
  • Set limit to clock.
  • Update class name for warning and error based on threshold timer.

JSFiddle

function timer() {
  var time = {
    sec: 00,
    min: 00,
    hr: 00
  };
  var finalLimit = null,
    warnLimit = null,
    errorLimit = null;
  var max = 59;
  var interval = null;

  function init(_hr, _min, _sec) {
    time["hr"] = _hr ? _hr : 0;
    time["min"] = _min ? _min : 0;
    time["sec"] = _sec ? _sec : 0;
    printAll();
  }

  function setLimit(fLimit, wLimit, eLimit) {
    finalLimit = fLimit;
    warnLimit = wLimit;
    errorLimit = eLimit;
  }

  function printAll() {
    print("sec");
    print("min");
    print("hr");
  }

  function update(str) {
    time[str] ++;
    time[str] = time[str] % 60;
    if (time[str] == 0) {
      str == "sec" ? update("min") : update("hr");
    }
    print(str);
  }

  function print(str) {
    var _time = time[str].toString().length == 1 ? "0" + time[str] : time[str];
    document.getElementById("lbl" + str).innerHTML = _time;
  }

  function validateTimer() {
    var className = "";
    var secs = time.sec + (time.min * 60) + (time.hr * 60 * 60);
    if (secs >= finalLimit) {
      stopTimer();
    } else if (secs >= errorLimit) {
      className = "error";
    } else if (secs >= warnLimit) {
      className = "warn";
    }
    var element = document.getElementsByTagName("span");
    document.getElementById("lblsec").className = className;
  }

  function startTimer() {
    init();
    if (interval) stopTimer();
    interval = setInterval(function() {
      update("sec");
      validateTimer();
    }, 1000);
  }

  function stopTimer() {
    window.clearInterval(interval);
  }

  function resetInterval() {
    stopTimer();
    time["sec"] = time["min"] = time["hr"] = 0;
    printAll();
    startTimer();
  }

  return {
    'start': startTimer,
    'stop': stopTimer,
    'reset': resetInterval,
    'init': init,
    'setLimit': setLimit
  }
};

var time = new timer();

function initTimer() {
  time.init(0, 0, 0);
}

function startTimer() {
  time.start();
  time.setLimit(10, 5, 8);
}

function endTimer() {
  time.stop();
}

function resetTimer() {
  time.reset();
}
span {
  border: 1px solid gray;
  padding: 5px;
  border-radius: 4px;
  background: #fff;
}
.timer {
  padding: 2px;
  margin: 10px;
}
.main {
  background: #efefef;
  padding: 5px;
  width: 200px;
  text-align: center;
}
.btn {
  -webkit-border-radius: 6;
  -moz-border-radius: 6;
  border-radius: 6px;
  color: #ffffff;
  font-size: 14px;
  background: #2980b9;
  text-decoration: none;
  transition: 0.4s;
}
.btn:hover {
  background: #3cb0fd;
  text-decoration: none;
  transition: 0.4s;
}
.warn {
  background: yellow;
}
.error {
  background: red;
}
<div class="main">
  <div class="timer"> 
      <span id="lblhr">00</span>
    : <span id="lblmin">00</span>
    : <span id="lblsec">00</span>

  </div>
  <button class="btn" onclick="startTimer()">Start</button>
  <button class="btn" onclick="endTimer()">Stop</button>
  <button class="btn" onclick="resetTimer()">Reset</button>
</div>

share|improve this question
    
I have rolled back the last edit. Please see what you may and may not do after receiving answers. – Vogel612 Nov 25 '15 at 17:35
    
@Vogel612 Thanks. I'll keep that in mind. :-) – Rajesh Nov 25 '15 at 17:37
up vote 2 down vote

It looks really good! Here are a few points;

I don't like the arguments in your setLimit and init functions, especially because some of them are optional.

I would prefer an Object argument in this case.

function setLimit(limits) {
  finalLimit = limits.final;
  warnLimit = limits.warn;
  errorLimit = limits.error;
}


this.setLimit({
  final: 10,
  warn: 5,
  error: 8
});

This makes it obvious at call time which argument is which.

Your var declarations are inconsistent here:

var finalLimit = null,
  warnLimit = null,
  errorLimit = null;
var max = 59;
var interval = null;

Pick either to comma separate them or not. Not both.

In a couple of places you use == instead of ===. Generally we stick to === for reasons.

You CSS selectors are very generic. This is generally a bad thing as they will easily collide with other styles the moment you integrate it into a real page.

I favour BEM syntax, which would make it look something like this;

<div class="timer">
  <div class="timer__labels"> 
      <span id="timer__hours-label">00</span>
    : <span id="timer__minutes-label">00</span>
    : <span id="timer__seconds-label">00</span>

  </div>
  <button class="timer__btn js-start-timer">Start</button>
  <button class="timer__btn js-end-timer">Stop</button>
  <button class="timer__btn js-reset-timer">Reset</button>
</div>

Note I've also removed your onclick handlers in favour of interaction specific classes. I prefix my classnames with js- if they are used solely for JavaScript selectors. This helps decouple the CSS and JavaScript.

In your code you then attach the handlers;

document.querySelectorAll('.js-start-timer')[0]
  .addEventListener('click', startTimer);
share|improve this answer
    
Thanks! I would incorporate these points. Also I knew what == and === do but didn't know its not a good practice to use ==. Will keep this in mind. – Rajesh Nov 25 '15 at 17:06
1  
@Rajesh It's one of those things where sometimes you will need ==, but generally it's a source of very annoying bugs. – Jivings Nov 25 '15 at 17:07
    
Is it bad to use onclick or other events? I usually find them better. You can read HTML and you know where to look. Problem with event handlers that I feel, when multiple developers are involved, they add multiple event handlers to same element, and this complicates things. Though I'll try to incorporate that as well – Rajesh Nov 25 '15 at 17:15
    
@Rajesh Yeah, generally you want to keep your JavaScript code separate from your HTML. However, the js- classes allow me to see exactly what bit of JavaScript will be interacting with that element. – Jivings Nov 25 '15 at 17:18
    
@Rajesh Here's a good article about decoupling – Jivings Nov 25 '15 at 17:19

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.