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

Optimizing a JavaScript snippet for portability and readability

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

Problem

We have an in-house time-tracking application at the office.

We, the employees, can view an overview of our hours on a web page. This web page renders a table, with worked hours per day in table cells, which is styled as a monthly calendar. I wrote a small script which I can copy-paste in my console to get a total of hours worked too much for that month.

A relevant snippet of the HTML.


10
     
    
        08:33  
    

11
     
    
        08:25  
    


The JavaScript snippet.

var elementsToParse = $(".link").find("font:not(:empty)");
var totalMinutes = 0;
$.each(elementsToParse, function(index, element) {  
    var text = $(element).text();
    var hours = parseInt(text.substr(1, 2));            
    var minutes = parseInt(text.substr(3, 2));

    var elementTotal = ((hours * 60) + minutes) - 480;

    totalMinutes += elementTotal;
});

alert('Overtime so far: ' + totalMinutes + ' minutes');


Any thoughts on how to make the script more compact or elegant, while maybe improving readability?

Solution

I would make it less compact to improve readability. Leave it up to your build process (minifier) to make things compact. Your goal should be to make things understandable.

Use regular expressions

In cases like this, where you have a strict format, I love using regular expressions:

/** @const */
var TIME_FORMAT = /(\d{2}):(\d{2})/;
...
"10:30".match(TIME_FORMAT) // == ["10:30", "10", "30"]


This makes it clear that you expect a date to consist of two digits a colon, and two more digits. As a bonus, let everyone know it's a constant with JSDoc and common coding conventions.

Define functions

Short modular code is easy to read:

/**
 * Parses a time string and returns the hours and minutes.
 */
function parseTime(text) {
    var match = String(text).match(TIME_FORMAT);

    if (match === null) {
        throw new Error('Time "' + text + '" could not be parsed.');
    }

    return {
        hours:   Number(match[1]),
        minutes: Number(match[2])
    };
}

/**
 * Gets the total minutes of overtime based on time worked and number of working hours
 * in a day.
 */
function calculateOvertime(parsedTime, workingHours) {
    var minutesWorked = parsedTime.hours * 60 + parsedTime.minutes;
    var minutesRequired = workingHours * 60;
    return minutesWorked - minutesRequired;
}


Here, the parseTime parses the time, returning an object, and calculateOvertime calculates the overtime.

Put it all together

Putting it together, your actual function does become more compact and more readable because tangental functions are extracted:

/**
 * Gets the total minutes of overtime from a tracking table.
 *
 * @param selector A CSS2.1 selector, DOM element or jQuery object pointing to the
 * tracking table.
 */
function getOvertimeFromTable(selector, dailyWorkingHours) {
    var totalMinutesOfOvertime = 0;
    var $dailyTimes = $(selector).find("font:not(:empty)");

    $dailyTimes.each(function () {
        var parsedTime = parseTime($(this).text());
        var minutesOfOvertime = calculateOvertime(parsedTime, dailyWorkingHours);

        totalMinutesOfOvertime += minutesOfOvertime;
    });
}

alert('Overtime so far: ' + getOvertimeFromTable('.links', 8) + ' minutes');


Protecting global, enabling compilation

Right now we are exposing a variety of variables and functions on the global object. This means that anyone could call parseTime or read a similar function. It also means a minifier can't rename the function to a shorter name. Wrapping everything in a closure will be the final step for making sure your output code is more compact and your source needn't be:

/**
 * Alerts the number of hours of overtime you have accumulated so far in this pay cycle,
 * assuming a standard 8-hour day.
 */
(function () {
    /* Your code here */
}());

Code Snippets

/** @const */
var TIME_FORMAT = /(\d{2}):(\d{2})/;
...
"10:30".match(TIME_FORMAT) // == ["10:30", "10", "30"]
/**
 * Parses a time string and returns the hours and minutes.
 */
function parseTime(text) {
    var match = String(text).match(TIME_FORMAT);

    if (match === null) {
        throw new Error('Time "' + text + '" could not be parsed.');
    }

    return {
        hours:   Number(match[1]),
        minutes: Number(match[2])
    };
}

/**
 * Gets the total minutes of overtime based on time worked and number of working hours
 * in a day.
 */
function calculateOvertime(parsedTime, workingHours) {
    var minutesWorked = parsedTime.hours * 60 + parsedTime.minutes;
    var minutesRequired = workingHours * 60;
    return minutesWorked - minutesRequired;
}
/**
 * Gets the total minutes of overtime from a tracking table.
 *
 * @param selector A CSS2.1 selector, DOM element or jQuery object pointing to the
 * tracking table.
 */
function getOvertimeFromTable(selector, dailyWorkingHours) {
    var totalMinutesOfOvertime = 0;
    var $dailyTimes = $(selector).find("font:not(:empty)");

    $dailyTimes.each(function () {
        var parsedTime = parseTime($(this).text());
        var minutesOfOvertime = calculateOvertime(parsedTime, dailyWorkingHours);

        totalMinutesOfOvertime += minutesOfOvertime;
    });
}

alert('Overtime so far: ' + getOvertimeFromTable('.links', 8) + ' minutes');
/**
 * Alerts the number of hours of overtime you have accumulated so far in this pay cycle,
 * assuming a standard 8-hour day.
 */
(function () {
    /* Your code here */
}());

Context

StackExchange Code Review Q#13705, answer score: 3

Revisions (0)

No revisions yet.