-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Solution #1524
base: master
Are you sure you want to change the base?
Solution #1524
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on the code! It's functional and gets the job done. 🎉 However, there are a few areas that could be improved for better readability and maintainability. Consider storing DOM elements in variables to avoid repeated querying. Also, remember to provide an initial value for the reduce method and simplify your arrow function inside the map method. Lastly, use more descriptive variable names for clarity. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
const population = [...document.querySelectorAll('.population')]; | ||
const toNumbers = population.map((element) => { | ||
return parseInt(element.textContent.replace(/,/g, '')); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit confusing to use the variable name 'population' for an array of DOM elements, and then 'toNumbers' for the actual population numbers. Consider renaming 'population' to something like 'populationElements' or 'populationNodes', and 'toNumbers' to 'populations' or 'populationNumbers'.
const toNumbers = population.map((element) => { | ||
return parseInt(element.textContent.replace(/,/g, '')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The arrow function inside the map method doesn't need to have a return statement since it's a single line of code. You can simplify it to (element) => parseInt(element.textContent.replace(/,/g, ''))
.
return parseInt(element.textContent.replace(/,/g, '')); | ||
}); | ||
|
||
const totalSum = toNumbers.reduce((sum, element) => sum + element, 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to provide an initial value for the reduce method, even though in this case, it defaults to the first element of the array when not provided. It makes the code more explicit and helps avoid potential bugs in the future if the code changes.
document.querySelector('.average-population').textContent = | ||
average.toLocaleString(); | ||
|
||
document.querySelector('.total-population').textContent = | ||
totalSum.toLocaleString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is repeatedly querying the DOM to find the '.average-population' and '.total-population' elements. This can be inefficient, especially if the DOM is large or if this operation is done frequently. Consider storing these elements in variables at the start of the script and reusing those variables.
DEMO LINK