Repository navigation
Solution for population sum and average - #2396
insomniaorno wants to merge 4 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The total and average calculations are implemented and formatted, but changes are needed to meet the requirement to validate each population value before using it. In src/scripts/main.js, an unconvertible population string can become NaN and invalidate both results; check that each parsed value is numeric before adding it to the total. The rest of the reviewed calculation and display behavior appears to meet the requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| let sum = 0; | ||
|
|
||
| for (const element of populations) { | ||
| sum += +element.textContent.replaceAll(',', ''); |
There was a problem hiding this comment.
This converts each population string without first checking that it is numeric, as required. Validate the parsed value before adding it to sum so an unconvertible value cannot make the total and average NaN.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The total correctly excludes non-numeric population values, but the average still divides by the full number of population elements in src/scripts/main.js. When any value is invalid, this produces an incorrect average and does not meet the requirement to calculate results from the parsed numeric values. Track how many values are successfully parsed and use that count as the average’s divisor, handling the case where no values are valid. This is a focused correction to an otherwise solid implementation.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| total.textContent = sum.toLocaleString('en-US'); | ||
|
|
||
| average.textContent = Math.round(sum / populations.length).toLocaleString( |
There was a problem hiding this comment.
Invalid population values are skipped from sum, but populations.length still includes them in the average’s divisor. Track the number of valid values and divide by that count so the average is based on the same parsed numbers as the total.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution correctly excludes invalid population values from the total and uses the valid-value count for the average. One edge case remains in src/scripts/main.js: when no values are valid, dividing by zero produces NaN, which is not a meaningful result for the user. Consider checking whether the valid-value count is zero before calculating the average and displaying an appropriate fallback in that case. I am approving this as an educational override to help you progress while learning; handling this edge case will make the result more robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| total.textContent = sum.toLocaleString('en-US'); | ||
|
|
||
| average.textContent = Math.round(sum / validPopulation).toLocaleString('en-US'); |
There was a problem hiding this comment.
The parsed-value count now fixes the average when some values are invalid. If no values are valid, though, this divides by zero and displays NaN; handle that case so the result remains meaningful.
DEMO LINK