add task solution#2299
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the average population display is currently broken and will show NaN instead of a number, which violates the functional checklist requirement for correctly formatted output [CHECKLIST ITEM #7]. In src/scripts/main.js, the expression Math.round(average.toLocaleString()) is incorrect, since average.toLocaleString() returns a string and Math.round expects a number. To fix this, first round the numeric average (e.g., const roundedAverage = Math.round(average);) and then call roundedAverage.toLocaleString() when assigning to averageSpan.textContent. This ensures the average is a rounded integer and formatted with thousands separators, consistent with how total and individual populations are shown. Aside from this issue, your logic for reading .population spans, converting them to numbers, and computing total and average is solid and meets the core requirements. Once you correct the average formatting so it produces a valid number with separators, your solution will satisfy the task’s functional standards.
✨ 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
| const averageSpan = document.querySelector('.average-population'); | ||
| const totalSpan = document.querySelector('.total-population'); | ||
|
|
||
| averageSpan.textContent = Math.round(average.toLocaleString()); |
There was a problem hiding this comment.
Math.round(average.toLocaleString()) is incorrect: average.toLocaleString() returns a string, so Math.round will produce NaN. To meet checklist item #7 and correctly show the rounded average with thousands separators, round the numeric average first (e.g. const roundedAverage = Math.round(average);) and then call roundedAverage.toLocaleString() when assigning textContent.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully meets the functional requirements of the task. In src/scripts/main.js, you correctly read all .population spans, strip out the commas, and convert the text values into numbers before performing calculations. You then compute both the total and the average population from this numeric list, round the average using Math.round(average), and format both values with toLocaleString() so they display with thousands separators. The resulting values are written back into the .average-population and .total-population spans, replacing the initial Calculate it! text as required, and the earlier NaN issue is resolved by formatting only after rounding the numeric result. There are no critical or blocking issues in your implementation, and it adheres to the constraint of only modifying main.js. Nice work implementing the data parsing and display logic cleanly and reliably.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK