CODEWITHSUNDEEP
javascriptdatetimetimeif-statement
Sino
Sino

Reputation: 990

Javascript time comparision

I currently have the following script:

<script>
if(new Date().getHours() > 17 || (new Date().getHours() == 17 &&     
new Date().getMinutes()== 0 && new Date().getSeconds() == 0) && 
(new Date().getHours() < 21 && new Date().getMinutes() < 30 
&& new Date().getSeconds() == 0)){
        //do nothing.   
    } else {
    $(document).ready(function() {
        $(".inline").colorbox({inline:true, open:true, width:"50%"});
        });
    }

So basicly what stand in the if: If the time is 17:00 till 21:30, do nothing, or else display the box. But what happends is that the box stops working around 18:00 and starts working at midnight again.. Someone see whats wrong here?

Upvotes: 0

Views: 124

Answers (4)

Luis Nell
Luis Nell

Reputation: 544

Here's how I'd write this: 

var now = new Date();

if (now.getHours() >= 17 && now.getHours() <= 21) {
    if (now.getHours() == 21 && now.getMinutes() > 30) {
        return;
    }
}

// Do your document.ready stuff here

First I did save the current time into a variable, which enables me to type less (remember: be a lazy typist!). Furthermore this also cleans up your condition a bit, so it is easier to spot any logic error.

Second of all, I split your condition (doing nothing between 17:00 and 21:30) into 2 separate if conditions. Personally, I prefer it that way because it is a no brainer, even if you come back to your code 2 years later.
Your code is only as good as it is readable. Always remember that. Complex if-conditions, even if commented nicely, just make it hard for you and others in the future. Ignore people who call you a noob for that.

Also I find it more readable to use a return, which simply aborts the current function/<script> if the condition matches. This saves you 1 indentation level :-)

Update: You should also read peakxu's answer (and therefore the MDN page for Date). Note that, as peakxu said, all of this is 0 indexed.

Upvotes: 1

webdeveloper
webdeveloper

Reputation: 17288

$(document).ready(function()
{
    var now = new Date(),
        block = $('div');

    if(now.getHours() >= 17 && (now.getHours() < 21 || (now.getHours() == 21 && now.getMinutes() <= 30)))
    {
        block.text('17:00 - 21:30');
        //do nothing.    
    }
    else
    {
        block.text('not 17:00 - 21:30');
        //$(".inline").colorbox({inline:true, open:true, width:"50%"});
    }
});

Demo: http://jsfiddle.net/FwtRb/10/

Upvotes: 1

peakxu
peakxu

Reputation: 6675

Note that many fields in Date (including Hour) is 0-indexed. This is why you observe this stop working around 18:00.

I suggest using variables to make the conditional simpler to reason about. Try something like this. If you're worried about namespace pollution, throw a closure around it.

var now = new Date();
var startQuietPeriod = new Date();
startQuietPeriod.setHours(16); startQuietPeriod.setMinutes(0); startQuietPeriod.setSeconds(0); startQuietPeriod.setMilliseconds(0);  // Today at 17:00
var endQuietPeriod = new Date();
endQuietPeriod.setHours(20); endQuietPeriod.setMinutes(30); endQuietPeriod.setSeconds(0); endQuietPeriod.setMilliseconds(0);  // Today at 21:30
if (startQuietPeriod < now && now < endQuietPeriod) {
  // don't show prompt
} else {
  // show prompt
}

Upvotes: 1

I Hate Lazy
I Hate Lazy

Reputation: 48761

var d = new Date();

if ( d.getHours() < 17 || 
     d.getHours() > 21 ||
    (d.getHours() == 21 && d.getMinutes() >= 30)) {

        $(document).ready(function() {
            $(".inline").colorbox({inline:true, open:true, width:"50%"});
        });
}

Upvotes: 0

Related Questions