3
\$\begingroup\$

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>

\$\endgroup\$
2
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 3 → 2. \$\endgroup\$ Commented Mar 7, 2019 at 6:46
  • \$\begingroup\$ Here's the follow-up question. \$\endgroup\$ Commented Mar 9, 2019 at 18:05

4 Answers 4

Reset to default
2
\$\begingroup\$

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>

\$\endgroup\$
11
  • \$\begingroup\$ "the wheel only has effect when the input is focused" - it varies and can be much worse. In Firefox, the wheel adjusts only when the input is focused AND the pointer is over the input. PLUS, if pointer landed on the focused input as a result of a scroll, it changes the value AND scrolls the page! Insane, but there you have it. So you enter a value with pointer somewhere else, operate wheel to scroll page and wheel appears to work normally...until you scroll past the (still-focused) field and silently change someone's grades. The concept of a number input is fine; the reality is not. \$\endgroup\$ Commented Mar 6, 2019 at 23:45
  • \$\begingroup\$ @Blindman67: "You have the average calculations inside the for loop. That means you calculate the average 3 time, 2 times too many." Good point! Just updated my code in this demo. "this answer assumes you are targeting ES6+ and HTML5 compliant browsers" Would you mind converting it to an older version? \$\endgroup\$ Commented Mar 7, 2019 at 7:09
  • \$\begingroup\$ See also the new post. \$\endgroup\$ Commented Mar 7, 2019 at 12:29
  • \$\begingroup\$ @Mort Sorry i don't do legacy. if you need to support abandoned browsers there are various turn key solutions when you are ready to go wild. Do not fixate on legacy as it prevents you developing the skills you will need for tomorrow... IMHO \$\endgroup\$ Commented Mar 7, 2019 at 13:43
  • 1
    \$\begingroup\$ @Mori As pointed out in the answer I round the result to prevent results like 5,2,3 giving 3.3333333333333335 (yes JS gets it wrong). You can set the number of decimal places you want on the last line (sum / count).toFixed(2) the toFixed takes that number of decimal places as argument and rounds to the nearest returning a string representation of the number \$\endgroup\$ Commented Mar 24, 2019 at 12:55
3
\$\begingroup\$

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>

\$\endgroup\$
3
  • \$\begingroup\$ "By testing the truth of element.value directly, you'll discard any zero scores." As you see in my demo, you can also enter 0. \$\endgroup\$ Commented Mar 7, 2019 at 6:01
  • \$\begingroup\$ @Mori yes, that actually works. The answer is incorrect but mainly because the value is a string at this point. The string "0" (zero) is not considered falsey, the number zero is. So if you decide to refactor the code and change that test to (effectively) be if (Number(physicsTests[i].value) you will have a bug and it might not be immediately obvious why. You might end up getting this over two or three refactors that are each logical and concise, so the final one introduces a bug without meaning to. \$\endgroup\$ Commented Mar 7, 2019 at 9:27
  • \$\begingroup\$ @Mori you're right; the bug I named is not there. The principle I'm referring to - "be explicit about string/number conversions" - is a good habit but the code works here even if you don't follow it. I edited my answer. \$\endgroup\$ Commented Mar 7, 2019 at 9:39
1
\$\begingroup\$

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>

\$\endgroup\$
2
  • \$\begingroup\$ "// Returns NaN if invalid input" NOT NECESSARILY! Sorry for the caps but I do have to stress that parseFloat("5Banana") will result in 5 even when I'd consider that invalid input. Using Number (or a unary + - they are equivalent) to do an explicit number conversion will correctly give you NaN for inputs that cannot at all be converted to a JS numeric. Although "5,50" (a valid decimal separator for a different culture) would return NaN. While a parseFloat will give you 5, so it's still off. \$\endgroup\$ Commented Mar 7, 2019 at 9:51
  • \$\begingroup\$ "// The .length property already has the value" but now the code behaves differently - inputting 3 in the first field and leaving the other two empty would have calculated the average for a single value (so, the same value) while with this implementation you HAVE to fill in all fields. \$\endgroup\$ Commented Mar 7, 2019 at 9:54
0
\$\begingroup\$

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>

\$\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.