5
\$\begingroup\$

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.

\$\endgroup\$

4 Answers 4

Reset to default
5
\$\begingroup\$

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>

\$\endgroup\$
4
  • \$\begingroup\$ "querySelectorAll is your friend." Good point! \$\endgroup\$ Commented Mar 12, 2019 at 9:04
  • 1
    \$\begingroup\$ When recommending ecmascript-6 features like arrow functions, it would be wise to mention this to the OP, since, while it wasn’t listed in the description there may be browser restrictions for the OPs code \$\endgroup\$ Commented Mar 18, 2019 at 23:00
  • \$\begingroup\$ All major browsers (except e11 and under) cover 99% of es6 features. es6 is JavaScript at this point. Arrow functions are covered by all major browsers except e11 and under. There won't be browser restrictions. \$\endgroup\$ Commented Mar 19, 2019 at 6:39
  • \$\begingroup\$ Using custom data attributes is a brilliant idea! 👍 \$\endgroup\$ Commented Apr 8, 2019 at 4:14
4
+250
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ Thanks for the review! I just added an update according to your hints. Is it better now? \$\endgroup\$ Commented Mar 12, 2019 at 15:30
  • \$\begingroup\$ @Mori if you would like a review of your updated implementation, it's good to ask it as a new question. See the conversations here for example \$\endgroup\$ Commented Mar 12, 2019 at 15:57
  • \$\begingroup\$ Good point, but I've already offered a bounty for this question and don't want to take the reviewers to a new one. \$\endgroup\$ Commented Mar 12, 2019 at 16:06
  • \$\begingroup\$ @Mori Why did you award the bounty to this answer? I was certain you would award it to other answers with more concrete detail. If this happened by mistake, the moderators might be able to fix it. \$\endgroup\$ Commented Mar 19, 2019 at 6:27
  • \$\begingroup\$ @Mori I reviewed your revised code now (sorry I was busy / forgetful). It looks much better. The only missing is the weights per grade are still magic numbers 3 and 2. They are called "magic" because they have no name, and so no clue to the reader what they represent, and why they are those particular values. To make them less magical, give them descriptive names. For example physicsWeight and historyWeight. \$\endgroup\$ Commented Mar 19, 2019 at 6:29
2
\$\begingroup\$

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>

\$\endgroup\$
1
  • \$\begingroup\$ "I would suggest using <fieldset> and <legend>" 👍 \$\endgroup\$ Commented Mar 13, 2019 at 8:14
1
\$\begingroup\$

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>

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.