3
\$\begingroup\$

I'm using this code to have a site show either "Open" or "Closed" depending on what time it is.

I'm a junior developer and I want to show this to my boss but one of the Senior Developers said to make it more robust/reusable before I do.

var currentDate = new Date();
var currentMonth = currentDate.getMonth();
var currentDay = currentDate.getDay();
var currentHour = currentDate.getHours();
var currentMinute = currentDate.getMinutes();

function openClose() {
  if(currentDay >= 1 && currentDay <= 4) { 
    document.getElementById("friday").style.display = "none";
    if (currentHour >= 8 && currentHour <= 21) { 
      document.getElementById("closed").style.display = "none";
    }
  else {
    document.getElementById("open").style.display = "none";
  }
}
if (currentDay === 5) { 
  document.getElementById("mon-thurs").style.display = "none";
  if (currentHour >= 8 && currentHour <= 17.5) { 
    document.getElementById("closed").style.display = "none";
  }
  else {
    document.getElementById("open").style.display = "none";
  }
}
if (currentDay === 0 || currentDay === 6) { 
  document.getElementById("open").style.display = "none";
  document.getElementById("friday").style.display = "none";
}
};

setInterval(openClose(), 5000);
.layout-header .header-one #open,
.layout-header .header-one #closed,
.layout-header .header-one #friday,
.layout-header .header-one #mon-thurs {
   display: inline-block;
   color: #000;
}
<div id="open-display">
 <div id="open">Open</div>
 <div id="closed">Closed</div>
 <div id="mon-thurs">Sales: 8am - 8pm</div>
 <div id="friday">Sales: 8am - 5:30pm</div>
</div>

\$\endgroup\$
4
\$\begingroup\$

The first bug is that you are misusing setInterval(). You should be passing a function, not the void result of the function, when calling setInterval().

Even if you had used setInterval() correctly, you still have a second bug, which is that currentDate is set just once when the code loads. If you call openClose() every five seconds, you would get the same result every time!

Third, the JavaScript sets .style.display = 'none', but never resets elements to be visible. Therefore, even if the first two issues were corrected, your page would appear emptier and emptier through the course of the week.

The code should degrade gracefully in the absence of JavaScript.

Display decisions should be written in CSS. Ideally, the JavaScript should refrain from making decisions like .style.display = 'none', and should restrict itself to doing the minimum necessary (i.e. setting classes) to let the CSS work.

There is a giant maintenance headache: the hours are encoded both in the HTML and the JavaScript. It is likely that someone in the future would edit one but forget to edit the other accordingly. Furthermore, the JavaScript representation is both cryptic (what does 17.5 mean?) and cumbersome (all those if conditions!). I recommend changing the JavaScript code so that it interprets the opening hours from the HTML. The easiest way would be to store the hours redundantly as machine-readable attributes in the HTML.

function isOpen(timeRangeEl, date) {
    var day = '' + date.getDay();
    var hhmm = ('0' + date.getHours()).slice(-2) + ':' + ('0' + date.getMinutes()).slice(-2);

    var days = timeRangeEl.getAttribute('data-days');
    var openTime = timeRangeEl.getAttribute('data-open');
    var closeTime = timeRangeEl.getAttribute('data-close');
    return days.indexOf(day) >= 0 && openTime <= hhmm && hhmm < closeTime;
}

function openClose() {
    var date = new Date();
    var display = document.getElementById('open-display');
    var els = display.getElementsByClassName('timerange');
    var anyActive = false;
    for (var i = 0; i < els.length; i++) {
        if (isOpen(els[i], date)) {
            anyActive = true;
            els[i].className = els[i].className.replace(/ *inactive\b/g, '');
        } else if (els[i].className.indexOf('inactive') < 0) {
            els[i].className += ' inactive';
        }
    }
    if (anyActive) {
        display.className = 'open';
    } else {
        display.className = 'closed';
    }
}

setInterval(openClose, 5000);
openClose();
#open-display.open > .timerange.inactive,
#open-display.open .days {
     display: none;
}
#open-display.open > .openclosed::before {
    content: 'Open';
}
#open-display.closed > .openclosed::before {
    content: 'Closed';
}
<div id="open-display">
 <div class="openclosed"></div>
 <div class="timerange" data-days="1,2,3,4" data-open="08:00" data-close="20:00">Sales: <span class="days">Mon - Thu</span> 8am - 8pm</div>
 <div class="timerange" data-days="5" data-open="08:00" data-close="17:30">Sales: <span class="days">Fri</span> 8am - 5:30pm</div>
</div>

\$\endgroup\$
  • \$\begingroup\$ I think we should call the function once before setting the interval because I can see text changes after some time. \$\endgroup\$ – CodeYogi Sep 27 '17 at 12:07
  • \$\begingroup\$ And yes you are right, I think presentation should go to CSS, data to HTML and logic to JS. \$\endgroup\$ – CodeYogi Sep 27 '17 at 12:08
2
\$\begingroup\$

I concur with the great points that 200_success♦ made. The suggestions below might not make the code much more re-usable, but should make it more robust.

DOM-queries.

Be aware that every query of the DOM is expensive! It is best to cache lookups for elements. For more information about this, I suggest reading Stop writing Slow Javascript - especially the section labeled Cache DOM Lookups.

Using that information, it is good to declare the variables for the DOM lookups outside the functions so they can be re-used. Additionally, the code should wait until the DOM is ready, lest the queries to get elements fail due to an external dependency that requires a long time to load. For this, use document.addEventListener() and pass the type argument DOMContentLoaded, followed by the code to be run.

In your original code, the functions like openClose() query the DOM each time for elements - e.g.:

function openClose() {
    if(currentDay >= 1 && currentDay <= 4) { 
        document.getElementById("friday").style.display = "none";

so move those to a function called when the DOM is loaded:

document.addEventListener('DOMContentLoaded', function() {
    //these could be declared with const instead of var, unless older browsers need to be supported
    var friday= document.getElementById('friday');
    var monThurs = document.getElementById("mon-thurs");
    var open = document.getElementById("open");
    var closed = document.getElementById("closed");


  function isOpen(timeRangeEl, date) {
    if(currentDay >= 1 && currentDay <= 4) { 
      friday.style.display = "none";

Use CSS

If you read that article mentioned earlier, the section Don’t touch my DOM, bro discusses how modifying the style property can cause performance losses.

Instead, add and remove classes. This can be done directly using the .className property, or using the classList interface.

Instead of modifying the style attribute, a class hidden could be added:

friday.classList.add('hidden');

where there is a CSS rule:

.hidden {
    display: none;
}

Updated code

var currentDate = new Date();
var currentMonth = currentDate.getMonth();
var currentDay = currentDate.getDay();
var currentHour = currentDate.getHours();
var currentMinute = currentDate.getMinutes();
document.addEventListener('DOMContentLoaded', function() {
  var friday = document.getElementById('friday');
  var monThurs = document.getElementById("mon-thurs");
  var open = document.getElementById("open");
  var closed = document.getElementById("closed");

  function openClose() {
    //reset display each time this function runs
    [monThurs, friday, open, closed].forEach(function(element) {
      element.classList.remove('hidden');
    });
    if (currentDay >= 1 && currentDay <= 4) {
      friday.classList.add('hidden');
      if (currentHour >= 8 && currentHour <= 21) {
        closed.classList.add('hidden');
      } else {
        open.classList.add('hidden');
      }
    }
    if (currentDay === 5) {
      monThurs.classList.add('hidden');
      if (currentHour >= 8 && currentHour <= 17.5) {
        closed.classList.add('hidden');
      } else {
        open.classList.add('hidden');
      }
    }
    if (currentDay === 0 || currentDay === 6) {
      open.classList.add('hidden');
      friday.classList.add('hidden');
    }
  };

  setInterval(openClose, 5000);
  openClose();
});
.layout-header .header-one #open,
.layout-header .header-one #closed,
.layout-header .header-one #friday,
.layout-header .header-one #mon-thurs {
  display: inline-block;
  color: #000;
}

.hidden {
  display: none;
}
<div id="open-display">
  <div id="open">Open</div>
  <div id="closed">Closed</div>
  <div id="mon-thurs">Sales: 8am - 8pm</div>
  <div id="friday">Sales: 8am - 5:30pm</div>
</div>

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

Great answers from 200_success♦ and Sam Onela.

Maybe I can throw in my two cents here as well and this is more like a suggestion rather than a full featured example. Let's take a look at this:

setInterval(openClose, 5000);

You run the check every 5 seconds over and over again.

This shouldn't be necessary because you actually know when you have to run the script again: the moment the state open|closed changes.

If you store the state somewhere and calculate the difference between now and then you only need to tun your script once using setTimeout:

Example HTML:

<div data-target data-open="8:00" data-close="18:0"></div>

JavaScript:

var element = document.querySelectorAll('[data-target]')[0];
var state = 'open';

function changeState() {
  state = 'open' === state ? 'closed' : 'open';
  alert(state);
}

function runNext() {
  var now = new Date();
  var upcoming;
  var nextChange;
  var deltaDay = 0;
  var deltaMS;

  if ('open' === state) {
    nextChange = element.getAttribute('data-close').split(':');
  } else {
    nextChange = element.getAttribute('data-open').split(':');
    if (24 > now.getHours()) {
      deltaDay = 1;
    }
  }

  upcoming = new Date(
    now.getFullYear(),
    now.getMonth(),
    now.getDate() + deltaDay,
    nextChange[0],
    nextChange[1],
    0,
    0
  );

  deltaMS =  upcoming - now;

  setTimeout(function() {
    changeState();
  }, deltaMS); 
}

runNext();

You can test it in this fiddle by modifying the data-close-attribute.


Important note: In this example the store would open and close every day at the same time. Of course this must be extended with the actual working days and their respective opening hours - coming from a configuration object for example. Also state must be set initially and could be a boolean too – used a string to make the example easier to follow.

\$\endgroup\$

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.