add task solution#2287
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 some core task requirements are not yet met and they affect the correctness and completeness of the solution. You correctly gather all .population elements, strip commas, sum the values, and compute an average, but you never validate that the cleaned string is actually convertible to a number; if one value is invalid, parseInt will yield NaN and break the calculations, violating the requirement to “make sure that the given string can be converted to a number” (e.g., near the loop in src/scripts/main.js). You should check the parsed result with something like Number.isNaN(parsedValue) and skip or otherwise handle invalid entries instead of adding them to sum. In addition, both sum and avg are written directly to total.textContent and average.textContent without thousands separators or rounding, which conflicts with the requirement that the total and average be displayed in the same numeric style as the populations (with separators). Please format the total and average using a thousands-separator formatting method (for example, someNumber.toLocaleString(...)) and round the average to a reasonable number of decimal places or to an integer before displaying it. Once these validation and formatting issues are addressed, your overall approach will fully align with 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
| 'use strict'; | ||
|
|
||
| // write your code here | ||
| const populate = document.querySelectorAll('.population'); |
There was a problem hiding this comment.
You correctly collect all .population elements, but you never verify that their cleaned text can actually be converted to a number before using it, which conflicts with the requirement to "make sure that the given string can be converted to a number." Consider checking for NaN after parsing and handling it appropriately before adding to the sum.
| populate[i].textContent = populate[i].textContent.split(',').join(''); | ||
| sum += parseInt(populate[i].textContent); |
There was a problem hiding this comment.
Stripping commas for parsing is good, but you then use parseInt directly without checking the result. You should ensure the parsed value is a valid number (not NaN) before adding it to sum, as required by the description.
| sum += parseInt(populate[i].textContent); | ||
| } | ||
|
|
||
| const avg = sum / populate.length; |
There was a problem hiding this comment.
You compute the average correctly in terms of raw math, but you never format avg with thousands separators, nor do you round it. The task requires the average to use the same numeric style as the populations (with thousands separators).
|
|
||
| const avg = sum / populate.length; | ||
|
|
||
| total.textContent = sum; |
There was a problem hiding this comment.
sum is displayed without any thousands separators. The requirements specify that the calculated total must be separated by a thousands separator in the same style as the populations, so you need to format this number before assigning it to textContent.
| const avg = sum / populate.length; | ||
|
|
||
| total.textContent = sum; | ||
| average.textContent = avg; |
There was a problem hiding this comment.
Similar to total, the average text should be formatted with thousands separators (and likely rounded) to match the numeric style of the population values, instead of showing the raw unformatted number.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because the core functionality in main.js is correctly implemented and meets the task requirements. You properly select all .population spans, clean and convert their text to numbers, validate with Number.isNaN, and then compute both the total and average only from valid values, which addresses the main correctness concerns. The total and average are then rounded/formatted using toLocaleString, and you update .total-population and .average-population in the DOM as required, matching the expected thousands-separator style. You also respected the constraint to only modify main.js, leaving the HTML and CSS unchanged.
The only remaining issue is outside this file: you still need to update the DEMO link in the README to replace the <your_account> placeholder with your actual GitHub username [CHECKLIST ITEM #16]. Once you fix that in the README, your project will fully align with all checklist items. Nicely done getting the parsing, validation, and formatting details correct—those are easy places to introduce subtle bugs.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.