IE Failing to evaluate second if condition (&&)

Question

So I have this script:

        function makeActive() {
            var element, name, arr;
            element = document.getElementById("liveChat");
            name = "active";
            arr = element.className.split(" ");
            if (arr.indexOf(name) == -1) {
                element.className += " " + name;
            }
        }

        var currentTime = new Date();
        var currentTimeFormatted = currentTime.toLocaleTimeString();

        if(currentTimeFormatted >= '08:00:00' && currentTimeFormatted <= '16:30:00'){
            makeActive();
        }

Which works perfectly in Chrome, however in IE the class doesn't get added.

If I remove the

&& currentTimeFormatted <= '16:30:00'

IE also adds the class. Why would adding a second condition, break this script within IE?

what is `currentTimeFormatted` ? Log it to see whats going on. — Jonas Wilms, Nov 06 '18 at 14:40
When I alert current time formatted, I get the value I expect. E.g. 14:41:00. When I remove the && part of the conditional IE will add the class. It seems to fall over when trying to evaluate the second part of the condition. Chrome works with all occurrences. — Alex Wilkes, Nov 06 '18 at 14:41
Sorry, I've edited the first response. I accidentally hit enter which posted the comment — Alex Wilkes, Nov 06 '18 at 14:43
You should not rely on `toLocaleTimeString` for this purpose. Its output is implementation and locale-dependant. — str, Nov 06 '18 at 14:45
I see, do you have any suggestions on best practices for running a function between certain times? — Alex Wilkes, Nov 06 '18 at 14:46
Read this post to help you understand the issue better https://stackoverflow.com/q/21413757/9801177 — Pietro Nadalini, Nov 06 '18 at 14:47

score 1 · Answer 1 · answered Nov 06 '18 at 15:09

To make this a tad easier than having to use && and || mix, or if your values are stored somewhere in a static file etc. You could create a kind of pseudo time, by multiply each section.

eg.

const cTime = new Date();
const ptime =
  cTime.getHours() * 10000 +
  cTime.getMinutes() * 100 +
  cTime.getSeconds();
if (ptime >= 80000 && ptime <= 163000) {
  console.log("Active");
} else {
  console.log("InActive");
}

The Coprolal · Accepted Answer · 2018-11-07T12:40:40.707

You are doing string comparisons, which means that the browser and locale dependent output of toLocaleTimeString() screws your code in IE, and possibly also in other browsers or regions, because this function is solely intended for producing a human-readable time representation.

So you should either:

(1) Use a string representation that is standardized, e.g. invoking toISOString(). This will also get rid of time zone problems, because the result will always be in UTC time:

var currentTimeFormatted = new Date().toISOString(); // 2018-11-07T12:28:12.448Z'
currentTimeFormatted = currentTimeFormatted.substr(currentTimeFormatted.indexOf('T') + 1, 8); // 12:27:12

Now the rest of your code will work (assuming you 08:00:00 and 16:30:00 are UTC times).

(2) Extract the hour and minute parts of the new Date() and compare those to integers:

    var currentTime = new Date();
    if(currentTime.getHours() >= 8 
       && // similarly a comparison to < 16:30
    ) {
        makeActive();
    }

(3) Use the great solution by Keith (see below), which I think is the best way to go

Hey, I like this one considering the comments above about to locale string. Have amended the script to use this and works brilliant. Much appreciated. — Alex Wilkes, Nov 06 '18 at 15:08
Just a heads up, this isn't totally the same as the OP's.. eg. `16:30:10` would return active for this, but the OP's wouldn't. — Keith, Nov 06 '18 at 15:12
if(currentTime.getHours() >= 8 && currentTime.getHours() < 16 || currentTime.getHours() === 16 && currentTime.getMinutes() <= 30) I ended up with this in the end. Can you spot any potential issues? — Alex Wilkes, Nov 06 '18 at 15:13

John · Answer 3 · 2018-11-06T15:21:07.323

-1

IE's implementation of date.toLocaleTimeString() adds non-printable characters into the string. The easiest way to deal with them is to trim them from the string;

currentTimeFormatted = currentTime.toLocaleTimeString().replace(/[^ -~]/g,'')

When dealing with localized timezones and timezone comparison, it might be worth trying a library like moment.js which can also deal with comparing values using the isBetween funciton

Edit

As the other solutions have suggested - using toLocaleTimeString() is not a safe method of performing date comparison and should be avoided.

edited Nov 06 '18 at 15:21

answered Nov 06 '18 at 14:48

John

1,313
9
21

Doing string comparisons here should be disregarded, because the code still depends on the locale (e.g. if one-digit hours in some region or browser are not padded with a zero then you're screwed again), and adding a monstrous JS library just to compare hours and minutes should be mulled over too. – The Coprolal Nov 06 '18 at 15:01
I have changed the script to use whats below from The Corporal. Many thanks for your answer John, whilst it did work after learning about toLocaleString and it being used only for human readable, I'd prefer to use the integer version from getHours() and getMinutes() as suggested by The Corporal – Alex Wilkes Nov 06 '18 at 15:10
@AlexWilkes just before this gets downvoted to oblivion, I have been researching alternatives to moment.js and date-fns looks like it offers similar functionality but in theory allows you to only import the parts that you want to use. - I have never used it though. – John Nov 06 '18 at 15:15

IE Failing to evaluate second if condition (&&)

3 Answers3