Skip to content

solution#2190

Open
uselessmorning wants to merge 3 commits intomate-academy:masterfrom
uselessmorning:develop
Open

solution#2190
uselessmorning wants to merge 3 commits intomate-academy:masterfrom
uselessmorning:develop

Conversation

@uselessmorning
Copy link

No description provided.

Copy link

@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 a few core task requirements are not yet fully met and they can break correctness in edge cases. Your script correctly reads the population values, sums them, computes an average, and writes formatted results into the .average-population and .total-population spans, which is great.

Critical issues that need fixing:

  • The selector currently uses .population without restricting to spans; the requirement is to read data from span.population, so update the query to document.querySelectorAll('span.population') (e.g. around the line where you collect the nodes).
  • You use replaceAll(',', '') to strip thousands separators; this may not be supported in all environments, so switch to a regular expression like el.textContent.replace(/,/g, '') before converting to Number.
  • After parsing, you don’t validate that the result is a real number; per the requirements, check each parsed value with Number.isFinite(value) (or !Number.isNaN(value)) and only then add it to total and push it into counts, skipping invalid entries.
  • When no valid numbers are found, counts.length can be 0, leading to total / counts.length becoming Infinity or NaN; guard the average calculation with if (counts.length > 0) and otherwise leave the output empty or set a sensible default.
  • Call toLocaleString('en-US') only on confirmed numeric totals/averages after these checks, so formatting is always applied to valid numbers and matches the comma style of the input.

Once you address these points, your solution will both match the explicit requirements and behave robustly in the face of malformed or empty data. Keep going—you’re very close to a solid, production-ready implementation.


✨ 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

'use strict';

// write your code here
const population = document.querySelectorAll('.population');

Choose a reason for hiding this comment

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

The task requires getting text data from span elements with class population. Change the selector to target spans explicitly, e.g. document.querySelectorAll('span.population') so you match the requirement exactly.

const counts = [];

population.forEach((el) => {
const number = Number(el.textContent.replaceAll(',', ''));

Choose a reason for hiding this comment

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

Using replaceAll may not be supported in all environments. Use a RegExp to remove thousands separators reliably, e.g. el.textContent.replace(/,/g, '').

Comment on lines 12 to 15
const number = Number(el.textContent.replaceAll(',', ''));

total += number;
counts.push(number);

Choose a reason for hiding this comment

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

You convert the string to a number but do not validate the conversion. Per the requirements, ensure the parsed value is a valid number (e.g. Number.isFinite(number) or !Number.isNaN(number)) and only then add it to total and counts — skip invalid entries.

counts.push(number);
});

const average = total / counts.length;

Choose a reason for hiding this comment

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

If no valid numeric entries are found counts.length will be 0 and total / counts.length will produce Infinity/NaN. Guard the average calculation (compute average only when counts.length > 0, otherwise set a sensible default or leave the fields empty).

Comment on lines 20 to 21
totalField.textContent = total.toLocaleString('en-US');
averageField.textContent = average.toLocaleString('en-US');

Choose a reason for hiding this comment

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

Call toLocaleString only on confirmed numeric results. After validating numbers and handling the empty-list case, format total and average with toLocaleString('en-US') so the thousands separator matches the populations' style.

Copy link

@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 one core checklist requirement is still not met, which blocks full task completion. The task description still contains the placeholder demo link https://<your_account>.github.io/js_get_data_DOM/ instead of your real GitHub username, which violates the explicit requirement to update this link [CHECKLIST ITEM #1]. This is critical because the reviewer must be able to reach your live demo from the description.

To fix this, open the description file where the task text lives, find the line with https://<your_account>.github.io/js_get_data_DOM/, and replace <your_account> with your actual GitHub username (for example: https://your-username.github.io/js_get_data_DOM/). After that change, double-check in the rendered description that the link is clickable and correctly points to your GitHub Pages deployment. Once this is updated, your JavaScript implementation in src/scripts/main.js already satisfies the parsing, validation, aggregation, and formatting requirements, so the task should be ready to pass. Your logic for handling span.population, validating numeric values, guarding against division by zero, and formatting with toLocaleString('en-US') looks solid—this is just about completing the last required checklist detail.


✨ 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

'use strict';

// write your code here
const population = document.querySelectorAll('span.population');

Choose a reason for hiding this comment

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

The task description still contains the demo link placeholder <your_account>. Replace it with your GitHub username as required by checklist item #1 (DEMO LINK: https://<your_account>.github.io/js_get_data_DOM/). See the description file for the requirement.

Copy link

@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 as an educational override so you can keep progressing while you address the remaining checklist items. Your src/scripts/main.js implementation correctly collects all span.population values, strips thousands separators, safely converts them to numbers, guards against invalid data, and then writes the total and average back into .total-population and .average-population using toLocaleString('en-US'), which fully meets the core JavaScript requirements. The main issue left is that the task description still contains the placeholder demo URL https://<your_account>.github.io/js_get_data_DOM/ instead of your real GitHub username, so please update that link to your actual Pages URL so the demo works for reviewers [CHECKLIST ITEM #1]. In addition, several non-JS files (like README.md, package.json, and workflow templates) were modified even though the instructions asked you to only change main.js; for future tasks, try to keep changes scoped to the requested file or clearly document why additional edits are needed. As a small optional improvement, you could add a brief comment near where you strip commas and use Number.isFinite to make your intent clearer for future readers. Overall, your core logic is solid and well-structured—fixing the demo link and keeping changes tightly scoped will bring the submission fully in line with the assignment expectations.


✨ 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