Skip to content

add solution - #2308

Open
NataKovbasniuk wants to merge 2 commits into
mate-academy:masterfrom
NataKovbasniuk:develop
Open

add solution#2308
NataKovbasniuk wants to merge 2 commits into
mate-academy:masterfrom
NataKovbasniuk:develop

Conversation

@NataKovbasniuk

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/scripts/main.js Outdated
Comment on lines +9 to +11
const number = Number(cleaned);

numbers.push(number);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants