HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

JavaScript clock

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
javascriptclockstackoverflow

Problem

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  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();

Solution

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);

Code Snippets

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 timePeriod = ''; 
    var dateString;
    var timeString;

    if (hour < 10) {
        hour = '0' + hour;
    }

    if (minutes < 10) {
        minutes = '0' + minutes;
    }
if (hour < 12) {
        timePeriod = 'AM'; 
    } else {
        timePeriod = 'PM';
    }
if (hour > 12) {
        hour -= 12;
    }

    dateString = monthNames[month] + ' ' + day;

Context

StackExchange Code Review Q#155671, answer score: 3

Revisions (0)

No revisions yet.