Calculate inputs average

Question

I'm creating an online calculator to get the average grade of different tests my students take. I wonder if you see any problems in the following code or any way to improve it:

document.getElementById('calculator').addEventListener('click', function() {
  var physicsTests = document.getElementById('physics').getElementsByTagName('input'),
    historyTests = document.getElementById('history').getElementsByTagName('input'),
    physicsTestsCount = 0,
    historyTestsCount = 0,
    physicsAverage = document.getElementById('physicsAverage'),
    historyAverage = document.getElementById('historyAverage'),
    i;
  for (i = 0; i < physicsTests.length; i++) {
    if (physicsTests[i].value) {
      physicsTestsCount++;
    }
    if (!physicsTestsCount) {
      physicsAverage.value = 'No assessment made!';
    } else {
      physicsAverage.value = (Number(physicsTests[0].value) + Number(physicsTests[1].value) + Number(physicsTests[2].value)) / physicsTestsCount;
    }
  }
  for (i = 0; i < historyTests.length; i++) {
    if (historyTests[i].value) {
      historyTestsCount++;
    }
    if (!historyTestsCount) {
      historyAverage.value = 'No assessment made!';
    } else {
      historyAverage.value = (Number(historyTests[0].value) + Number(historyTests[1].value) + Number(historyTests[2].value)) / historyTestsCount;
    }
  }
});

<form>
  <p id="physics">
    Physics:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="physicsAverage"></output>
  </p>
  <p id="history">
    History:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="historyAverage"></output>
  </p>
  <button type="button" id="calculator">Calculate</button>
</form>

Answer 1

Not a bug, but incorrect

• You have the average calculations inside the for loop. That means you calculate the average 3 time, 2 times too many.

Improvements

• Use querySelectorAll and a CSS selector string, Its flexible and you avoid the long winded combination of queries.

• You can avoid the extra id for the outputs using the query "#subject > output"

• Use for of to iterate the query, saves you having to index into the array.

• Round the results Math.round or use toFixed to set the number of decimal points . Floating point numbers can add tiny amounts that make numbers look bad.

• Rather than hard code the calculations a function can be created that takes the subject name and then does the summing and average.

• Good UI design means preventing bad input using constraints. The input type number lets you set the min and max constraints values

  As the other answer suggested that the number input type be avoided due to accidental scrolling. Do not buy into that argument. Input type number is the correct type to use, and note that the wheel only has effect when the input is focused.

^{Note this answer assumes you are targeting ES6+ and HTML5 compliant browsers}

Example

const subjects = ["physics", "history"];
document.querySelector("button")
    .addEventListener("click",() => subjects.forEach(calcResults));

function calcResults(subject) {
    const tests = document.querySelectorAll(`#${subject} > input`);
    const ave = document.querySelector(`#${subject} > output`);
    var sum = 0, count = 0;
    for (const {value} of tests) {
        if (value !== "") { 
            sum += Number(value);
            count += 1;
        }
    }
    ave.value = count === 0 ? "No assessment." : (sum / count).toFixed(2);
}

<form>
<p id="physics">
    Physics:
    <input type="number" min="0" max="100"/>
    <input type="number" min="0" max="100"/>
    <input type="number" min="0" max="100"/>
    <output/>
</p>
<p id="history">
    History:
    <input type="number" min="0" max="100"/>
    <input type="number" min="0" max="100"/>
    <input type="number" min="0" max="100"/>
    <output/>
</p>
<button type="button">Calculate</button>
</form>

Extras

I am guessing that the two subjects are but an example and that there may be more. JavaScript makes it easy to extend repeated content.

The following example creates the UI from a simple settings object. To reduce the amount of typing 3 helper functions provide a shorthand for common DOM calls, tag creates tag and sets its properties, query does querySelectAll on document, and append, append siblings to parent element

settings = {
    subjects : ["Physics", "History", "English", "Biology"],
    numberOfGrades : 3,
    numberInput : {type: "number", min: "0", max: "100"},
}
const tag = (name, prop = {}) => Object.assign(document.createElement(name), prop);
const query = query =>  document.querySelectorAll(query);
const append = (parent, ...sibs) => {
    for (const sib of sibs) { parent.appendChild(sib) }
    return parent;    
}

addEventListener("load", () => {
    const form = query("form")[0];
    settings.subjects.forEach(subject => {
        var count = settings.numberOfGrades;
        const inputs = [];
        while (count--) { inputs.push(tag("Input", settings.numberInput)) }
        append(
            form,
            append(
                tag("p", {textContent: subject + ": ", id: subject}),
                ...inputs,
                tag("output"),
            )
        );
    });
    const button = tag("button", {textContent: "Calculate", type: "button"});
    append(form, button);
    button.addEventListener("click", () => settings.subjects.forEach(calcResults));    
});


function calcResults(subject) {
    const tests = query(`#${subject} > input`);
    const ave = query(`#${subject} > output`)[0];
    var sum = 0, count = 0;
    for (const {value} of tests) {
        if (value !== "") { 
            sum += Number(value);
            count += 1;
        }
    }
    ave.value = count === 0 ? "No assessment." : (sum / count).toFixed(2);
}

input { margin: 4px; }
p { margin: 0px; }

<form></form>

Answer 2

type=number inputs can be error-prone because it's easy to mistakenly adjust a value with the mouse wheel. Unless you specifically intend to enter grades with a mouse wheel, or have some other use for that interface, go with regular text fields.

You're testing the truth of element.value as a string and then adding it as a number. It's good practice to be explicit about these kinds of conversions.

It would be nice to generalize the code to handle any subject, without the use of hard-coding or copy-and-paste.

We only need the subject name to put a label next to the inputs; the code doesn't need to know it at all. The code snippet below uses the name=... attribute to label the inputs (via CSS) and class=subject to denote an average-eligible set of inputs. The number of inputs is determined solely by the HTML.

document.getElementById('calculator').addEventListener('click', function() {
    Array.from( document.getElementsByClassName('subject') ).forEach(subject => {
        const scores=Array.from( subject.getElementsByTagName('input') ).map( e => ""+e.value.trim() ).filter( s => s.length ).map( n => parseFloat(n) ),
          output=subject.getElementsByTagName('output'),
          sum=scores.length ? scores.reduce((partial_sum, a) => partial_sum + a) : 0,
          average=scores.length ? ( sum / scores.length ).toFixed(2) : "No assessment made!"
      if (output.length) output[0].textContent = average;
  })
});
    

.subject::before {  text-transform: capitalize; content: attr(name) ": " }
.subject > input { width:3em }

<form>
  <p class=subject name=physics>   <input> <input> <input> <output/>  </p>
  <p class=subject name=history>   <input> <input> <input> <output/>  </p>
  <p class=subject name=chemistry> <input> <input> <input> <input> <output/>  </p>
  <p class=subject name=algebra>   <input> <output/>  </p>
  <button type=button id=calculator>Calculate</button>
</form>

Answer 3

Optimisations

Based on your snippet, I assume you are writing in ES5 and not ES6. So I'm gonna use ES5 syntax below.

See code below, explanations are in comments. My focus in more in the DRY and SoC principles:

/*
1. Use objects to store elements so that:
    - they can be easily accessed and looped
    - we can easily add more subjects to calculate without writing more codes
    - we don't have to keep revisiting the DOM on every calculation
2. Use `.querySelectorAll` for:
    - shorter code
    - easily understood because of CSS syntax and better readability
*/
var inputs = {
  physics: document.querySelectorAll('#physics input'),
  history: document.querySelectorAll('#history input')
};

var outputs = {
  physics: document.querySelector('#physicsAverage'),
  history: document.querySelector('#historyAverage')
};

var calculator = document.querySelector('#calculator');


/*
1. Wrap all calculations into one function.
2. Here, it's called `calculate`.
*/
calculator.addEventListener('click', calculate);


/*
1. Split operations into separate functions so that:
    - the code is DRY.
    - it follows the Separation of Concern (SoC) principle.
*/

function calculate(){
  var subject;

  // Loop over the `inputs` object for its keys
  for (subject in inputs){
    // Wrap the actual calculations into one function (SoC).
    // So that you don't have to write repeating code (DRY).
    calculateScoreOf(subject);
  }
}

function calculateScoreOf(subject){
  var tests = inputs[subject];
  var output = outputs[subject];
  
  // No need for `count` variable in your original code.
  // The `.length` property already has the value
  var testCount = tests.length;
  
  // Use `Array.prototype.map` instead of for-loop to get all values
  // For better readability and SoC.
  var scores = Array.prototype.map.call(tests, getInputValue);
  
  // Use NaN checking for incomplete assessment indication instead of count checking (DRY)
  var hasNaN = scores.some(isNaN);
  
  var scoreTotal,
      scoreAverage;

  if (hasNaN)
    output.value = 'Incomplete assessment!';
  else {
    // Use `.reduce` to sum scores instead of hard-coding the addition (DRY and SoC)
    scoreTotal = scores.reduce(sumScores, 0);
    scoreAverage = scoreTotal/testCount;

    // Use `.toFixed` to set decimal place.
    output.value = scoreAverage.toFixed(2);
  }
}

function getInputValue(input){
  // Returns NaN if invalid input
  return parseFloat(input.value);
}

function sumScores(a, b){
  return a + b;
}

<form>
  <p id="physics">
    Physics:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="physicsAverage"></output>
  </p>
  <p id="history">
    History:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="historyAverage"></output>
  </p>
  <button type="button" id="calculator">Calculate</button>
</form>

Links

1. Don't Repeat Yourself (DRY)
2. Separation of Concerns (SoC)
3. for...in loop
4. .querySelectorAll and .querySelector
5. Array.prototype.map
6. Array.prototype.some
7. Array.prototype.reduce
8. isNaN
9. .toFixed
10. parseFloat

Extra: Code in ES6

const inputs = {
  physics: [...document.querySelectorAll('#physics input')],
  history: [...document.querySelectorAll('#history input')]
};

const outputs = {
  physics: document.querySelector('#physicsAverage'),
  history: document.querySelector('#historyAverage')
};

const calculator = document.querySelector('#calculator');

calculator.addEventListener('click', calculate);

function calculate(){
  for (const subject in inputs)
    calculateScoreOf(subject);
}

function calculateScoreOf(subject){
  const tests = inputs[subject];
  const output = outputs[subject];
  const testCount = tests.length;
  
  const scores = tests.map(el => parseFloat(el.value));
  const hasNaN = scores.some(isNaN);

  if (hasNaN)
    output.value = 'Incomplete assessment!';
  else {
    const scoreTotal = scores.reduce((a, b) => a + b, 0);
    const scoreAverage = scoreTotal/testCount;
    output.value = scoreAverage.toFixed(2);
  }
}

<form>
  <p id="physics">
    Physics:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="physicsAverage"></output>
  </p>
  <p id="history">
    History:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="historyAverage"></output>
  </p>
  <button type="button" id="calculator">Calculate</button>
</form>

Answer 4

Main updates

• Calculated the total values of each row through the loop.
• Put the average calculation outside the loop.
  Credit: Thanks to Blindman67 for this pointer!

Final code

document.getElementById('calculator').addEventListener('click', function() {
  var physicsTests = document.getElementById('physics').getElementsByTagName('input'),
    historyTests = document.getElementById('history').getElementsByTagName('input'),
    physicsTestsTotal = 0,
    historyTestsTotal = 0,
    physicsTestsCount = 0,
    historyTestsCount = 0,
    physicsAverage = document.getElementById('physicsAverage'),
    historyAverage = document.getElementById('historyAverage'),
    warning = 'Please enter a grade.',
    i;
  for (i = 0; i < physicsTests.length; i++) {
    if (physicsTests[i].value) {
      physicsTestsTotal += Number(physicsTests[i].value);
      physicsTestsCount++;
    }
  }
  if (physicsTestsCount) {
    physicsAverage.value = physicsTestsTotal / physicsTestsCount;
  } else {
    physicsAverage.value = warning;
  }
  for (i = 0; i < historyTests.length; i++) {
    if (historyTests[i].value) {
      historyTestsTotal += Number(historyTests[i].value);
      historyTestsCount++;
    }
  }
  if (historyTestsCount) {
    historyAverage.value = historyTestsTotal / historyTestsCount;
  } else {
    historyAverage.value = warning;
  }
});

[type="number"] {
  width: 5em;
}

<form>
  <p id="physics">
    Physics:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="physicsAverage"></output>
  </p>
  <p id="history">
    History:
    <input type="number">
    <input type="number">
    <input type="number">
    <output id="historyAverage"></output>
  </p>
  <input type="button" value="Calculate" id="calculator">
</form>