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 2 down vote favorite

When a button is pressed, an animation loop is triggered. At the start of the loop, I create a new time object.

I also have a pause button. A problem was created when paused and then started as the Date object would be recreated each time.

The variable is global and not defined to be anything. I could set it to 0 and run the conditional test on that instead, but either way there has to be a cleaner way of doing this. Apologies, I'm quite new to JS.

The code works, but it's ugly (in my opinion):

var t;//not defined

//for starting the animation
startSim.onclick = function(){

    if(t == undefined){
        //get time object
        t = new Date();
    }

    //other code...

}

EDIT: In case the question is unclear - I'm checking to see if the Date object has been created by finding out if the variable is undefined or not. What I'm asking is simply Is there a way to find out if the Date object already exists that doesn't rely on checking if the var is undefined. There is little code as the above is the only relevant part, I can post the rest but it's pointless as not required. (I've added a little more to show structure but I don't think it's required to understand the question)

share|improve this question
    
You want us to review 2 lines of code? –  George Mauer Jan 13 at 2:12
    
@GeorgeMauer - was the question unclear? –  Steve Green Jan 13 at 2:24
    
Yes it is, I can't tell what you're asking. Generally this is for code review meaning that you need code to be reviewed. I'm not against asking for design review but you've got to give me a design to review - a chart, some pseudocode, something –  George Mauer Jan 13 at 3:13
    
@GeorgeMauer - apologies, I thought the question was clear, edited to try and clarify further... –  Steve Green Jan 13 at 9:06
1  
Consider t = t || new Date() which either keeps t if it is defined or assigns the new Date() value –  konijn Jan 13 at 21:18
show 2 more comments

1 Answer

up vote 2 down vote accepted

I can think of the following improvements:

  1. Hide t. If you're running other JS modules, you can't really be sure that no-one else writes to the same variable.
  2. If you want to be sure that your code doesn't mix up an uninitialized timer with a "cleared" timer, you might want to pre-initialize the variable to null instead.
  3. Use === when comparing null, undefined, etc. In JavaScript, null == undefined, but only null === null.

You might want to do something like this:

var timer = (function () {
    var t = null,

    start = function () {
        if (t === null) {
            t = new Date();
        }
    },

    stop = function () {
        var time = new Date() - t;
        t = null;
        return time;
    };

    return {
        start: start,
        stop: stop
    };
})();

timer.start();

var timePassed = timer.stop();
share|improve this answer
    
thank you for the suggestions and advice, it's much better than what I had, thank you. –  Steve Green Jan 13 at 12:03
    
Hi again, I'm just trying to implement a pause feature into the timer function and can't seem to get it right, would you mind lending a hand please? I've added a pause function such as: pause = function(){ pause = true; pausedTime = t; return pausedTime; }, But it's not working as expected. –  Steve Green Feb 15 at 19:36
add comment

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.