Skip to content

add task solution#2246

Open
vectormeleshko wants to merge 1 commit into
mate-academy:masterfrom
vectormeleshko:master
Open

add task solution#2246
vectormeleshko wants to merge 1 commit into
mate-academy:masterfrom
vectormeleshko:master

Conversation

@vectormeleshko
Copy link
Copy Markdown

No description provided.

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

Your solution is approved because it correctly gathers all .population spans, converts their text content to numbers, computes the total and average, and writes the results into the .total-population and .average-population elements as required. In src/scripts/main.js (around the block where you parse population text and update totalPopulation and averagePopulation), you currently call Number(...) on the cleaned string but do not explicitly check for invalid results; to fully meet the requirement that each string “can be converted to a number,” consider validating with something like const n = Number(cleaned); if (!Number.isFinite(n)) { /* skip or handle */ }. In the same area where you compute the average, you divide by elements.length without guarding against the case where no .population elements are found; for robustness, you could early-return or set total and average to 0 (or leave the original text) when elements.length === 0. Also, when formatting the output, you currently use toLocaleString('en-US'), which forces US-style separators; using toLocaleString() without a fixed locale or inferring separators from the input strings would better match the numeric style presented in the list. Overall, the main functional requirements are satisfied, and with these refinements you can make your solution more robust and better aligned with the specification.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment thread src/scripts/main.js
let totalPopulation = 0;

elements.forEach(el => {
const population = Number(el.textContent.replace(/,/g, ''));
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The code converts the string to a number using Number(...) but does not validate the result. The requirement says to "make sure that the given string can be converted to a number" — you should check the parsed value (e.g., with Number.isFinite(parsed) or !Number.isNaN(parsed)) and decide how to handle invalid entries (skip them or notify).

Comment thread src/scripts/main.js
totalPopulation += population;
})

let averagePopulation = Math.round(totalPopulation / elements.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.

Average is calculated by dividing by elements.length without checking whether any .population elements were found. If elements.length is 0 this yields Infinity/NaN. Guard against an empty NodeList (for example, skip calculation or set totals/average to 0 when elements.length === 0).

Comment thread src/scripts/main.js
Comment on lines +17 to +18
totalPopulationElement.textContent = totalPopulation.toLocaleString('en-US');
averagePopulationElement.textContent = averagePopulation.toLocaleString('en-US');
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 use toLocaleString('en-US') which forces US-style separators. The requirement asks to use the same numeric style as the populations presented; consider inferring the style from the existing population strings or using toLocaleString() (no locale) so it matches the user's locale or the format in the list, instead of hardcoding 'en-US'.

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