add task solution#2284
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical gap in numeric validation that can break the required functionality. In src/scripts/main.js, the line where you convert span text to a number (e.g., Number(spanEl.textContent.split(',').join(''))) does not check for NaN, so any unexpected non-numeric content in a .population span will turn your totalPopulation into NaN and propagate incorrect totals and averages, violating the requirement to “make sure that the given string can be converted to a number” [CHECKLIST ITEM #12]. You should guard this conversion by checking Number.isNaN(parsedValue) and either skipping that span or handling it explicitly before adding to totalPopulation and including it in the count; this keeps your totals valid even with bad data.
Another blocking issue is that the task explicitly says only main.js should be modified, but SCSS files like src/styles/_fonts.scss and src/styles/main.scss have changes, which conflicts with [CHECKLIST ITEMS #10 and #11]; those style changes need to be reverted or removed from this task. Once you add the NaN check around your number parsing and ensure only main.js is changed, your solution will satisfy the functional requirements and constraints. The overall structure and logic in main.js—collecting .population spans, summing, averaging, and formatting with Intl.NumberFormat('en-US')—is good, so you are very close; focus on these two fixes to complete the task correctly.
✨ 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 totalPopulation = 0; | ||
|
|
||
| for (const spanEl of allPopulationSpans) { | ||
| const currentNumber = Number(spanEl.textContent.split(',').join('')); |
There was a problem hiding this comment.
Number(spanEl.textContent.split(',').join('')) assumes the converted value is always a valid number; per checklist item #12 you should verify the result is not NaN (e.g., with Number.isNaN) before adding it to totalPopulation so that unexpected non-numeric content in .population spans doesn’t propagate NaN into your totals.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical logic bug that prevents the core requirement from being satisfied. On src/scripts/main.js, you are calling Number.isNaN(parsedValue) while parsedValue is still a string, so this check will always be false and the block that should accumulate totalPopulation and compute currentNumber never runs. This violates the requirement to “make sure that the given string can be converted to a number and convert it to number,” because you never actually validate the numeric result; you need to first convert the cleaned string (without commas) to a number (e.g., const num = Number(cleaned);) and then check Number.isNaN(num) before using it. In addition, your average currently divides by allPopulationSpans.length instead of the count of successfully parsed numeric values, so invalid entries would still affect the average, which contradicts the requirement to base calculations only on values that can be converted to numbers.
To fix this, update the loop so that you: 1) strip commas from the span text, 2) convert that to a number, 3) continue or skip when Number.isNaN(parsedNumber) is true, and 4) only then add to totalPopulation and increment a separate validCount. Use validCount as the divisor when computing the average so both total and average reflect only valid numeric populations. Your DOM selection, formatting with Intl.NumberFormat('en-US'), and writing into .average-population and .total-population are all set up correctly, so once the parsing/validation and average divisor logic are corrected, your solution should 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
| for (const spanEl of allPopulationSpans) { | ||
| const parsedValue = spanEl.textContent; | ||
|
|
||
| if (Number.isNaN(parsedValue)) { |
There was a problem hiding this comment.
parsedValue is still a string here; Number.isNaN(parsedValue) will always be false, so this if block never executes and totalPopulation stays 0. You should first convert parsedValue (after removing commas) to a number, then check Number.isNaN on that numeric value and only add it to the total if it is a valid number, as required by the "make sure that the given string can be converted to a number" step.
| const average = | ||
| allPopulationSpans.length > 0 | ||
| ? totalPopulation / allPopulationSpans.length |
There was a problem hiding this comment.
The average is always divided by allPopulationSpans.length, even though you are supposed to count only values that can actually be converted to numbers. Consider tracking how many entries were successfully parsed (i.e., not NaN) and using that count in the denominator so the average reflects only valid numeric populations.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing while you practice these concepts. You did a good job selecting all .population spans, stripping commas from the strings, converting them to numbers, and formatting the output with Intl.NumberFormat, which matches the requirements nicely. The main logic issue is that the if (Number.isNaN(currentNumber)) check in src/scripts/main.js (inside your loop over .population spans) is inverted: right now the true branch adds to totalPopulation and validCount, which means only NaN values are counted; you should instead skip/continue when Number.isNaN(currentNumber) is true and only add to the totals when it is false. Related to that, your average calculation currently checks allPopulationSpans.length > 0 but divides by validCount, which can be 0 if no valid numbers were parsed; this guard should be based on validCount > 0 to avoid a divide‑by‑zero and to follow the requirement that only valid parsed values count. A helpful pattern would be something like: if (!Number.isNaN(currentNumber)) { totalPopulation += currentNumber; validCount++; } and later if (validCount > 0) { const average = totalPopulation / validCount; ... }, which keeps the logic clearly focused on valid numbers. Overall, your structure is solid and you’re very close—tightening these two conditions will make your total and average calculations both correct and robust.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
|
|
||
| for (const spanEl of allPopulationSpans) { | ||
| const parsedValue = spanEl.textContent; | ||
| const currentNumber = Number(parsedValue.split(',').join('')); |
There was a problem hiding this comment.
Here you convert the cleaned string to a number correctly, but when you later check Number.isNaN(currentNumber), you use the true branch to add to the total. That means only NaN values are accumulated, while valid numbers are skipped. Flip the condition so that you continue/skip when Number.isNaN(currentNumber) is true, and only add to totalPopulation and increment validCount when it is false.
| const parsedValue = spanEl.textContent; | ||
| const currentNumber = Number(parsedValue.split(',').join('')); | ||
|
|
||
| if (Number.isNaN(currentNumber)) { |
There was a problem hiding this comment.
This if (Number.isNaN(currentNumber)) condition is inverted relative to the requirement: you should skip values that are NaN and only aggregate values that can be converted to numbers. Update the logic so that invalid numbers are ignored and only valid parsed populations contribute to totalPopulation and validCount.
| const average = | ||
| allPopulationSpans.length > 0 ? totalPopulation / validCount : 0; |
There was a problem hiding this comment.
Here you guard the average calculation using allPopulationSpans.length, but divide by validCount. This can lead to division by zero if no values were successfully parsed. Since the requirements say to base calculations on values that can be converted to numbers, adjust this condition to depend on validCount (e.g., only compute the average when validCount > 0).
DEMO LINK