Optimizing a JavaScript snippet for portability and readability

Question

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.

<td height="17" align="right" >
<font size="1">10</font>
    <b><font size="2">&nbsp;</font></b>
    <a href="/something/a" class="link">
        <b><font size="2" color=black>08:33  </font></b>
    </a>
</td>
<td height="17" align="right" ><font size="1">11</font>
    <b><font size="2">&nbsp;</font></b>
    <a href="/something/b" class="link">
        <b><font size="2" color=black>08:25  </font></b>
    </a>
</td>

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?

Answer 1

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 */
}());

Answer 2

Well, you only need to pass parameters to each if you need the index.

$.each(elementsToParse, function() {
    var text = $(this).text();

Also, a more elegant way than substr is just using split. You can trim the text before that.

var text = $.trim( $(this).text() ).split(':'),
    hours = parseInt( text[0], 10 ),
    minutes = parseInt( text[1], 10 );

Don't forget the second parameter of parseInt :-)

Answer 3

As Inkbug mentions, you could run each() straight on the query. split() is definitely an elegant means to retrieve minutes and hours. For style reasons, you probably should have 1 var inside the the each() function. You could split up 480 into 60 * 8, this makes it clear you are supposed to work 8 hours a day. Finally, if you put the code inside a self executing function, you will not mess up the global namespace, and you can then store the code as a bookmarklet.

This should work:

void((function () {
    var totalMinutes = 0;
    $(".link").find("font:not(:empty)").each(function () {
        var time = $(this).text().split(":"),
            hours = +time[0],
            minutes = +time[1],
            overtime = ((hours * 60) + minutes) - (8 * 60);

        totalMinutes += overtime;
    });

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