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 <= x
comparison. And since i
starts at 1, there's no need for you to double-check by saying if(x > 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-row
s), 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.floor(clockMinutes / 5),
singleHours = clockHours % 5,
singleMinutes = clockMinutes % 5;
secondsLamp.toggleClass("off");
display.text(clockHours + ":" + ((clockMinutes < 10) ? "0" + clockMinutes : clockMinutes) + ":" + ((clockSeconds < 10) ? "0" + clockSeconds : clockSeconds));
toggleLamps(fiveHourLamps, fiveHours);
toggleLamps(oneHourLamps, singleHours);
toggleLamps(fiveMinuteLamps, fiveMinutes);
toggleLamps(oneMinuteLamps, singleMinutes);
setTimeout(tick, delay);
}
tick();
}
$(startClock);
body {
background: rgba(0,0,0,0.35);
}
.clock {
width: 16rem;
margin: 3em auto 0 auto;
font-family: sans-serif;
}
.circle {
width: 5rem;
height: 5rem;
border-radius: 50%;
border: 1rem solid rgba(204, 204, 204, 1);
margin: 0 auto 0 auto;
}
.lamp-row {
border-radius: 1rem;
background: rgba(204, 204, 204, 1);
display: flex;
padding: 0.5rem;
margin: 0.5rem 0 0 0;
}
.rectangle-large, .rectangle-small {
border-radius: 0.5rem;
height: 2.5rem;
box-sizing:border-box;
-moz-box-sizing:border-box;
-webkit-box-sizing:border-box;
margin: 0.1rem;
display: inline-block;
}
.rectangle-large {
width: 4rem;
}
.rectangle-small {
width: 1.45rem;
}
.red {
background: rgba(255, 0, 0, 1);
}
.red.off {
background: rgba(255, 0, 0, 0.5);
}
.yellow {
background: rgba(255, 255, 0, 1);
}
.yellow.off {
background: rgba(255, 255, 0, 0.4);
}
#display {
margin-top: 1rem;
text-align: center;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<div class="clock">
<div>
<div id="seconds-lamp" class="circle yellow"></div>
</div>
<div id="five-hour-lamps" class="lamp-row">
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
</div>
<div id="one-hour-lamps" class="lamp-row">
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
<div class="rectangle-large red"></div>
</div>
<div id="five-minute-lamps" class="lamp-row">
<div class="rectangle-small yellow"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small red"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small red"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small red"></div>
<div class="rectangle-small yellow"></div>
<div class="rectangle-small yellow"></div>
</div>
<div id="one-minute-lamps" class="lamp-row">
<div class="rectangle-large yellow"></div>
<div class="rectangle-large yellow"></div>
<div class="rectangle-large yellow"></div>
<div class="rectangle-large yellow"></div>
</div>
<div>
<div id="display"></div>
</div>
</div>