Set theory clock code

I wrote some JS to power an HTML/CSS "set theory" clock, and I'd like to share what I have so far and ask if anyone can suggest improvements.

Specifically, I'm wondering:

• Should I bother checking to see if a variable is greater than zero before running the for loop?

• Is there a better way to handle togging the classes? Setting all elements to "off" and then turning "on" the correct lamps seems a little heavy handed.

• Should the tick() function be broken up?

For added context, you can view the code in context on GitHub and here is a live demo.

$(document).ready(function () {
    startClock();
});

function startClock() {
    'use strict';
    var delay = 1000;

    function tick() {
        var clockDate = new Date(),
            clockHours = clockDate.getHours(),
            clockMinutes = clockDate.getMinutes(),
            clockSeconds = clockDate.getSeconds(),
            fiveHours = Math.floor(clockHours / 5),
            fiveMinutes = Math.floor(clockMinutes / 5),
            singleHours = Math.floor(clockHours - (fiveHours * 5)),
            singleMinutes = Math.floor(clockMinutes - (fiveMinutes * 5));

        $(".circle").toggleClass("off");
        $("#display").text(clockHours + ":" + ((clockMinutes  0) {
            for (var i = 1; i  0) {
            for (var i = 1; i  0) {
            for (var i = 1; i  0) {
            for (var i = 1; i <= singleMinutes; i++) {
                $("#m1-" + i).removeClass("off");
            }
        }

        setTimeout(tick, delay);
    }

    tick();
}

To answer your questions up front:


Should I bother checking to see if a variable is greater than zero before running the for loop?

No need. The first thing the loop does is the i 0).


Is there a better way to handle togging the classes? Setting all elements to "off" and then turning "on" the correct lamps seems a little heavy handed.

jQuery's toggleClass() works in a few different ways. One of them accepts a boolean as its 2nd argument, which forces the toggle state: If true, the class name is added (if missing), if false, the class name is removed (if present).

So for instance, you might do this:

lamps.each(function (index) {
  $(this).toggleClass("on", index < count);
});

Where lamps is a set of elements you want to manipulate, and count is the number of lamps that should light up.

I've also flipped the on/off states, so on is the class name you add, while the lamps by default are off. This isn't really important, but considering you call the elements "lamps" (albeit indirectly; they're in divs classed as lamp-rows), it seems ever-so-slightly more natural to me that they default to their off state.

Anyway, the point is that you loop through each "lamp", turning it on or off as needed, instead of turning them all off first.


Should the tick() function be broken up?

You've got some repetition with the loops and all that, so it'd probably be a good idea to extract the repeated parts. For instance, you could package the each-loop above in a function:

function updateLamps(lamps, count) { ... }

Then simply pass it a set of elements, and the "lit lamp count".

For this, it might also be worth taking a look at the HTML. Right now, you have IDs for each individual lamp. But that's a tad too specific for my liking, since they're all essentially the same kind of thing. No great need to give them unique IDs.

I'd suggesting IDing the rows instead, and use a selector like #single-hours > .lamp (for instance) to find the lamps, where #single-hours is the ID of the row. This will also allow you to find them once and simply loop through them on each tick, rather than re-finding them each time.

Incidentally, you use a class name selector to find the blinking circle, although that, if anything, should have an ID as it's a unique element.

Another alternative is to build the elements in code, and inject them into the page. The advantage would be that you don't have tight coupling between the markup and the JS (if either changes, the other might well break). The disadvantage is that you now have markup in you JS (while the CSS is still somewhere else). While I'd suggest you try it out, I won't necessarily recommend one approach over the other, when it's just a single page/single feature thing. (If it's instead intended as a widget meant to be added to any kind of page, it's a different story).

Other things:

-
Instead of Math.floor(clockMinutes - (fiveMinutes * 5)) you can simply use the modulo operator to get the remainder of a division: clockMinutes % 5. Same for singleHours of course.

-
The line used to update the #display element is exceedingly long and repetitive. Consider making a formatTime function (or similar) to handle the heavy-lifting.

-
You have classes like rectangle-*, yellow and red which describe appearance much to specifically. What if you decide the hours should be purple triangles? The classes they have now are suddenly misleading. Always aim for class names the describe what something is, not how it looks.

-
Speaking of semantics, you could consider using ol list elements for the lamps, since they are essentially ordered lists of elements.

-
The whole $(document).ready(...) thing can be shortened to simply $(startClock);

-
Lastly, you could consider skipping some steps (loops) entirely. For instance, only if singleMinutes == 0 is there a need to update "five minute" lamps; only if fiveMinutes is zero, do you need to update the "single hour" lamps, etc.. In other words, you only need to update the more "coarse" steps, if the finer-grained ones roll over.

Here's a snippet that incorporates some of the comments above.



`function startClock() {
'use strict';
var delay = 1000,
display = $("#display"),
secondsLamp = $("#seconds-lamp"),
fiveHourLamps = $("#five-hour-lamps > div"),
oneHourLamps = $("#one-hour-lamps > div"),
fiveMinuteLamps = $("#five-minute-lamps > div"),
oneMinuteLamps = $("#one-minute-lamps > div");

function toggleLamps(lamps, count) {
lamps.each(function (index) {
$(this).toggleClass('off', index >= count);
});
}

function tick() {
var clockDate = new Date(),
clockHours = clockDate.getHours(),
clockMinutes = clockDate.getMinutes(),
clockSeconds = clockDate.getSeconds(),
fiveHours = Math.floor(clockHours / 5),
fiveMinutes = Math.f

StackExchange Code Review Q#72056, answer score: 4