Get the number of edited inputs


#21

Of course, there’s more duplication that’s now revealed in the code:

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);
});

We can put “physics” and “history” in an array, and loop through those instead.

document.getElementById('calculator').addEventListener('click', function() {
  var courses = ['physics', 'history'];
  courses.forEach(function courseAverage(course) {
    var tests = document.getElementById(course).getElementsByTagName('input');
    var average = document.getElementById(course + 'Average');
    average.value = calculateAverage(tests);
  });
});

That helps to remove the duplication, making it easier to deal with other types of courses when they grow from just 2 currently, to several more.

The updated code can be seen at https://jsfiddle.net/hx61jcv8/5/


#22

And, I think that the details of the function don’t need to be in the event listener function. It’s best to keep the event listener as simple as possible, which helps to reduce the amount of mental overhead.

function courseAverage(course) {
  var tests = document.getElementById(course).getElementsByTagName('input');
  var average = document.getElementById(course + 'Average');
  average.value = calculateAverage(tests);
}
  
document.getElementById('calculator').addEventListener('click', function() {
  var courses = ['physics', 'history'];
  courses.forEach(courseAverage);
});

It’s even possible to do it as one line:
['physics', 'history'].forEach(courseAverage);

But that might be condensing things too far.

The updated code (without the one-liner) is at https://jsfiddle.net/hLp54mt8/


#23

I’m still reading your notes and enjoying your wealth of knowledge. Thanks again! :rose:


#24

Just to add something small to your comprehensive list, the following also seems interesting to me:

var count = !!tests[0].value + !!tests[1].value + !!tests[2].value;

#25

In your code, I noticed you didn’t use else:

  if (!count) {
    return 'No assessment made!';
  }
  return total / count;

Generally speaking, can we avoid using it in if/else statements?


#26

When the same thing is being affected by an if/else statement, that can be the case, yes.

In the above code the if statement is acting as a Guard clauses, protecting the formula from giving meaningless results when he count is zero. When things like guard clause and default parameters are used, that helps to simplify the code making it easier for people to understand.


#28

Dear Paul,
I just loved your calculateAverage function and used it even in two other functions. I also added some new features such as a reset button. I wonder if you’d mind reviewing my last update. I really appreciate it!

<!DOCTYPE HTML>
<html lang="en">
<head>

<title>Average Grade</title>

</head>
<body>

<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>

<script>
  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 (isNaN(averagesTotal)) {
      finalGrade.value = '';
    } else {
      finalGrade.value = 'Final grade:' + (averagesTotal / 5).toFixed(1);
    }
  });
  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.';
    }
  });
</script>

</body>
</html>

DEMO


#29

Remove the i variable

The i variable doesn’t belong outside of the calculateAverage function and should be moved into the calculateAverage function. Even better though, the i variable can be removed completely by using a forEach method instead.

An effective way to achieve that transition, is to first assign the indexed item to a separate variable.

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

The physicsTests and historyTests should use querySelectorAll instead, as that gives an iterable nodeList.

    // var physicsTests = physics.getElementsByTagName("input");
    var physicsTests = physics.querySelectorAll("input");
    ...
    // var historyTests = history.getElementsByTagName("input");
    var historyTests = history.querySelectorAll("input");

We can now much more easily transition that loop over to a forEach method instead.

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

Benefits achieved by doing that is that the i variable is no longer needed, the code is easier to understand when there aren’t lots of array references all over the place, and it’s a good step towards functional programming. Further steps would involve using filter/map/reduce as those help to simplify things by breaking down the code into smaller component pieces, but there’s no need to go there yet.

Simplify the event handlers

Event handlers really should have only the most simple of initial setup before calling a separate function to do the work.

Looking at the existing code, the calculator event handler does two main things, that can be best represented as the following:

function calculatorClickHandler() {
    var scores = averageScores(["physics", "history"]);
    var grade = finalGrade(scores);
    show(scores, grade);
}

The above event handler doesn’t currently work with your existing code, but it’s a vision of a possible future.
Is that a direction for the code that you can get behind?


#30

After making several updates, the course and its weight are all given at the same time.

const courseInfo = {
    "physics": 3,
    "history": 2
};
gradeCourses(courseInfo);

The gradeCourses function is structured with the main functions first, followed by event handlers, and code to assign those event handlers.

In the gradeCourses function, the calculate event handler is where the averages are obtained and graded.

    function calculatorClickHandler() {
        var averages = calculateAverageScores(courseInfo);
        var grade = calculateFinalGrade(averages);
        show(averages, grade);
    }

Here’s the full updated script:

function gradeCourses(courseInfo) {
    // tests: nodeList of input fields
    function calculateAverage(tests) {
        var total = 0;
        var count = 0;
        tests.forEach(function addScores(test) {
            if (test.value) {
                total += Number(test.value);
                count += 1;
            }
        });
        if (count > 0) {
            return total / count;
        }
    }
    // courseInfo: [{course: str, weight: num}, ...]
    function calculateAverageScores(courseInfo) {
        var averages = {};
        Object.entries(courseInfo).forEach(function ([course, weight]) {
            var section = document.querySelector("#" + course);
            var scores = section.querySelectorAll("input");
            averages[course] = {
                "score": calculateAverage(scores),
                "weight": weight
            };
        });
    return averages;
    }
    // averages: [{..., average: {score: num, weight: num}}, ...]
    function calculateFinalGrade(averages) {
        var eachScore = Object.entries(averages);
        var totalWeight = 0;
        var grade = eachScore.reduce(function grade(total, [ignore, average]) {
            totalWeight += average.weight;
            return total + average.score * average.weight;
        }, 0);
        return grade / totalWeight;
    }
    // averages: [{course: str, average: {score: num, weight: num}}, ...]
    // grade: num
    function show(averages, grade) {
        Object.entries(averages).forEach(function showScore([course, average]) {
            var scoreEl = document.querySelector("#" + course + "Average");
            scoreEl.value = "Please enter a grade.";
            if (average.score !== undefined) {
                scoreEl.value = "Average: " + Number(average.score).toFixed(1);
            }
        });

        var gradeEl = document.querySelector("#finalGrade");
        gradeEl.value = "";
        if (grade > 0) {
            gradeEl.value = "Final grade:" + Number(grade).toFixed(1);
        }
    }
    function reset(form) {
        function confirmReset() {
            return window.confirm(
                "Your changes will be lost.\nAre you sure you want to reset?"
            );
        }
        if (form.hasChanged && confirmReset()) {
            form.reset();
        }
    }

    // Event handlers
    function calculatorClickHandler() {
        var averages = calculateAverageScores(courseInfo);
        var grade = calculateFinalGrade(averages);
        show(averages, grade);
    }
    function resetterClickHandler(evt) {
        var button = evt.target;
        var form = button.form;
        reset(form);
    }
    function formHasChangedHandler(evt) {
        var field = evt.target;
        var form = field.form;
        form.hasChanged = true;
    }
    function beforeUnloadHandler(evt) {
        var form = document.querySelector("#form");
        if (form.hasChanged) {
            evt.returnValue = "Your changes may be lost.";
        }
    }

    // Assign event handlers
    var calculator = document.querySelector("#calculator");
    calculator.addEventListener("click", calculatorClickHandler);

    var form = document.querySelector("#form");
    form.addEventListener("input", formHasChangedHandler);

    var resetter = document.querySelector("#resetter");
    resetter.addEventListener("click", resetterClickHandler);

    window.addEventListener("beforeunload", beforeUnloadHandler);
}

const courseInfo = {
    "physics": 3,
    "history": 2
};
gradeCourses(courseInfo);

A working example of the code is at https://jsfiddle.net/2bLxqs0h/
The above code has been updated in response to the below posts, with the example code being at https://jsfiddle.net/tb5rm9dn/1/


#31

Great, but it doesn’t work for the grade 0.


#32

That’s an interesting requirement that should be easy to resolve.


#33

Two more things: the reset and beforeunload should work only when there’s an edited input.


#34

Currently when there are empty fields or a zero score, the averages object just has 0.

{
  physics: {score: 0, weight: 3},
  history: {score: 0, weight: 2}
}

Instead of that, when all history fields, for example, are empty I want the score to be undefined instead.

That is achieved by updating the end of the calculateAverage function:

    function calculateAverage(tests) {
        ...
        // return total / count || 0;
        if (count > 0) {
            return total / count;
        }
    }

When physics has just a 0 entry and the history ones are empty, the averages object is now:

{
  physics: {score: 0, weight: 3},
  history: {score: undefined, weight: 2}
}

Which gives us enough information to properly update the show function.

        function show(averages, grade) {
            ...
            scoreEl.value = "Please enter a grade.";
            // if (average.score) {
            if (average.score !== undefined) {
                scoreEl.value = "Average: " + Number(average.score).toFixed(1);
            }
        }

The updated code is at https://jsfiddle.net/tb5rm9dn/1/

Thanks to the structured code and its objects, it’s been very easy to understand what needs to be modified to fit the requirements.


#35

What behaviour causes that to work improperly?


#36

Never mind — it works like a charm! :ok_hand: