add solution - #2308
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical functional gap around number validation that can break your total and average calculations. Right now, after stripping commas from each .population span, you convert with Number(cleaned) and push the result directly into the numbers array; if any span contains non-numeric text, this will produce NaN and corrupt the reduce total and the average. This violates the requirement to "make sure that the given string can be converted to a number" before including it in the calculation [CHECKLIST ITEM #12]. To fix this, add a guard such as const value = Number(cleaned); if (!Number.isNaN(value)) { numbers.push(value); } at the point where you build the numbers array (refer to the line where you currently call Number(cleaned) in src/scripts/main.js). Once you ensure only valid numeric values are pushed, your existing total/average logic and toLocaleString() formatting will work reliably and fully meet the task requirements. Overall, your DOM selection and formatting are solid, so this should be a small but important change to make your solution robust.
✨ 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 number = Number(cleaned); | ||
|
|
||
| numbers.push(number); |
There was a problem hiding this comment.
According to checklist item #12, you should verify that cleaned can be converted to a number before pushing it into numbers; currently NaN values would also be included and could corrupt the total/average calculations.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly reads the .population span values, strips commas, converts them to numbers, and safely ignores any entries that cannot be parsed as numbers, preventing NaN from breaking the calculations. You compute both total and average from the validated values and update the .total-population and .average-population elements, replacing the original "Calculate it!" text as required. The use of toLocaleString() to format the total and average matches the expected thousands-separator style and keeps the display consistent with the original data. From the perspective of the core task and the critical validation requirement [CHECKLIST ITEM #12], your main.js now fulfills the functionality and behaves correctly. The remaining points (like ensuring the demo link uses your actual GitHub username) are non-code configuration details and do not block this task’s approval, but you should still double-check them in your repository. Nicely done addressing the earlier issue and implementing a clean, robust solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.