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

Hours:Minutes Addition

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

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=0 && mm


Simple Hours:Minutes Adder

Enter duration in format hh:mm and select ok, running total at top

text









`

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 ` and deeper than ,
so instead of:


    Simple Hours:Minutes Adder


This would be better:


    
        
    
    
        Simple Hours:Minutes Adder


HTML5 validation

In HTML5, the
tag should not have a closing .
The first
tag here violates that:



There are some other HTML5 violations, for example missing
.
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;
}

Code Snippets

<!DOCTYPE html>
<html>
<head>
<script src="behavior.js"></script>
</head>
<body>
    <h1>Simple Hours:Minutes Adder</h1>
<!DOCTYPE html>
<html>
    <head>
        <script src="behavior.js"></script>
    </head>
    <body>
        <h1>Simple Hours:Minutes Adder</h1>
<input id="input_text" type="text"></input>
<input id="add_button" type="button" value="add">
<input id="pop_button" type="button" value="pop item">
if (1) { //add validation, input must be hh:mm
if (true) { //add validation, input must be hh:mm

Context

StackExchange Code Review Q#115261, answer score: 4

Revisions (0)

No revisions yet.