add task solution by LiliiaVol · Pull Request #2194 · mate-academy/js_get_data_DOM

LiliiaVol · 2026-03-01T00:16:30Z

No description provided.

LiliiaVol · 2026-03-01T00:17:09Z

https://liliiavol.github.io/js_get_data_DOM/

mateacademy-ai-mentor

Review Summary

Great job getting the core calculations for total and average population working! However, a couple of changes are needed before this can be approved.

The primary issue is the logic for formatting numbers with thousands separators. Your current implementation is hardcoded and doesn't work correctly for all numbers, which is a key requirement. For a much simpler and more reliable solution, I recommend looking into the built-in number.toLocaleString() method.

Additionally, the task instructions specified that only the main.js file should be changed. Please revert the modifications made to the style files.

Once these issues are addressed, your solution will be in great shape. Keep up the good work!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-01T00:18:11Z

src/scripts/main.js

+  population.forEach((populationContent) => {
+    let num = '';
+
+    for (let i = 0; populationContent.textContent.length >= i; i++) {


The loop condition here should be i < populationContent.textContent.length. Using >= causes the loop to run one extra time where populationContent.textContent[i] will be undefined. While it doesn't break your code in this case because of how Number(undefined) is handled, it's a common source of bugs and logically incorrect.

mateacademy-ai-mentor · 2026-03-01T00:18:11Z

src/scripts/main.js

+  for (let i = arrayOfNum.length - 1; i >= 0; i--) {
+    if (i % 3 === 0 && i !== arrayOfNum.length - 1) {
+      result += ',';
+      result += arrayOfNum[i];
+    } else {
+      result += arrayOfNum[i];
+    }
+  }


This logic for adding thousands separators is not quite right. The condition i % 3 === 0 depends on the index from the start of the number, but it should be based on groups of three digits from the end. As a result, this will format numbers incorrectly (e.g., 12345 would become 1,2345). A much simpler and more reliable way to achieve this is with the built-in number.toLocaleString() method.

mateacademy-ai-mentor · 2026-03-01T00:18:11Z

src/scripts/main.js

+    return result;
+  }
+
+  result = result.split(',').join('');


It's inefficient to remove the commas from the formatted result string just to calculate the average. The original numeric sum is still available in the total variable. You should use total directly to perform this calculation, for example: const average = Math.round(total / population.length);.

mateacademy-ai-mentor · 2026-03-01T00:18:11Z

src/scripts/main.js

+  result = '';
+
+  for (let i = arrayOfNumAvarage.length - 1; i >= 0; i--) {
+    if (i === 5 || i === 2) {


This condition is "hardcoded" and will only work for formatting a 9-digit number. This logic is not robust and will fail if the average population has a different number of digits. Your formatting logic should be generic enough to work for any number. The toLocaleString() method mentioned earlier would be a great solution here as well.

mateacademy-ai-mentor

Review Summary

Excellent work on the JavaScript logic! Your use of toLocaleString() to format the numbers is a great solution and works perfectly.

However, this submission requires a change because it includes modifications to style files, such as src/styles/main.scss. The task instructions explicitly state that only the main.js file should be modified.

Please remove the style files from your pull request and resubmit with only the changes to main.js. After that small correction, your solution will be ready to go!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-03-01T00:31:35Z

src/scripts/main.js

+    for (let i = 0; populationContent.textContent.length > i; i++) {
+      if (
+        Number(populationContent.textContent[i]) ||
+        Number(populationContent.textContent[i]) === 0
+      ) {
+        num += populationContent.textContent[i];
+      }
+    }


This loop works for extracting the digits, but a more common and concise way to handle this is by using the replace() method to remove the commas from the string before converting it to a number. For example: const numberString = populationContent.textContent.replace(/,/g, '');.

mateacademy-ai-mentor · 2026-03-01T00:31:35Z

src/scripts/main.js

+    return result;
+  }
+
+  result = Math.round(Number(total) / population.length);


Since total is already a number, wrapping it in Number() here is redundant. You can simplify this to Math.round(total / population.length).

mateacademy-ai-mentor · 2026-03-01T00:31:35Z

src/scripts/main.js

+document.querySelector('.total-population').textContent = getTotalSum('total');
+document.querySelector('.average-population').textContent = getTotalSum('');


Calling getTotalSum twice causes the total to be calculated two separate times, which is inefficient. A better approach is to calculate the total only once, then derive the average from that total, and finally update both elements in the HTML.

mateacademy-ai-mentor · 2026-03-01T00:31:35Z

src/styles/main.scss


 body {
-  background: #eee;
+  counter-reset: section;


The task instructions specify that only the main.js file should be changed. Please remove any style files from your submission to adhere to this requirement.

mateacademy-ai-mentor

Review Summary

I'm approving this solution as an educational override to help you progress, but please pay close attention to the feedback for future tasks.

Great job on the JavaScript logic! Your calculations are correct, and you've used toLocaleString() perfectly to format the numbers.

However, a critical requirement for this task was to only modify the main.js file. Your submission included changes to style files (.scss), which should be avoided. Please make sure to only commit the required files in your future pull requests.

Also, I noticed a small typo in your variable name: avarage should be average. Correcting this improves code readability.

Keep up the great work, and remember to carefully read the task requirements!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-03-01T00:46:52Z

src/styles/_fonts.scss

 @font-face {
  font-family: Roboto, Arial, Helvetica, sans-serif;
-  src: url('../fonts/Roboto-Regular-webfont.woff') format('woff');
  font-weight: normal;
  font-style: normal;
+  src: url('../fonts/Roboto-Regular-webfont.woff') format('woff');


According to the task requirements, only the main.js file should be modified. Please remove this file and other style files from your submission.

mateacademy-ai-mentor · 2026-03-01T00:46:52Z

src/scripts/main.js

+};
+
+const totalResult = getTotalSum('total');
+const avarage = getTotalSum('');


There's a small typo in this variable name. It should be average.

add task solution

65a68ac

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

fix

7b19cda

LiliiaVol requested a review from mateacademy-ai-mentor March 1, 2026 00:29

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

fix 3.0

0bce61c

LiliiaVol requested a review from mateacademy-ai-mentor March 1, 2026 00:45

mateacademy-ai-mentor approved these changes Mar 1, 2026

View reviewed changes

		document.querySelector('.total-population').textContent = getTotalSum('total');
		document.querySelector('.average-population').textContent = getTotalSum('');

Conversation

LiliiaVol commented Mar 1, 2026

Uh oh!

LiliiaVol commented Mar 1, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants