Skip to content

solutions#2182

Open
OliverBars wants to merge 1 commit intomate-academy:masterfrom
OliverBars:develop
Open

solutions#2182
OliverBars wants to merge 1 commit intomate-academy:masterfrom
OliverBars:develop

Conversation

@OliverBars
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 checklist-required behaviors are not yet fully met, and they can lead to incorrect results at runtime. Your main logic for selecting .population spans, converting to numbers, and writing total/average (with toLocaleString) looks good, but there are some critical edge cases missing.

Critical issues to fix:

  • The parsing currently only removes commas; it doesn’t trim or strip other non-digit characters (e.g. non‑breaking spaces or stray text), so Number(text) can still produce NaN and break the calculations [CHECKLIST ITEM #11]. At the point where you read span.innerText (in src/scripts/main.js), normalize the string (e.g. trim(), remove \u00A0, and drop non‑numeric characters) before conversion.
  • After calling Number(...), you are not checking for invalid values; per the requirements you must validate with Number.isNaN() or Number.isFinite() and either skip invalid entries or handle them explicitly [CHECKLIST ITEMS #11 and #12]. For example, build an array of only valid numeric populations and ignore anything that can’t be converted.
  • When there are no valid numeric entries (e.g. all invalid or no .population spans), total / populations.length will divide by zero, producing NaN and an incorrect average [CHECKLIST ITEM #3]. Add a guard so that if the filtered array is empty you either set the average to 0 or avoid doing the division.

Minor but recommended improvements:

  • Before writing results, check that .total-population and .average-population elements exist from document.querySelector to avoid possible runtime errors, and assign the rounded average to a variable before formatting so it’s easy to test.
    Once you’ve tightened the parsing, validated numbers, and handled the empty-array case, your solution will fully satisfy the functional requirements; the structure is solid and just needs these robustness fixes.

✨ 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

const populationSpans = document.querySelectorAll('.population');

const populations = Array.from(populationSpans).map((span) => {
const text = span.innerText.replace(/,/g, '');

Choose a reason for hiding this comment

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

Parsing removes commas but doesn't trim or handle other possible characters (e.g. non-breaking spaces or stray text). Consider trimming and removing non-digit characters before conversion so the value is reliably convertible. This relates to checklist item #11.

const populations = Array.from(populationSpans).map((span) => {
const text = span.innerText.replace(/,/g, '');

return Number(text);

Choose a reason for hiding this comment

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

You convert using Number(text) but do not check for NaN. Per checklist items #11 and #12 you must ensure each string can be converted to a number — validate with Number.isNaN() (or isFinite) and decide whether to skip invalid entries or handle them explicitly.

});

const total = populations.reduce((sum, val) => sum + val, 0);
const average = total / populations.length;

Choose a reason for hiding this comment

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

If populations is empty the computation total / populations.length will produce NaN or throw a logic error. Add a guard for an empty array (e.g. set average to 0 or avoid division) to satisfy correct average calculation (checklist item #3).

const total = populations.reduce((sum, val) => sum + val, 0);
const average = total / populations.length;

document.querySelector('.total-population').innerText = total.toLocaleString();

Choose a reason for hiding this comment

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

Writing directly to .total-population assumes the element exists. It’s safer to check the result of querySelector before setting innerText to avoid runtime errors. Also confirm you want toLocaleString() formatting here (this is correct for checklist items #7 and #8).

document.querySelector('.total-population').innerText = total.toLocaleString();

document.querySelector('.average-population').innerText =
Math.round(average).toLocaleString();

Choose a reason for hiding this comment

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

Average is rounded and formatted which is fine, but compute/assign the rounded value to a variable first for clarity and easier testing. Also, ensure the target element exists before assigning innerText (see previous comment). This ties to checklist items #5, #7 and #8.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

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.

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]

If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

good job, but don't forget to provide a demo link

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.

3 participants