add population calculation logic#2243
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 current implementation can produce NaN for the total and average populations when any .population span contains non-numeric text, which breaks a core requirement to ensure strings can be converted to numbers before using them in calculations. In src/scripts/main.js, around the section where you map span text to Number (roughly lines 15–25), you should filter or otherwise validate the parsed values (for example, using Number.isFinite(parsedValue)) so that only valid numbers are kept in the numbers array. Then, in the part where you compute the total and average (roughly lines 25–40), make sure you reduce over this validated array and divide by validNumbers.length, also guarding against division by zero if all entries are invalid. These issues are critical because a single bad value in the DOM will cause the entire total and average outputs to become NaN, violating the task requirement that parsed data be safely converted and used. As a minor improvement, you could also check that document.querySelector('.total-population') and .average-population actually return elements before setting textContent, and consider whether you want to round the average or display a fixed number of decimal places. Overall, your selection of .population spans, stripping commas, and formatting with toLocaleString('en-US') is solid; once you tighten up the validation and averaging logic, this will fully meet the task 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
| const numbers = Array.from(populations) | ||
| .map((span) => span.textContent.replace(/,/g, '').trim()) | ||
| .map(Number); |
There was a problem hiding this comment.
You convert span text to Number here but do not validate the result. The task requires ensuring each string can be converted to a number; if conversion fails you'll get NaN. Consider filtering out non-numeric entries (e.g., check Number.isFinite) or handle invalid values explicitly before further processing.
| .map((span) => span.textContent.replace(/,/g, '').trim()) | ||
| .map(Number); | ||
|
|
||
| const total = numbers.reduce((sum, num) => sum + num, 0); |
There was a problem hiding this comment.
Reducing an array that may contain NaN values will produce NaN for the total. Make sure to filter the numbers array to include only valid numeric values (or handle NaN in the reducer) before calling reduce.
| .map(Number); | ||
|
|
||
| const total = numbers.reduce((sum, num) => sum + num, 0); | ||
| const average = numbers.length ? total / numbers.length : 0; |
There was a problem hiding this comment.
Average is computed using numbers.length, which reflects the original list length and may include non-numeric entries. Use the length of the filtered/validated numeric array when dividing, and guard against division by zero.
| const average = numbers.length ? total / numbers.length : 0; | ||
|
|
||
| document.querySelector('.total-population').textContent = | ||
| total.toLocaleString('en-US'); |
There was a problem hiding this comment.
You format numbers with toLocaleString('en-US'), which yields comma thousands separators. This is fine if the populations are presented with that style; ensure the chosen locale matches the numeric style shown in the HTML (requirement: same numeric style as populations presented).
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 correctly collects all .population spans, sanitizes the text (removing commas and trimming), filters out non-numeric values, and then safely computes and displays both the total and average population without risking NaN or division-by-zero issues. The results are written back into the .total-population and .average-population elements and formatted with thousands separators, which aligns well with the task’s functional requirements. As a minor improvement, you could extract the 'en-US' locale used in toLocaleString('en-US') into a constant near the top of main.js so it’s easier to change in one place later. Another optional UX tweak is in the section where you compute and display the average: instead of Math.round(average), you might consider formatting with one or two decimal places using average.toLocaleString('en-US', { maximumFractionDigits: 1 }) if the design calls for more precision. Overall, your implementation is solid and fulfills the core requirements; the remaining suggestions are small refinements to make the code more flexible and user-friendly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.