Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on this task! You've correctly implemented most of the requirements, especially the data parsing and number formatting. However, I'm requesting a couple of changes to fix a logical error in the average calculation.
- Incorrect Average: The average is currently calculated by dividing the total population by the count of all
<span>elements. To get the correct average, you should divide the total by the count of only the valid numbers that were actually summed up. - Parsing Non-Numeric Text: The way non-numeric text (like 'N/A') is handled can lead to incorrect results. Your current code converts such text to
0, which skews the calculations. It would be more robust to ensure these values becomeNaNso they can be filtered out completely before summing.
Fixing these two points will make your solution fully correct. Keep up the great work!
✨ 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
src/scripts/main.js
Outdated
|
|
||
| for (const population of populations) { | ||
| const raw = population.textContent.trim(); | ||
| const number = Number(raw.replace(/[^\d.-]/g, '')); |
There was a problem hiding this comment.
This line handles numbers with commas correctly, but it has a side effect for text that doesn't contain any digits. For example, if a <span> contained 'N/A', raw.replace(/[^\[d.-]/g, '') would result in an empty string '', and Number('') evaluates to 0. This 0 would then be included in your calculations, which leads to an incorrect average. Consider a way to produce NaN for such cases so they can be properly filtered out later.
src/scripts/main.js
Outdated
| .filter((n) => !Number.isNaN(n)) | ||
| .reduce((acc, val) => acc + val, 0); | ||
|
|
||
| const average = Math.round(total / numbers.length); |
There was a problem hiding this comment.
The average is calculated using numbers.length, which is the total count of all population elements. However, the total is calculated from a filtered list of numbers. To get the correct average, you should divide the total by the count of valid numbers that were actually summed up.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the main feedback from the last review! The logic for calculating the average is much improved. However, this requires changes because a critical issue remains with handling non-numeric text. Strings like 'N/A' are currently being converted to 0 instead of being filtered out. This incorrectly increases the count of numbers used for the average, which leads to a wrong result. Please adjust your parsing logic so that non-numeric strings result in NaN. This will allow your existing .filter((n) => !Number.isNaN(n)) to work as intended and correctly exclude these values. You're very close to a perfect solution!
✨ 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
src/scripts/main.js
Outdated
|
|
||
| for (const population of populations) { | ||
| const raw = population.textContent.trim(); | ||
| const number = Number(raw.replace(/[^\d.-]/g, '')); |
There was a problem hiding this comment.
This regular expression is a bit too broad. It correctly handles numbers with commas, but it also converts non-numeric strings like 'N/A' into an empty string, which Number() then converts to 0. This 0 is then incorrectly included in your calculations.
Consider using a more specific regular expression that only removes the comma (,) as the thousands separator. This will cause Number() to correctly evaluate non-numeric strings to NaN, allowing your filter on line 14 to remove them as intended.
| @@ -7,6 +7,7 @@ | |||
| content="width=device-width, initial-scale=1.0" | |||
| /> | |||
| <title>Population top</title> | |||
There was a problem hiding this comment.
This line handles numeric strings with commas well, but there's a subtle issue with non-numeric strings like 'N/A'. After the replace call, 'N/A' becomes an empty string '', and Number('') evaluates to 0.
This 0 is then included when calculating the average, which leads to an incorrect result because it increases the count of numbers (allNumbers.length) without affecting the total sum.
Consider using parseInt() instead of Number(), as parseInt('') returns NaN, which would then be correctly filtered out by your logic on line 14.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this iteration! You've successfully addressed all the feedback from the previous review. Your logic for parsing strings, converting non-numeric values to NaN, and then filtering them out is now implemented perfectly. The code is clean, efficient, and fully meets all the requirements of the task. I am happy to approve your solution. Great job!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.