Skip to content

add task solution#2245

Open
excmka wants to merge 2 commits intomate-academy:masterfrom
excmka:develop
Open

add task solution#2245
excmka wants to merge 2 commits intomate-academy:masterfrom
excmka:develop

Conversation

@excmka
Copy link
Copy Markdown

@excmka excmka commented Apr 20, 2026

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

I am asking for some changes before this can be considered complete because an explicit task constraint was not met, even though your main.js logic itself correctly computes and formats the total and average population. The instructions specified that only src/scripts/main.js should be modified, but your submission also changes package.json, package-lock.json, README.md, and a GitHub workflow file, which conflicts with that requirement and can break the automated grading setup. To fix this, please revert all changes outside src/scripts/main.js so that only your population-calculation code remains in the PR. As a minor improvement (not required for acceptance after you address the file-scope issue), you could guard against the case where there are no .population spans before dividing to compute the average, so that you avoid producing NaN in that edge case. Another small enhancement would be to skip or log clearly non-numeric span values instead of silently treating them as 0, which would make your totals more robust. Once only main.js is changed and the extra file modifications are removed, your solution will align well with the task requirements—your core implementation is on the right track.


✨ 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

Copy link
Copy Markdown

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

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, while your src/scripts/main.js implementation correctly parses .population spans, computes total and average, and formats them with thousands separators, the PR also modifies README.md, which violates the requirement that only src/scripts/main.js be changed ([CHECKLIST ITEM #5] and [CHECKLIST ITEM #11]). To resolve this blocker, revert all changes in README.md so that the diff only contains your JavaScript updates (for example, by checking out the original version of README.md from the base branch). Once that is done, the functional parts of the task will be fully aligned with the project constraints.

As optional robustness improvements in src/scripts/main.js, you could guard against the case where there are no .population spans so that average is only computed when numbers.length > 0, and skip or log non-numeric span values instead of silently treating them as 0. These suggestions help avoid NaN averages and misleading totals in malformed input scenarios but are not required for acceptance. Overall, your core logic and DOM manipulation look solid; after reverting the extra file changes, your solution should be in great shape.


✨ 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
const numbers = Array.from(spans).map((span) => {
const num = Number(span.textContent.replace(/,/g, ''));

return isNaN(num) ? 0 : num;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You map invalid parses to 0 here which silently includes them in totals/average. Consider skipping non-numeric span values or logging them instead so a malformed entry doesn't skew results (or, if intended, document that behavior).

Comment thread src/scripts/main.js
});

const total = numbers.reduce((sum, n) => sum + n, 0);
const average = Math.round(total / numbers.length);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If there are no .population spans numbers.length will be 0 and the average becomes NaN. Consider guarding the division (e.g. compute average only when numbers.length > 0, otherwise default to 0) to make the code more robust.

Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution because it correctly selects all span.population elements, parses their text into numbers, computes the total and average, and writes nicely formatted values (with thousands separators) into the .total-population and .average-population spans. Overall, the logic in src/scripts/main.js is clear and concise, and it fulfills the main task requirements.

This is an educational approval override so you can keep progressing while you learn, but there are a couple of robustness improvements to keep in mind for future work. In the section where you compute the average, e.g. near const average = Math.round(total / numbers.length); (around the middle of the file), you should guard against the case where there are no .population spans so you don’t end up dividing by zero and producing NaN or Infinity. Also, in the part where you convert text to numbers, any non-numeric content is effectively treated as 0; a more robust approach would be to skip those entries or log a warning so bad data doesn’t silently change your totals and average. Finally, remember to run npm run lint (if configured in your project) to catch style or potential error issues early.

You’re very close to a production-ready solution here—nice job meeting the core requirements and keeping the code straightforward and readable; keep these edge-case checks in mind as you continue improving your skills.


✨ 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