current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 3 down vote favorite

Just as a learning exercise, I set out making a stopwatch without looking up how to do it etc.

I know it will have been done many times before. I'm just looking for some feedback on what I should do to make the code more efficient / cleaner / in keeping with standards etc.

This is the result

// declare vars
var secondsDiv = $("#seconds");
var minsDiv = $("#mins");
var hoursDiv = $("#hours");
var interval = null;
var timer = false;

// return the value of a given div
function getCurrentValue(value) {
  return value.html();
}

// reset the value of a chosen div to 00
function resetValue(value){
  value.html("00");
}

// check if values are more than 59 to progress the timer
function check59() {
  var currentSec = getCurrentValue(secondsDiv);
  var currentMins = getCurrentValue(minsDiv);
  var currentHours = getCurrentValue(hoursDiv);
  // check the seconds to become a minute
  if (currentSec > 59) {
    currentMins++;
    if (currentMins < 10) {
      minsDiv.html("0" + currentMins);
    } else {
      minsDiv.html(currentMins);
    }
    resetValue(secondsDiv);
  }
  // check the minutes to become an hour
  if (currentMins > 59) {
    currentHours++;
    if (currentHours < 10) {
      hoursDiv.html("0" + currentHours);
    } else {
      hoursDiv.html(currentHours);
    }
    resetValue(minsDiv);
  }
}

// add seconds
function addSecond() {
  var currentSec = getCurrentValue(secondsDiv);
  currentSec++;
  if (currentSec < 10) {
    secondsDiv.html("0" + currentSec);
  } else {
    secondsDiv.html(currentSec);
  }
  check59();
}  

// run the initial addSecond function every second
$("#startTimer").click(function(){
  if (timer===false) {
    timer = true;
    interval = setInterval(addSecond, 1000);
  }
});

// stop the addSecond function every second
$("#pauseTimer").click(function(){
  clearInterval(interval);
  timer = false;
});

// reset all values
$("#clearTimer").click(function(){
  resetValue(secondsDiv);
  resetValue(minsDiv);
  resetValue(hoursDiv);
});
share|improve this question
    
I don't know if you have noticed but when you paus and then start the watch it begins counting from 0.00 instead of where I paused, for instance 0.84. You would probably want to fix this. –  Chrillewoodz Jan 15 at 17:11

1 Answer 1

up vote 1 down vote

Looks pretty good! Here's a couple of pointers for you:

It's custom to declare jQuery variables with a $ prefix. This way it's easy to tell at a glance that it is a element collection. So your top variables would look like this:

var $secondsDiv = $("#seconds");
var $minsDiv = $("#mins");

Instead of:

if (timer===false)

Consider:

if (!timer)

If you're looking for legibility there are probably a few other things I'd do differently.

Grouping your variables allows you to make a few shortcuts:

var time = {
  seconds: $("#seconds"), 
  minutes: $("#mins"),
  hours:   $("#hours")
}

function getValue(timeStr) {
  return time[timeStr].html();
}

getValue('seconds')

And I'm not sure about the check59() implementation. I'm sure we could make that more efficient, and reduce the repetition.

But the next step for you is definitely to create a class for this and scope your variables, that way you'll be able to include multiple timers in the same page :)

share|improve this answer
    
Thanks for the suggestions, have tidied up the check59() function slightly and made a function to check if a value is a double digit. Cant think of a way yet to make that bit more efficient. I will be looking to make it into a class soon. Learning PHP classes currently so Javascript will come soon. –  Lovelock Sep 17 '14 at 11:06

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.