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 there are a few requirements that directly affect correctness and robustness that are not yet met. Your main logic (querying .population spans, converting text to numbers, summing, averaging, and writing to .total-population / .average-population) is solid, but it currently assumes all values are valid numbers. If any span has unexpected content or an empty string, Number(...) will yield NaN, which then propagates through your populationArray and makes both total and average become NaN when you call reduce and divide [CHECKLIST ITEM #12]. You also compute the average without guarding against the case where there are zero valid population values, which would cause a division by zero and produce NaN [CHECKLIST ITEM #3]. In addition, toLocaleString() is used without an explicit locale, so on some systems the formatting might not match the comma-separated style required by the task; you should pass a concrete locale like toLocaleString('en-US') or equivalent options to guarantee the correct thousands separator [CHECKLIST ITEMS #7 and #8].
To fix this, update the mapping step to validate conversions (e.g., using Number.isFinite(...) or !Number.isNaN(...)) and either filter out invalid entries or skip them when building populationArray. Before computing the average, handle the case where the filtered array is empty (for example, by setting the average to 0 or skipping the update entirely). Also, keep the numeric formatting deterministic by explicitly specifying the locale/options in toLocaleString. Once these issues are addressed, your solution will meet the core functional and robustness requirements and should be in good shape going forward.
✨ 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
| const populationArray = Array.from(populationEl).map((el) => { | ||
| return Number(el.textContent.replace(/,/g, '')); |
There was a problem hiding this comment.
You convert each span to a number with Number(el.textContent.replace(/,/g, '')) but don't verify the conversion succeeded. If conversion yields NaN it will corrupt total/average. Consider filtering out invalid values or checking with Number.isFinite() / !isNaN() before including them in the array (checklist item #12).
src/scripts/main.js
Outdated
| return Number(el.textContent.replace(/,/g, '')); | ||
| }); | ||
|
|
||
| const total = populationArray.reduce((acc, curr) => acc + curr, 0); |
src/scripts/main.js
Outdated
| }); | ||
|
|
||
| const total = populationArray.reduce((acc, curr) => acc + curr, 0); | ||
| const average = Math.round(total / populationArray.length); |
There was a problem hiding this comment.
Average is computed without guarding against an empty array (populationArray.length may be 0), which would produce NaN. Add a guard (e.g. set average to 0 or skip calculation when length is 0) to satisfy checklist item #3.
src/scripts/main.js
Outdated
| totalRes.textContent = total.toLocaleString(); | ||
| averageRes.textContent = average.toLocaleString(); |
There was a problem hiding this comment.
You use toLocaleString() which depends on the runtime environment. The task requires the same thousands-separator style as the populations shown; to guarantee that, explicitly pass a locale or formatting options (for example toLocaleString('en-US')) so the separator matches the source data (checklist items #7 and #8).
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 there are still a couple of critical correctness issues that can break the required behavior. First, when filteredArr.length === 0 you set both result elements to '0', but the code then continues and later divides by filteredArr.length, which is zero, causing the average to become NaN and overwrite the displayed values; you should either return immediately after setting the zeros or wrap the calculation in an else block so division only happens when there is at least one valid number. Related to this, the average computation itself must be guarded so that Math.round(total / filteredArr.length) is only executed when filteredArr.length > 0 [CHECKLIST ITEM #3].
Another critical issue is the use of inconsistent and nonstandard locale tags ('en-En' / 'en-EN') in toLocaleString, which can lead to unpredictable formatting and incorrect thousands separators; use a concrete, standard locale like 'en-US' consistently for both total and average [CHECKLIST ITEMS #7 and #8]. On the parsing side, you should trim whitespace in addition to removing commas (e.g., el.textContent.replace(/,/g, '').trim()) and use Number.isFinite or !Number.isNaN instead of the global isNaN to more reliably filter valid numeric conversions [CHECKLIST ITEM #12]. As a minor but helpful improvement, keeping the formatting options identical for both total and average will make the output more consistent with the source populations. Once you address these points, your core pipeline (read → parse → filter → compute → format → render) 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 populationArray = Array.from(populationEl).map((el) => { | ||
| const clean = el.textContent.replace(/,/g, ''); | ||
| const n = Number(clean); | ||
|
|
||
| return n; |
There was a problem hiding this comment.
When extracting the numeric string, trim whitespace as well as remove commas. For example: el.textContent.replace(/,/g, '').trim(). This helps avoid unexpected conversion issues from stray spaces and matches the requirement to ensure the string can be converted to a number.
src/scripts/main.js
Outdated
|
|
||
| return n; | ||
| }); | ||
| const filteredArr = populationArray.filter((n) => !isNaN(n)); |
There was a problem hiding this comment.
Filtering with !isNaN(n) uses the global isNaN (which coerces) and is less explicit. Prefer Number.isFinite(n) or !Number.isNaN(n) to reliably detect valid numeric conversions (addresses checklist item #12).
src/scripts/main.js
Outdated
| if (filteredArr.length === 0) { | ||
| totalRes.textContent = '0'; | ||
| averageRes.textContent = '0'; | ||
| } |
There was a problem hiding this comment.
You set both results to '0' when filteredArr.length === 0, but execution continues and later code overwrites these values. Exit early (e.g., return after setting the texts) or wrap the subsequent calculations in an else block so you don't compute average/reduce on an empty array.
src/scripts/main.js
Outdated
| const total = filteredArr.reduce((acc, curr) => acc + curr, 0); | ||
| const average = Math.round(total / filteredArr.length); |
There was a problem hiding this comment.
Computing average with Math.round(total / filteredArr.length) without guarding against filteredArr.length === 0 risks division by zero and producing NaN. Only compute the average when there is at least one valid value.
src/scripts/main.js
Outdated
| totalRes.textContent = total.toLocaleString('en-En'); | ||
| averageRes.textContent = average.toLocaleString('en-EN'); |
There was a problem hiding this comment.
The locale tags passed to toLocaleString are incorrect/inconsistent ('en-En' and 'en-EN'). Use a concrete, standard locale like 'en-US' (the same for both total and average) so the thousands separator is deterministic and matches the required numeric style.
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 as an educational override so you can keep progressing while you learn, even though there are a couple of points to improve.
You did a strong job on the core logic: you correctly query all .population spans, trim whitespace, remove commas, convert to Number, filter with Number.isFinite, and then safely compute total and average only when there is at least one valid number. You also format both total and average using toLocaleString('en-US'), which keeps the thousands separators consistent with the source data. The main issue to fix next time is that your pull request changes files beyond src/scripts/main.js (for example, README or config files), which goes against the “change only main.js” requirement—keep all unrelated files untouched for this kind of task. In addition, it’s important to actually run npm run lint as requested and address any warnings or errors so your code style and correctness are automatically checked.
For robustness, consider adding a small guard so the script safely exits if the result elements for total or average aren’t found in the DOM, and in the no-data case you might want to use (0).toLocaleString('en-US') instead of a plain '0' string to keep formatting consistent. Overall, your implementation of the parse → validate → compute → format → render pipeline is clear and correct—nice work, and these remaining tweaks will help you align fully with the project requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK