Problem
I wrote this simple page to help at work, and as an exercise to get more familiar with JavaScript.
I’m especially interested in help with style and structure – is this “standard”?
I know the input isn’t validated at all – feel free to make suggestions here but this isn’t my main concern at this time.
behavior.js:
var output_prefix = 'Total duration is: ';
var entries = [];
var list_display = null;
function addDuration() {
var input_text = document.getElementById("input_text");
if (1) { //add validation, input must be hh:mm
var hhmm = input_text.value.split(":");
var input_val = Number(hhmm[0]*60) + Number(hhmm[1]);
entries.push(input_val);
updateTotalDisplay();
var new_node = document.createElement("P");
new_node.appendChild(document.createTextNode(formatTime(input_val)));
list_display.appendChild(new_node);
} else {
alert("not a number");
}
input_text.value = "";
input_text.focus();
}
function popDuration() {
entries.pop();
updateTotalDisplay();
list_display.removeChild(list_display.lastChild);
}
function updateTotalDisplay() {
var total = 0;
for (var i = 0; i<entries.length; i++) {
total = total + entries[i];
}
document.getElementById("totaltimetext").innerHTML = output_prefix + formatTime(total);
return;
}
function formatTime(num_minutes) {
var time_str = "" + Math.floor(num_minutes/60).toString() + ":";
var mm = num_minutes%60;
if (mm>=0 && mm<10) {
time_str += "0";
}
time_str += mm;
return time_str;
}
window.onload = function() {
list_display = document.getElementById("duration_list");
updateTotalDisplay();
document.getElementById("add_button").addEventListener("click", addDuration);
document.getElementById("pop_button").addEventListener("click", popDuration);
}<!DOCTYPE html>
<html>
<head>
<script src="behavior.js"></script>
</head>
<body>
<h1>Simple Hours:Minutes Adder</h1>
<p>Enter duration in format hh:mm and select ok, running total at top</p>
<div id="output_line">
<p id="totaltimetext">text</p>
</div>
<form>
<input id="input_text" type="text"></input>
<input id="add_button" type="button" value="add">
<input id="pop_button" type="button" value="pop item">
</form>
<div id="duration_list">
</div>
</body>
</html>Solution
HTML5 style
I’m not sure if there’s a standard for HTML formatting.
I think it usually depends on the team.
You might be interested in Google’s style guide for HTML.
It seems logical to indent <head> and <body> deeper than <html>,
so instead of:
<!DOCTYPE html>
<html>
<head>
<script src="behavior.js"></script>
</head>
<body>
<h1>Simple Hours:Minutes Adder</h1>
This would be better:
<!DOCTYPE html>
<html>
<head>
<script src="behavior.js"></script>
</head>
<body>
<h1>Simple Hours:Minutes Adder</h1>
HTML5 validation
In HTML5, the <input> tag should not have a closing </input>.
The first <input> tag here violates that:
<input id="input_text" type="text"></input>
<input id="add_button" type="button" value="add">
<input id="pop_button" type="button" value="pop item">
There are some other HTML5 violations, for example missing <title>.
Use a validator to catch these.
JavaScript style
Indenting using two spaces is a very common practice, so that’s good.
In general your JavaScript style seems fine, clean, easy to read.
Some minor violations:
- pointless
return;at the end ofupdateTotalDisplay total = total + entries[i]can be simplified tototal += entries[i]- instead of setting
totaltimetexttooutput_prefix + formatTime(total), it would be better to haveoutput_prefixin the HTML document, and only update the changing part, in a dedicated field
Use booleans
Instead of this:
if (1) { //add validation, input must be hh:mm
Better use proper booleans:
if (true) { //add validation, input must be hh:mm
But even better would be to replace the comment with a function call:
function is_valid(hhmm) {
//implement validation, input must be hh:mm
return true;
}
var hhmm = input_text.value.split(":");
if (is_valid(hhmm)) {
Improving the logic
Instead of re-summing the minutes stored in entries,
it would be more efficient to keep track of the running total,
and simply add the newly added number of minutes.
Improving formatTime
Instead of:
if (mm >= 0 && mm < 10) {
It’s generally more readable to arrange the terms in increasing order:
if (0 <= mm && mm < 10) {
But actually, the 0 <= mm seems pointless.
It’s reasonable to expect that this function will receive already validated number of minutes (0 or positive).
The function could also use better variable names,
and some string operations can be written simpler,
and some a variable can be eliminated:
function formatTime(total_minutes) {
var hours = Math.floor(total_minutes / 60);
var minutes = total_minutes % 60;
if (minutes < 10) {
minutes = '0' + minutes;
}
return hours + ':' + minutes;
}
function formatTime(num_minutes) {
var time_str = "" + Math.floor(num_minutes/60).toString() + ":";
var mm = num_minutes%60;
if (mm>=0 && mm<10) {
time_str += "0";
}
time_str += mm;
return time_str;
}
The mm>=0 may avoid things like converting -3 into 0-3, but it still doesn’t fix the hours portion. So formatTime(-3) returns -1:-3, which probably is not what you want. You can either return an error in that case:
if (num_minutes < 0) {
return "Error: num_minutes must be non-negative.";
}
Or convert it into something that this logic can handle:
function formatTime(num_minutes) {
var time_str = "";
if (num_minutes < 0) {
time_str = "-";
num_minutes = Math.abs(num_minutes);
}
time_str += Math.floor(num_minutes/60).toString() + ":";
var mm = num_minutes%60;
if (mm<10) {
time_str += "0";
}
time_str += mm;
return time_str;
}
Note: I’m not arguing against the changes suggested in the @janos answer. I just didn’t try to start with them.
function formatTime(total_minutes) {
var sign = "";
if (total_minutes < 0) {
sign = "-";
total_minutes = Math.abs(total_minutes);
}
var hours = Math.floor(total_minutes / 60);
var minutes = total_minutes % 60;
if (minutes < 10) {
minutes = '0' + minutes;
}
return sign + hours + ':' + minutes;
}
Similar variant starting with that version. Note that the error return stays pretty much the same either way.