True, Thank you for all your help so far! Would I build the function before the rest of the code?
So instead, I will talk about some improvements that I would make to your existing code.
CSS Naming Conventions
Even though class names can include uppercase and lowercase, it’s a best-practice naming convention to use all lowercase with dashes between words if multiple words are used.
That means renaming Calculation throughout your code to calculation instead.
<!--<tr class="Calculation">-->
<tr class="calculation">
And update the scripting code:
var calculations = document.getElementsByClassName("calculation");
Allow Iteration Over Nodes
The getElementsByClassName method is an older one that just gives you a live HTMLCollection, which doesn’t let you easily iterate over the nodes. The preferred way to do that is with querySelectorAll instead which gives you a static nodeList, which lets you use any css selectors with it and supports iteration over the list.
// var calculations = document.getElementsByClassName("calculation");
var calculations = document.querySelectorAll(".calculation");
We can now use the forEach method with the calculations nodeList:
var calculations = document.querySelectorAll(".calculation");
// var numberOfCalculations = calculations.length;
// for (var counter = 0; counter < numberOfCalculations; counter++) {
calculations.forEach(function checkCalculation(calculation) {
// var calculation = calculations[counter]; //get the calculation
...
// }
});
Extract function
It’s easier to understand what’s going on in code when the body of the function isn’t distracting you from what’s happening there, so we can extract that checkCalculations function out of the forEach loop and place the function earlier up in the code instead.
function checkCalculation(calculation) {
...
}
var calculations = document.querySelectorAll(".calculation");
calculations.forEach(checkCalculation);
// calculations.forEach(function checkCalculation(calculation) {
// ...
// });
Remove unneeded comments
Sometimes a comment is useless because it tells you exactly what you can see from the code.
For example, this comment serves no benefit at all:
var result = firstNumber + secondNumber; //add the first 2 numbers together
All of the other comments have been removed because they uselessly repeat exactly what the code already says.
Better comments are ones that describes why something happens instead. For example this one I’ve added at the top of the code:
// Checks the table for incorrect results, and shows the correct result.
Reduce Duplication
The way that values are obtained from the HTML code has lots of duplication:
var firstNumber = Number(calculation.childNodes[1].textContent);
var secondNumber = Number(calculation.childNodes[3].textContent);
var expectedResult = Number(calculation.childNodes[5].textContent);
There’s a [rule-of-three](https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) principle where when something happens three times, refactor the code to remove that duplication.
In this case we can create a new function to give us a value from a node.
function getNumber(node) {
return Number(node.textContent);
}
function checkCalculation(calculation) {
var firstNumber = getNumber(calculation.childNodes[1]);
var secondNumber = getNumber(calculation.childNodes[3]);
var expectedResult = getNumber(calculation.childNodes[5]);
...
There is now more duplication that we can work at removing, but it has awkward numbers of 1, 3, and 5. That’s because the childNodes are being directly worked with, and the whitespace formatting of the HTML code needs to be skipped.
Instead of using childNodes, there are a couple of different ways of dealing with this. One is more specific to tables where we access the cells collection, which gives a live HTMLCollection of the TD elements. Another is to use querySelectorAll(“td”) to give us a static nodeList, and another yet is to use the children method which gives us the child elements. Here I’ll use the querySelectorAll method.
function checkCalculation(calculation) {
var cells = calculation.querySelectorAll("td");
// var firstNumber = getNumber(calculation.childNodes[1]);
var firstNumber = getNumber(cells[0]);
// var secondNumber = getNumber(calculation.childNodes[3]);
var secondNumber = getNumber(cells[1]);
// var expectedResult = getNumber(calculation.childNodes[5]);
var expectedResult = getNumber(cells[2]);
Use Map to Get Numbers
And now that we have array indexes from 0 onwards, we can change it into an array so that we can use the array map method to easily convert them into numbers:
var cells = calculation.querySelectorAll("td");
var numbers = Array.from(cells).map(getNumber);
// var firstNumber = getNumber(cells[0]);
var firstNumber = numbers[0];
// var secondNumber = getNumber(cells[1]);
var secondNumber = numbers[1];
// var expectedResult = getNumber(cells[2]);
var expectedResult = numbers[2];
Destructure Array
That has helped to expose more duplication, letting us use an ES6 array-matching feature letting us destructure the array into variables all at once.
// var firstNumber = numbers[0];
// var secondNumber = numbers[1];
// var expectedResult = numbers[2];
var [firstNumber, secondNumber, expectedResult] = numbers;
Expected vs Actual Naming Convention
Now that expectedResult variable is incorrect. It is instead the actual result that’s on the page. The expected result is what we calculate by adding the first and second number, so we should fix that now, before it leads to further confusion.
// var [firstNumber, secondNumber, expectedResult] = numbers;
var [firstNumber, secondNumber, actualResult] = numbers;
var expectedResult = firstNumber + secondNumber;
if (actualResult == expectedResult) {
} else {
...
resultCell.appendChild(document.createTextNode(expectedResult));
}
Improve If Statement Structure
There is a different programming language called Java has a convention to drop that opening brace to the next line. With JavaScript there is a code convention to place the opening brace at the end of an existing line. This convention helps to make it easier to tell the languages apart.
Also the if statement, we are checking if something is true, and taking action when it’s not true.
if (actualResult == expectedResult) {
} else {
...
}
}
As we are interested in when the comparison is not true, we can get rid of the else structure by checking if things are not equal instead.
// if (actualResult == expectedResult) {
// } else {
if (actualResult !== expectedResult) {
...
}
}
Extract If Condition
We can now extract that condition of the if statement out to a separate function:
function goodCalculation(actualResult, expectedResult) {
return actualResult === expectedResult;
}
if (!goodCalculation(actualResult, expectedResult)) {
being sure to use positive terminology for the function name.
Improve Body of If Statement
Currently the if statement has a variable being declared in the middle of it. Whenever that happens that’s a good signal that a function should be used instead.
if (!goodCalculation(actualResult, expectedResult)) {
calculation.style.backgroundColor = "#FA8072";
var resultCell = document.createElement("td");
calculation.appendChild(resultCell);
resultCell.appendChild(document.createTextNode(expectedResult));
}
When we move the body of this if statement into a function, what’s a good name for that function? The code does multiple things, it marks the bad row, and it shows the correct result.
We can put off the decision about what to call it, by first creating two different functions that do the above work.
function markBadRow(calculation) {
calculation.style.backgroundColor = "#FA8072";
}
...
if (!goodCalculation(actualResult, expectedResult)) {
// calculation.style.backgroundColor = "#FA8072";
markBadRow(calculation);
...
}
Use Class instead of Style
With the style.backgroundColor, usually it’s a bad idea to use JavaScript to change the styles of elements. It’s a best-practice instead to set or remove class names. So we can move that “#FA8072” out to a class declaration and use that instead:
.error {
background-color: #FA8072;
}
function markBadRow(calculation) {
// calculation.style.backgroundColor = "#FA8072";
calculation.classList.add("error");
}
Extract If Statement
The rest of the if statement goes in to a showCorrectResult function:
function showCorrectResult(calculation, expectedResult) {
var resultCell = document.createElement("td");
calculation.appendChild(resultCell);
resultCell.appendChild(document.createTextNode(expectedResult));
}
...
if (!goodCalculation(actualResult, expectedResult)) {
markBadRow(calculation);
// var resultCell = document.createElement("td");
// calculation.appendChild(resultCell);
// resultCell.appendChild(document.createTextNode(expectedResult));
showCorrectResult(calculation, expectedResult);
}
So what is a good function name to use for both marking a bad row, and showing the correct result?
if (!goodCalculation(actualResult, expectedResult)) {
markBadRow(calculation);
showCorrectResult(calculation, expectedResult);
}
It’s not suitable to use the word “and” in a function name, such as with markBadAndShowResult(). It can be hard to come up with a really good function name, so instead of that let’s come up with a better function name instead. I’m thinking of showCorrectCalculation() instead.
function showCorrectCalculation(calculation, expectedResult) {
markBadRow(calculation);
showCorrectResult(calculation, expectedResult);
}
...
if (!goodCalculation(actualResult, expectedResult)) {
showCorrectCalculation(calculation, expectedResult);
}
Filter and Process rows
That checkCalculation function is doing too much work. It’s getting the calculation values, figuring out what the result should be, comparing them to check if they’re correct, and updating to show the correct result. Each of those things should be done separately.
There is a good-practice way to process these table rows, and that is to use filter, map, and reduce. In this case, we want to filter the rows for the bad rows so that we can then do something with those bad ones.
To achieve that, we’ll want access to know the actual result both when checking the calculation, and when showing the correct calculation. The overall plan is to get an array of the invalid calculation rows along with their correct result, and loop over those making the updates.
So it’s time for a few more functions, where we get the expected result from a calculation row, and the actual result.
function getActualResult(calculation) {
var cells = calculation.querySelectorAll("td");
return getNumber(cells[2]);
}
function calcExpectedResult(calculation) {
var cells = calculation.querySelectorAll("td");
var numbers = Array.from(cells).map(getNumber);
var [firstNumber, secondNumber] = numbers;
var expectedResult = firstNumber + secondNumber;
return expectedResult;
}
...
function checkCalculation(calculation) {
// var cells = calculation.querySelectorAll("td");
// var numbers = Array.from(cells).map(getNumber);
// var [firstNumber, secondNumber, actualResult] = numbers;
// var expectedResult = firstNumber + secondNumber;
var actualResult = getActualResult(calculation);
var expectedResult = calcExpectedResult(calculation);
...
}
Filter an Array
I can make an initial approach by turning the calculations nodeList into an array and filtering them for the invalid rows:
var calculations = Array.from(document.querySelectorAll(".calculation"));
calculations.filter(function hasInvalidResult(calculation) {
var actualResult = getActualResult(calculation);
var expectedResult = calcExpectedResult(calculation);
return actualResult !== expectedResult;
}).forEach(checkCalculation);
// calculations.forEach(checkCalculation);
As everything still works, I can move the function out which leaves us with the main structure, of filter then forEach.
function hasInvalidResult(calculation) {
var actualResult = getActualResult(calculation);
var expectedResult = calcExpectedResult(calculation);
return actualResult !== expectedResult;
}
...
var calculations = Array.from(document.querySelectorAll(".calculation"));
// calculations.filter(function hasInvalidResult(calculation) {
// var actualResult = getActualResult(calculation);
// var expectedResult = calcExpectedResult(calculation);
// return actualResult !== expectedResult;
// }).forEach(checkCalculation);
calculations.filter(hasInvalidResult).forEach(checkCalculation);
And now that we have that hasInvalidResult function that uses the calculation argument, we can use that from within the checkCalculation function too, letting us get rid of the actualResult variable.
function checkCalculation(calculation) {
// var actualResult = getActualResult(calculation);
var expectedResult = calcExpectedResult(calculation);
// if (!goodCalculation(expectedResult, actualResult)) {
if (hasInvalidResult(calculation)) {
showCorrectCalculation(calculation, expectedResult);
}
}
Improve Function Name
Now with the filter/forEach code, that checkCalculation function name is inappropriate because it’s already being checked in the hasGoodResult function, so we can rename checkCalculation to showCorrectCalculation instead.
function checkCalculation(calculation) {
// var expectedResult = calcExpectedResult(calculation);
if (hasInvalidResult(calculation)) {
// showCorrectCalculation(calculation, expectedResult);
showCorrectCalculation(calculation);
}
}
But, that means not passing expectedResult into showCorrectCalculation. We can move that in to the showCorrectCalculation function itself.
// function showCorrectCalculation(calculation, expectedResult) {
function showCorrectCalculation(calculation) {
var expectedResult = calcExpectedResult(calculation);
markBadRow(calculation);
showCorrectResult(calculation, expectedResult);
}
...
function checkCalculation(calculation) {
// var expectedResult = calcExpectedResult(calculation);
if (hasInvalidResult(calculation)) {
// showCorrectCalculation(calculation, expectedResult);
showCorrectCalculation(calculation);
}
}
Map over Filtered Array
And now we can use that showCorrectCalculation function directly with the filter.
var calculations = Array.from(document.querySelectorAll(".calculation"));
// calculations.filter(hasInvalidResult).forEach(checkCalculation);
calculations.filter(hasInvalidResult).map(showCorrectCalculation);
That code can now be moved in to the checkCalculation function instead:
// function checkCalculation(calculation) {
function checkCalculation() {
// if (hasInvalidResult(calculation)) {
// showCorrectCalculation(calculation);
// }
var calculations = Array.from(document.querySelectorAll(".calculation"));
calculations.filter(hasInvalidResult).map(showCorrectCalculation);
}
checkCalculation();
We can then move some of those stages into separate variables:
function checkCalculation() {
var calculations = document.querySelectorAll(".calculation");
var invalidRows = Array.from(calculations).filter(hasInvalidResult);
invalidRows.map(showCorrectCalculation);
}
Isn’t that a beautiful function now? It does three simple things. It gets the calculation rows, it filters them for the bad calculations, and it shows the correct calculations for those instead.
Our work here is done.
3 Likes
Thank you this has been a lot of help.
This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.