7
\$\begingroup\$

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>

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

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);
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Rajesh Nov 25 '15 at 17:06
  • 1
    \$\begingroup\$ @Rajesh It's one of those things where sometimes you will need ==, but generally it's a source of very annoying bugs. \$\endgroup\$ – Jivings Nov 25 '15 at 17:07
  • \$\begingroup\$ 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 \$\endgroup\$ – Rajesh Nov 25 '15 at 17:15
  • \$\begingroup\$ @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. \$\endgroup\$ – Jivings Nov 25 '15 at 17:18
  • \$\begingroup\$ @Rajesh Here's a good article about decoupling \$\endgroup\$ – Jivings Nov 25 '15 at 17:19

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.