Simple web alarm clock

Question

I decided to strengthen my "vanilla JavaScript" skills since I've lately been getting more into jQuery and just discovered the potential of node.js.

As a result, I decided to make a basic alarm clock in javascript and html. (Plus I needed an alarm clock).

Here's the Fiddle

// Load an alarm sound
 var sound = new Audio("http://www.freespecialeffects.co.uk/soundfx/sirens/alarm_01.wav");
 sound.loop = true;
// I just made this because It's easier to read.
getID = function(value){ return document.getElementById( value ); };
// List of all my objects im working with, and variables
var hour = getID("hour"),
 minute = getID("minute"),
 second = getID("second"),
 aHour = getID("aHour"),
 aMinute = getID("aMinute"),
 aSecond = getID("aSecond"),
 aSwitch = getID("aSwitch"),
 aOff = getID("turnOff"),
 refreshTime = 500,
 alarmTimer = null;
aSwitch.On = false;
aSwitch.value = "OFF";
// Turns the alarm off or on
function alarmSwitch(){
 switch( aSwitch.On )
 {
 case false:
 aSwitch.On = true;
 aSwitch.value = "ON";
 alarmSet();
 break;
 case true:
 aSwitch.On = false;
 aSwitch.value = "OFF";
 // CLEARS THE BEEPER 
 clearTimeout( alarmTimer );
 break;
 }
}
// Stops the alarm and closes the "stop button"
function disableAlarm(){
 sound.pause();
 aOff.style.display = "none";
}
//Fires the BEEPER noise, -- this is called from the alarmTimer timeout
function alarmFire(){
 if( aSwitch.On )
 {
 aOff.style.display = "block";
 sound.play();
 }
 else
 alert("error..");
}
/*This is how the beeper goes off:
 * It first checks the set time so the alarm can go to the 
 * next day without the user putting the date in.
 * The beeper goes off over the span of milliseconds difference.
 */
function alarmSet(){
 clearTimeout( alarmTimer );
 var tomo = false;// tomorrow.
 if( aHour.value < hour.value )
 {tomo = true;}
 else if( aHour.value == hour.value && aMinute.value < minute.value )
 {tomo = true;}
 else if( aHour.value == hour.value && aMinute.value == minute.value 
 && aSecond.value < second.value )
 {tomo = true;}
 var date = new Date(), year = date.getFullYear(), month = date.getMonth(), day = parseInt( date.getDate() );
 if( tomo ){day += 1;}
 time = new Date( year, month, day, aHour.value, aMinute.value, aSecond.value, date.getMilliseconds() );
 time = ( time - (new Date()) );
 // This turns the alarm back on if it's off when this function is called.
 if( aSwitch.On == false)
 alarmSwitch();
 alarmTimer = setTimeout( function(){alarmFire();} ,parseInt(time) );
}
// This is how the clock works
timeRefresh = function(){
 date = new Date();
 hour.innerHTML = date.getHours();
 hour.value = hour.innerHTML;
 minute.innerHTML = date.getMinutes();
 minute.value = minute.innerHTML;
 second.innerHTML = date.getSeconds();
 second.value = second.innerHTML;
 setTimeout("timeRefresh()", refreshTime);
};
// This is called whenever the inputs are changed to stop invalid forms.
numCap = function( obj, min, max){
 obj.value = Math.max(obj.min, Math.min(obj.max, obj.value) );
 alarmSet();// Starts up the alarm automatically when a value is changed.
};
window.onload=function(){
 timeRefresh();
 var a = getID("aHour");
 a.value = hour.innerHTML;
 a = getID("aMinute");
 a.value = minute.innerHTML;
 a = getID("aSecond");
 a.value = second.innerHTML;
};

I would love for some feedback on my code! What could I do to improve/simplify the code?

Also, if you would like to see the HTML page along with it, just ask!

• Pinoniq: you are poluting the document window. Look into the module pattern (or other) to encapsulate your logic.
• Andrew: @Pinoniq Thanks for the advice, I've never even heard of that before!

Answer 1

First of all, indent and use whitespace consistently. I find using braces always a good style too.

Do not pollute the global namespace: wrap everything in a function that keeps the variables local to a smaller scope. Also use "use strict"; and test in Firefox as it is one of the least lenient browsers in strict mode:

(function ourAlarmCode () {
 "use strict";
 var sound = ...
 // all your code goes here
}());

Use consistent variable naming - usually CamelCase signifies a constructor, thus on instead of On, since it is not a constructor.

Name all the functions, even if you store them in variables - this makes debugging easier.

var numCap = function numCap (obj, min, max) {

Do not use random properties on DOM objects - instead store them as variables - thus replace aSwitch.On with var alarmOn = false;.

The switch can be replaced with an if

// toggle
alarmOn = !alarmOn;
if (alarmOn) {
 aSwitch.value = "ON";
 alarmSet();
}
else {
 aSwitch.value = "OFF";
 clearTimeout(alarmTimer);
}

Also, setTimeout is not guaranteed to trigger after exactly N milliseconds - in this case does not matter but in general it should be kept in mind that it is a time-out functionality to ensure that at least N milliseconds have elapsed since the callback is invoked.

Answer 2

This is not a complete review, just a few things that jumped out to me:

getID()

Looks more like "easier to write" than "easier to read". From current function name "getID", most people reading the code will assume that the function will return an ID.

If you would like to use jQuery as you can simply write $("#hour") instead of getID("hour"). The core idea of jQuery is to let you select (or query) DOM elements.

alarmSwitch()

If you use simple if/else instead of the switch/case, the code will be much simpler.

if(aSwitch.On){
...
} else {
...
}

Variable Naming

Simply naming the variable tomorrow instead of tomo would have been better for read

var tomo = false;// tomorrow.

If/else blocks for single line

Consistently using if/else blocks, even for single line statements helps avoid hard to find bugs

if( aSwitch.On )
{
 aOff.style.display = "block";
 sound.play();
}
else
 alert("error..");

• Andrew: Thanks for the feed back! I actually practice all of those things you said: Variable naming, if else blocks, and I guess I wrote the alarmSwitch() function like that because I was so tired. --- And getID was made for easier reading by me personally, meaning I can glance at a shorter line of code versus a lot of document.getElementById() 's. But you're right, it's not a good solution. +1
• Lukazoid: It would perhaps be clearer if it were named getById or getElement or even getElementById, I think the point Tahir is making is that it is not returning an Id, but an element.

Answer 3

Everything should be wrapped in a function, for easier reading..

The alarm looks good!