6
\$\begingroup\$

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>

\$\endgroup\$
  • \$\begingroup\$ I don't understand the edited indention, eg why are the first two lines indented differently in addDuration? \$\endgroup\$ – nexus_2006 Dec 28 '15 at 16:54
4
\$\begingroup\$

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 of updateTotalDisplay
  • total = total + entries[i] can be simplified to total += entries[i]
  • instead of setting totaltimetext to output_prefix + formatTime(total), it would be better to have output_prefix in 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;
}
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ This validation of 0 <= mm && mm < 10 is so I can add a leading zero so minutes show :04 instead of :4 \$\endgroup\$ – nexus_2006 Dec 28 '15 at 20:11
  • \$\begingroup\$ I know. You only need mm < 10 for that. You don't need the 0 <= mm condition. \$\endgroup\$ – janos Dec 28 '15 at 20:13
  • \$\begingroup\$ I understand now \$\endgroup\$ – nexus_2006 Dec 28 '15 at 20:55
2
\$\begingroup\$
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.

| improve this answer | |
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.