JS Clock widget

Question

I want to get some best practices and code corrections.

JS

function clock(){
 var currentTime = new Date()
 var hours = currentTime.getHours()
 var minutes = currentTime.getMinutes()
 var seconds = currentTime.getSeconds()
 var timeOfDay = ( hours < 12 ) ? "AM" : "PM";
 hours = ( hours > 12 ) ? hours - 12 : hours;
 hours = ( hours === 0 ) ? 12 : hours;
 minutes = ( minutes < 10 ? "0" : "" ) + minutes;
 seconds = ( seconds < 10 ? "0" : "" ) + seconds;
 var place = (hours + ":" + minutes + ":" + seconds + timeOfDay);
 document.getElementById('clock').innerHTML = place;
 setTimeout(clock,1000);
};
clock();

CSS

#clock {
 font-family:arial,helvetica;
 background:#FFF;
 box-shadow:3px 3px 5px 6px #ccc;
 width:86px;
 -webkit-box-shadow: 0 10px 6px -6px #777;
 -moz-box-shadow: 0 10px 6px -6px #777;
 box-shadow: 0 10px 6px -6px #777;
}

HTML

<div id="clock">Time</div>

Answer 1

initial findings:

You aren't using the init function, so why keep it. Whatever code you don't need: get rid of it.
Your clock function calls itself sort-of recursively with a 1 second interval, due to your using the setTimeout function.
You could easily avoid calling setTimout every time by simply changing that to setInterval:

function clock()
{
 //code here
}
var clockInterval = setInterval(clock, 1000);

The result of this is that, through the return value of setInterval (which is the interval's ID) you can stop the code from running whenever you need to:

//some event handler, for example a pauseBtn.addEventListener('click'...)
clearInterval(clockInterval);

which makes gives you more flexibility, and will make things easier when you want to debug or indeed add functionality later on.
The same can be done with an timeout, of course:

var tId = setTimeout(function()
{
 console.log('~5 seconds have passed');
}, 5000);
clearTimeout(tId);//the callback won't ever be called, unless for some reason this statement takes more than 5 seconds to reach.

However, an interval's ID is the same throughout. If you decide to use setTimeout, you'll have to re-assign the return value of every setTimout call to the same variable, simply because each timeout can, and most likely will, have a new ID. For that reason alone I think setInterval is the better choice in your case.

in the function, you have this: document.getElementById('clock') DOM lookup that queries the DOM for the same element over and over again. The DOM API is what it is: clunky and slow, so wherever you can, saving on DOM queries is a good thing. The element in question is, as far as your function is concerned pretty much a constant, is it not?
Why not use a closure, so you can query the DOM once, and use the same reference over and over? Something like this:

var clock = (function(clockNode)
{//clockNode will remain in scope, and won't be GC'ed
 return function()
 {//the actual clock function
 //replace document.getElementById('clock') with
 clockNode.innerHTML = place;
 };
}(document.getElementById('clock')));//pass DOM reference here
var clockInterval = setInterval(clock, 1000);

However, it's worth noting that innerHTML is quite slow, looks dated and is non-standard. The "correct" way of doing things would be:

clockNode.replaceChild(document.createTextNode(place), clockNode.childNodes.item(0));

Which, admittedly looks a tad more verbose and over-complicates things, but just thought you'd like to know.

Speaking of over-complicating things, and as phyrfox rightfully pointed out, you're essentially building a locale time string from the Date instance. You are re-inventing the wheel, considering the Date object comes with a ready-made method for just such a thing: toLocaleTimeString. This dramatically simplifies our code:

clockNode.replaceChild(
 document.createTextNode((new Date()).toLocateTimeString()),
 clockNode.childNodes.item(0)
);

That's all you need!

here's a fiddle

Oh, and remember how I said that the interval ID returned by setInterval can be a useful thing to have?

Here's another fiddle that illustrates that point

The code you have can, therefore be re-written to be more standard-compliant and at the same time be made shorter. What you end up with is simply this:

var clock = (function (clockNode)
{
 return function()
 {
 clockNode.replaceChild(
 document.createTextNode(
 (new Date).toLocaleTimeString()
 ),
 clockNode.childNodes.item(0)
 );
 };
}(document.getElementById('clock')));
var clockInterval = setInterval(clock, 1000);

Conventions
Yes, semi-colons are optional in JS, but it is considered a good habit to write them nonetheless. That's an easy convention to follow, and it'll help you when you decide to learn, or have to switch to, other languages. Many of which see the semi-colon as being non optional.
It's a bit of a hang-up of mine, I admit, but try to adhere to the conventions as much as possible, like: avoiding too many var declarations in a block of code. You can easily compact them into a single statement, using a comma:

var currentTime = new Date(),
 hours = currentTime.getHours(),
 minutes = currentTime.getMinutes(),
 seconds = currentTime.getSeconds(),
 timeOfDay = ( hours < 12 ) ? "AM" : "PM";

You can do away with the temp var place all together, and can even move the var declarations to the IIFE which we've used to store the clockNode DOM reference in:

var clock = function(clockNode)
{
 var currentTime, hours, minutes, seconds, timeOfDay;
 return function(){};
}());

Pass your code through the rather pedantic JSLint tool never hurts.

It'll probably complain about your keenness on using the ternary operator, too, because, though I'm not particularly opposed to the odd ternary, you do seem to be using it an awful lot.

Answer 2

Your clock is going to be between 0 and 1 seconds behind local system time, since your function isn't fixed to fire at the turn of the second. Once in a while a second will be missed entirely due to a small delay beyond the 1000 ms, contrary to popular belief setInterval show a similar delay in most browsers, you never get exactly the time you ask for.

setTimeout(clock,1000-currentTime%1000);

Will to the best of the browsers timekeeping ability fire at the turn of the second, if the function is fired early (which does happen in some versions of some browsers), it will schedule for being fired again at the correct time, and when it is fired slightly late it will compensate.

That is all there is to say about the functionality.

You are not using the init function, so why is it there? If you were using it it would create the global timeDisplay, which seem to be intended to be a private variable of the function.

You have box-shadow twice in your CSS and your use of semicolons and whitespace is inconsistent.

• itsariadust: Thre is an error on the setTimeout(clock,1000-currentTime%1000);. The error is: Uncaught ReferenceError: currentTime is not defined
• aaaaaaaaaaaa: @jakobaindreas_11 It has to be placed within the clock function, replacing setTimeout(clock,1000);. That is where you have currentTime defined.