JavaScript clock

Question

I am trying to build a clock. You can see the working code here. However, I feel like I can do much better in the JavaScript code.

var getDate = function getDate() {
 var date = new Date();
 var month = date.getMonth();
 var day = date.getDate();
 var hour = date.getHours();
 var minutes = date.getMinutes();
 var monthNames = ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December'];
 var ampm = 'pm'; // by default, it is pm
 var showDate;
 var showTime;
 if (hour < 10) {
 hour = '0' + hour;
 }
 if (minutes < 10) {
 minutes = '0' + minutes;
 }
 if (hour < 12) {
 ampm = 'am';
 }
 if (hour > 12) {
 hour -= 12;
 }
 showDate = monthNames[month] + ' ' + day;
 showTime = hour + ':' + minutes + ampm;
 document.getElementById('js-date').innerHTML = showDate;
 document.getElementById('js-time').innerHTML = showTime;
 requestAnimationFrame(getDate);
};
getDate();

Answer 1

A few improvements:

var getDate = function getDate() {

Added some line breaks to better group the functionalities.

 var date = new Date();
 var month = date.getMonth();
 var day = date.getDate();
 var hour = date.getHours();
 var minutes = date.getMinutes();
 var monthNames = ['January', 'February', 'March', 'April', 'May', 'June', 'July', 'August', 'September', 'October', 'November', 'December'];

Better variable name for ampm, and setting empty string as default value.

 var timePeriod = ''; 
 var dateString;
 var timeString;
 if (hour < 10) {
 hour = '0' + hour;
 }
 if (minutes < 10) {
 minutes = '0' + minutes;
 }

You could capitalize the time period.

 if (hour < 12) {
 timePeriod = 'AM'; 
 } else {
 timePeriod = 'PM';
 }

Some people would prefer to collapse this if block into one line using hour = hour % 12, but yours is okay given it is simpler to understand.

 if (hour > 12) {
 hour -= 12;
 }
 dateString = monthNames[month] + ' ' + day;

Added a space between time string and time period string.

 timeString = hour + ':' + minutes + ' ' + timePeriod;

You might want to rename your span blocks to something like 'date' and 'time', instead of 'js-date' and 'js-time' because ID does not have to be an indicator of how the span gets its value.

 document.getElementById('js-date').innerHTML = dateString; 
 document.getElementById('js-time').innerHTML = timeString;

You must not use requestAnimationFrame for this application. It is typically called 60 times in a second, and it would be wasteful to update your watch. You can just use a setInterval as shown below. This would trigger your getDate function every second to update the UI - which is more than enough for your purpose. I would go a step further and set the duration to 60 seconds, as your watch does not display the seconds.

 // requestAnimationFrame(getDate);
};
setInterval(function() {
 getDate();
}, 1000);

• James Fenwick: Setting the interval to 60 seconds would make the clock off by +- 30 seconds
• Nisarg Shah: That's why I set it to 1 second in code, but would be ready to set it to 60 seconds nonetheless if the clock is not mission critical.

Answer 2

These statements are in a bad order:

if (hour < 10) {
 hour = '0' + hour;
}
if (minutes < 10) {
 minutes = '0' + minutes;
}
if (hour < 12) {
 ampm = 'am';
}
if (hour > 12) {
 hour -= 12;
}

The first block sometimes makes hour into a string. It is almost certainly a bad idea to make the type of a variable ambiguous.

When comparing a number with a string, the number is coerced into a string, then the comparison is done lexicographically. Your if (hour < 12) test happens to work, but not for the reason that one would expect.

Prepending a zero is a formatting operation, and formatting should be done after arithmetic. What happens if hour was initially 13? It becomes 1, with no leading zero pretended.

Finally, I would avoid dealing with minutes in the middle of code that deals with hours.

Here is what I would consider a correct order:

var ampm = (hour < 12) ? 'am' : 'pm';
if (hour > 12) {
 hour -= 12;
}
hour = (hour < 10 ? '0' : '') + hour;
minutes = ('0' + minutes).slice(-2);

Of course, pick one formatting technique or the other; don't mix them.

Answer 3

A few things:

• getDate should return the date. You should use another method to assign it to the view
• I am not certain why you're not using the standard methods of the Date object, I'm going to propose it instead if your own algorithm but I completely aknowledge the fact that I might be taking liberty in regards to your original specs.
• Finally, it's always good to think broad even for small projects. I will propose a more maintainable solution using some OOP practices below.

So I would do something such as:

/**
 * Formats dates to specific time formats.
 * @interface
 */
function ITimeFormatter() {
 /**
 * @param {Date} dateObject
 * @return {string}
 */
 this.formatTimeString = function(dateObject) {}
}
/**
 * Formats dates to specific date formats.
 * @interface
 */
function IDateFormatter() {
 /**
 * @param {Date} dateObject
 * @return {string}
 */
 this.formatDateString = function(dateObject) {}
}
/**
 * Formats dates to standard time format.
 * @implements ITimeFormatter
 */
function StandardTimeFormatter() {
 this.formatTimeString = function(dateObject) {
 dateObject = dateObject || new Date;
 return dateObject.toLocaleTimeString();
 }
}
/**
 * Formats dates to standard date format.
 * @implements IDateFormatter
 */
function StandardDateFormatter() {
 this.formatDateString = function(dateObject) {
 dateObject = dateObject || new Date;
 return dateObject.toDateString();
 }
}
function showDateAndTime() {
 document.getElementById('js-date').innerHTML = (new StandardDateFormatter()).formatDateString(new Date()); 
 document.getElementById('js-time').innerHTML = (new StandardTimeFormatter()).formatTimeString(new Date());
}
setInterval(showDateAndTime, 1000);

Answer 4

You definitely do not want to use requestAnimationFrame for your clock as you are currently implementing it. Unfortunately, the way you presently have it set up essentially creates a never ending loop (with no way of exiting itself) that will continue to re-compute and then update your clock's display approximately 60 times every second for as long as the window/tab holding it is active, when an update will only ever actually be needed every minute or so (unless you decide to add a seconds display at some point in the future).

Here is a viable alternative though, that uses setInterval instead, storing the interval timer identifier in a globally scoped variable, allowing for the interval to be cancelled at any point if needed, while also letting you set a specific time duration in regards to how often you would like the clock to update its display.

function setClock(updateInterval){
 /*destructuring assignment can make variable creation and 
 value assignment much quicker and easier*/
 var [ , month, day, , hour, minutes] = new Date().toString().split(/ |:/);
 var ampm;
 if(hour >= 12){
 ampm = 'PM'
 hour -= 12
 }else{
 ampm = 'AM'
 }
 document.getElementById('js-date').innerHTML = `${month} ${day}`;
 document.getElementById('js-time').innerHTML = `${hour}:${minutes} ${ampm}`;
 if(updateInterval){
 return setInterval(setClock, updateInterval)
 }
}
var clockTimer = setClock(/*desired update frequency here*/)

The one major difference this approach will produce, is to use an abbreviated form of the month, instead of the full month name (i.e. Feb, instead of February). You could always configure it to re-introduce full month names pretty easily if desired though.