add task solution#2012
Conversation
PrimaSB
commented
Jun 21, 2026
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one explicit requirement of the task is not met: the description says to write a helper function to convert salary strings to numbers, but your code currently performs the conversion inline instead of via a dedicated helper. This is critical because requirements about specific functions are part of the grading criteria, even if the current behavior appears to work. On src/scripts/main.js, in sortList you parse salary using regex and Number(...), while in getEmployees (same file) you use Number(person.dataset.salary) directly; this inconsistency means salaries with formatting (e.g. currency symbols or spaces) might be interpreted differently in the two functions.
To fix this, extract the regex-based logic into a helper, for example function parseSalary(rawSalary) { ... }, and then call parseSalary(person.dataset.salary) everywhere you need a numeric salary (both in sortList and getEmployees). That way, the dataset salary format is handled in one place and both functions stay in sync with any future formatting changes. Aside from this, your overall structure, use of dataset, sorting in descending order, and creation of employee objects all look good. Once you add and reuse the helper function for salary parsing, your solution should fully satisfy the task requirements.
✨ 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
| const listElements = list.querySelectorAll('li'); | ||
| const elementsArray = [...listElements]; | ||
| const sorted = [...elementsArray].sort((a, b) => { | ||
| const salaryA = Number(a.dataset.salary.replace(/\D/g, '')); |
There was a problem hiding this comment.
The task explicitly asks to "write helper function" to convert salary strings to numbers. Here you are doing the conversion inline with Number(...) and regex instead of reusing a dedicated helper function.
| const employees = elementsArray.map((person) => ({ | ||
| name: person.dataset.name, | ||
| position: person.dataset.position, | ||
| salary: Number(person.dataset.salary), |
There was a problem hiding this comment.
Number(person.dataset.salary) may behave differently from the regex-based parsing you use above in sortList. Consider using a shared helper function for salary parsing so sortList and getEmployees treat the dataset salary format consistently.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly implements all required features and behaves as specified in the task. The sortList(list) function takes the list element, reads data-salary via dataset, converts it to a number using the shared toNumber helper, and sorts the employees in descending order before re-appending the li elements, which fulfills the sorting requirement. The getEmployees(list) function also takes the list, iterates over its li elements, and returns an array of objects with the exact schema { name, position, salary, age }, converting numeric fields with the same helper. You have kept your changes confined to src/scripts/main.js and left the HTML/CSS untouched, as required.
There are no critical or blocking issues remaining; everything necessary for the assignment is in place. As a minor, optional improvement, you might consider a more descriptive name than toNumber (for example, parseNumericData) to make the helper’s purpose clearer, but this is not required. Overall, this is a solid, requirement-compliant implementation—well done.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨