Calculate Final Grade: follow-up

Question

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

var inputs = document.getElementsByTagName('input');
var i;

function calculateAverage(tests) {
  var total = 0;
  var count = 0;
  for (i = 0; i < tests.length; i++) {
    if (tests[i].value) {
      total += Number(tests[i].value);
      count++;
    }
  }
  if (!count) {
    return 'Please enter a grade.';
  }
  return 'Average: ' + (total / count).toFixed(1);
}
document.getElementById('calculator').addEventListener('click', function() {
  var physicsTests = document.getElementById('physics').getElementsByTagName('input');
  var physicsAverage = document.getElementById('physicsAverage');
  physicsAverage.value = calculateAverage(physicsTests);

  var historyTests = document.getElementById('history').getElementsByTagName('input');
  var historyAverage = document.getElementById('historyAverage');
  historyAverage.value = calculateAverage(historyTests);

  var finalGrade = document.getElementById('finalGrade');
  var averagesTotal = physicsAverage.value.slice(9) * 3 + historyAverage.value.slice(9) * 2;
  if (averagesTotal || averagesTotal == 0) {
    finalGrade.value = 'Final grade:' + (averagesTotal / 5).toFixed(1);
  } else {
    finalGrade.value = '';
  }
});
document.getElementById('resetter').addEventListener('click', function() {
  var form = document.getElementById('form');
  var edited = calculateAverage(inputs);
  if (edited != 'Please enter a grade.' && confirm('Your changes will be lost.\nAre you sure you want to reset?')) {
    form.reset();
  }
});
window.addEventListener('beforeunload', function(event) {
  var edited = calculateAverage(inputs);
  if (edited != 'Please enter a grade.') {
    event.returnValue = 'Your changes may be lost.';
  }
});

<form id="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>
  <button type="button" id="resetter">Reset</button>
  <output id="finalGrade"></output>
</form>

P.S. I've made changes to the previous code to shorten the script and added some new features such as a reset button.

Note

Please review the final product made thanks to the hints I've received in this thread.

Answer 1

I went ahead and refactored your code a lot. However, before I get into that, lets talk about what you did and what you could have done differently.

Feel free to let me know if you want me to expand on something specific.

Logic regarding: function calculateAverage(tests) {}

Calculate average function should not take in parameters a list of HTMLInputElements and should not return a string.

Why? Well it doesn't make sense. calculateAverage should take a list of values and calculate an average.

function calculateAverage(values){
  return values.reduce((a,c)=>a+c,0)/values.length;
}
const avg = calculateAverage([1,2,3]);

console.log(avg);

Because you return a string... later on you have to do this:

physicsAverage.value.slice(9)

Not good ! And can become extremely buggy !

Use querySelectorAll

querySelectorAll is your friend. It allows you to transform this:

document.getElementById('physics').getElementsByTagName('input');

into this:

document.querySelectorAll('#physics > input');

Declare static variables globally:

In your calculator listener callback, you declare a list of variables.

var physicsTests = document.getElementById('physics').getElementsByTagName('input');
var physicsAverage = document.getElementById('physicsAverage');
//etc

But these variables never change. So don't declare them in your callback, move them outside and at the top of your file.

Create generic code !

With the current code you have, a lot of manual labor is needed if you wish to add another subject. In my final solution, everything is built in a way where adding a new subject requires NO changes in the javascript.

To able to do this, use data-* HTML properties. It's super powerful when you need to add some extra info like the coefficients of each subject.

const subjects = document.querySelectorAll("div[data-coef]");

subjects.forEach(subject=>{
  console.log(subject.id, subject.dataset.coef);
});

<div id="physics" data-coef="3"></div>
<div id="history" data-coef="2"></div>

Another powerful tool is Map. It allows you to easily keep track of specific subjects and other variables.

Solution:

const inputContainers = document.querySelectorAll('form#form > .subject');

const finalGradeElement = document.getElementById('finalGrade');  
const form = document.getElementById('form');

const m = new Map();

inputContainers.forEach((container,i)=>{
  const output = container.querySelector(".output");
  const inputs = Array.from(container.getElementsByTagName("input"));
  const coef = Number(container.dataset.coef);
  
  const data = Array(inputs.length);
  m.set(i, {
    output, data, coef
  }); 
  
  inputs.forEach((item,j)=>item.addEventListener("input", function(){
    if(!this.value.length > 0){
       data[j] = undefined;
    } else {
       data[j] = Number(this.value);
    }
  }));
});

document.getElementById('calculator')
.addEventListener('click', function() {

  const res = Array.from(m.values()).reduce( (a,{output, data, coef})=>{
  
      const values = data.filter(item=>item!==undefined);
      
      if(values.length > 0){
        const avg = values.reduce((a,c)=>a+c, 0)/values.length;
        a.avgTotal  += avg * coef;
        a.coefTotal += coef;
        
        output.value = `Average: ${avg.toFixed(1)}`;
      } else {
        output.value = "Please enter a number";
      }
      
      return a;
  }, {avgTotal: 0, coefTotal: 0})

  const averagesTotal = res.avgTotal/res.coefTotal;
  
  finalGradeElement.value = `Final Grade: ${averagesTotal.toFixed(1)}`
  
});

function isEdited(){
  return !Array.from(m.values()).every(({data})=>{
    return data.every(i=>i===undefined);
  });
}

document.getElementById('resetter')
.addEventListener('click', function() {
  if (isEdited() && confirm('Your changes will be lost.\nAre you sure you want to reset?')) {
    form.reset();
    m.clear();
  }
});

window.addEventListener('beforeunload', function(event) {
  if (isEdited()) {
    event.returnValue = 'Your changes may be lost.';
  }
});

form > div {
  display: flex;
  flex-direction: column;
}

<form id="form">
  <div class="subject" data-coef="3">
    Physics:
    <input type="number">
    <input type="number">
    <input type="number">
    <output class="output"></output>
  </div>
  <div class="subject" data-coef="2">
    History:
    <input type="number">
    <input type="number">
    <input type="number">
    <output class="output"></output>
  </div>
  <button type="button" id="calculator">Calculate</button>
  <button type="button" id="resetter">Reset</button>
  <output id="finalGrade"></output>
</form>

Answer 2

Separate logical elements

The calculateAverage does multiple things:

• Calculate average
• Return the computed value as a formatted string
• Return a special string in case of missing user input

It would be better to separate these elements.

Because these are not separated well, the rest of the program has to work extra hard, for example:

var averagesTotal = physicsAverage.value.slice(9) * 3 + historyAverage.value.slice(9) * 2;

Readers may be puzzled, and ask questions like:

• Why do we need that .slice(9) there?
• And where does that magic number 9 come from anyway?

Of course it's to chop off the 'Average: ' prefix from the value. It would be much better if the rest of the program could work with numeric values directly, without having to do string manipulations.

And the way to do that is by using more functions, cleanly separating computation and printing.

Avoid magic numbers

On the same line in the earlier example, the physics average is multiplied by 3, the history average is multiplied by 2. Why? Where do these values come from? Are they correct like this? The purpose of these values, and the intention of the program could become clear by giving them good, descriptive names.

Answer 3

A couple of suggestions for a little more generality; the input event could be used on the form itself, as it propagates the events down to the inputs. From there one can use the HTMLFormElement API to grab the sibling inputs and output.

Also when using the input event, it allows for the <output> elements to update on every change, and to show for example input errors in real-time.

For semantic markup, I would suggest using <fieldset> and <legend>, although they still suffer from getting special treatment from some browser vendors which can make styling difficult. I would also recommend using <input type="number">'s min, max and step attributes.

document.addEventListener('DOMContentLoaded', () => {
  const outputs = document.querySelectorAll('output')
  const using = (thing, fun) => fun(thing)

  const average = ary =>
    using(ary.map(x => Number(x.value)).filter(Boolean),
      xs => (xs.reduce((x, y) => x + y, 0) / xs.length).toFixed(1))
  const lastAsAverage = coll =>
    using([...coll], xs => xs.pop().value = average(xs))

  document.forms[0].addEventListener('input',
    ({target: {parentElement: {elements: inputs}}}) =>
      [inputs, outputs].forEach(lastAsAverage))
})

input:invalid { background-color: #faa }

<form>
  <fieldset>
    <legend>Physics:</legend>
    <input type="number">
    <input type="number">
    <input type="number">
    <output></output>
  </fieldset>

  <fieldset>
    <legend>History:</legend>
    <input type="number">
    <input type="number">
    <input type="number">
    <output></output>
  </fieldset>

  <output></output>

  <input type="reset">
</form>

Answer 4

Main updates

• Added a new function to detect changes to the input fields. It's used when resetting the form or reloading the page.
• Used two separate functions to calculate and display averages.
• Removed the slice method.

Credit: Special thanks to janos for the above pointers!

Final code

var i;

function detectChanges() {
  var inputs = document.querySelectorAll('input');
  for (i = 0; i < inputs.length; i++) {
    if (inputs[i].value) {
      return true;
    }
  }
}

function calculateAverage(tests) {
  var total = 0;
  var count = 0;
  for (i = 0; i < tests.length; i++) {
    if (tests[i].value) {
      total += Number(tests[i].value);
      count++;
    }
  }
  return total / count;
}

function displayAverage(tests) {
  var avg = calculateAverage(tests);
  if (isNaN(avg)) {
    return 'Please enter a grade.';
  } else {
    return 'Average: ' + avg.toFixed(1);
  }
}

document.getElementById('calculator').addEventListener('click', function() {
  var physicsTests = document.querySelectorAll('#physics > input');
  var physicsAverage = document.getElementById('physicsAverage');
  physicsAverage.value = displayAverage(physicsTests);

  var historyTests = document.querySelectorAll('#history > input');
  var historyAverage = document.getElementById('historyAverage');
  historyAverage.value = displayAverage(historyTests);

  var finalGrade = document.getElementById('finalGrade');
  var averagesTotal = (calculateAverage(physicsTests) * 3 + calculateAverage(historyTests) * 2) / 5;
  // course average * its weight; weights total = 5
  if (isNaN(averagesTotal)) {
    finalGrade.value = '';
  } else {
    finalGrade.value = 'Final grade: ' + averagesTotal.toFixed(1);
  }
});

document.getElementById('resetter').addEventListener('click', function() {
  var form = document.getElementById('form');
  if (detectChanges() && confirm('Your changes will be lost.\nAre you sure you want to reset?')) {
    form.reset();
  }
});

window.addEventListener('beforeunload', function(event) {
  if (detectChanges()) {
    event.returnValue = 'Your changes may be lost.';
  }
});

input {
  width: 5em;
}

<form id="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>
  <button type="button" id="resetter">Reset</button>
  <output id="finalGrade"></output>
</form>