Solution - #2064
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your sorting logic works correctly, but the implementation does not meet the explicit requirements. The task specifies two required functions: sortList(list) to sort employees by salary and getEmployees(list) to return an array of employee objects with { name, position, salary, age } schema—both are completely missing from your code. You must also use item.dataset.salary instead of getAttribute('data-salary') as specified, and extract the salary conversion into a separate helper function. Finally, call both functions so the employees array is actually retrieved. These are not stylistic preferences—they are explicit requirements that define the expected code structure.
✨ 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 List = document.querySelector('ul'); | ||
| const list = Array.from(document.querySelectorAll('li')); |
There was a problem hiding this comment.
The task requires a sortList function that takes a list parameter. The current implementation uses global variables instead of a function. Wrap the sorting logic in a function that accepts a list parameter.
| const salariesMap = new Map(); | ||
|
|
||
| list.forEach((item) => { | ||
| const value = +item.getAttribute('data-salary').replace(/[$,]/g, ''); |
There was a problem hiding this comment.
The task requires using dataset for accessing data attributes (e.g., item.dataset.salary instead of getAttribute('data-salary')).
| const salariesMap = new Map(); | ||
|
|
||
| list.forEach((item) => { | ||
| const value = +item.getAttribute('data-salary').replace(/[$,]/g, ''); |
There was a problem hiding this comment.
The task requires a helper function to convert salary string to number. Extract the salary conversion logic into a separate helper function.
| for (let i = 0; i < sortedSalaries.length; i++) { | ||
| List.appendChild(sortedSalaries[i][0]); |
There was a problem hiding this comment.
The task requires a getEmployees function that takes a list parameter and returns an array of employee objects with schema { name, position, salary, age }. This function is completely missing.
|
|
||
| for (let i = 0; i < sortedSalaries.length; i++) { | ||
| List.appendChild(sortedSalaries[i][0]); | ||
| } |
There was a problem hiding this comment.
The task requires calling both functions and getting an array of employees. The code doesn't call getEmployees() to retrieve the employees array.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your code has critical issues that block approval. The function is named sortingList but the requirements explicitly specify it must be named sortList. More importantly, the getEmployees function is completely missing - this is a required function that must return an array of employee objects with the schema { name, position, salary, age }. Additionally, the salary conversion helper converter is nested inside sortingList when it should be a standalone function outside of sortList as specified in the requirements. Finally, the sorting logic is being called inside a for loop, causing it to execute multiple times unnecessarily - call the function once and store the result, then iterate over it.
✨ 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 list = document.querySelectorAll('li'); | ||
| const salariesMap = new Map(); | ||
|
|
||
| function sortingList(nodelist) { |
There was a problem hiding this comment.
The function is named sortingList but the requirements explicitly specify it must be named sortList. Rename this function to match the specification.
| for (let i = 0; i < sortingList(list).length; i++) { | ||
| List.appendChild(sortingList(list)[i][0]); |
There was a problem hiding this comment.
The getEmployees function is completely missing. Requirements specify it must return an array of employee objects with the schema: { name, position, salary, age }. You need to create this function that extracts these properties from each list item.
| nodelist.forEach((item) => { | ||
| function converter(val) { |
There was a problem hiding this comment.
The helper function converter is nested inside sortingList. The requirements state: 'write helper function for it' (salary conversion) - it should be a standalone function outside of sortList, not a nested function.
| for (let i = 0; i < sortingList(list).length; i++) { | ||
| List.appendChild(sortingList(list)[i][0]); |
There was a problem hiding this comment.
The sorting logic is called inside the for loop, causing it to execute multiple times unnecessarily. Call the function once and store the result, then iterate over it.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
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.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This is an educational approval to help you continue learning. You've done well implementing sortList correctly with descending salary sort and keeping the converter helper standalone. However, there's still a critical missing piece: the getEmployees function is completely absent — this is explicitly required and must accept a list parameter, returning an array of objects with { name, position, salary, age } extracted from data attributes. Additionally, the task requires calling both functions, which isn't happening yet. Once you add getEmployees and invoke both functions, you'll have a complete solution. Keep going — you're on the right track!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const List = document.querySelector('ul'); | ||
| const list = document.querySelectorAll('li'); | ||
| const salariesMap = new Map(); | ||
|
|
||
| function converter(item) { | ||
| const value = +item.dataset.salary.replace(/[$,]/g, ''); | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| function sortList(nodelist) { | ||
| nodelist.forEach((item) => { | ||
| salariesMap.set(item, converter(item)); | ||
| }); | ||
|
|
||
| const sortedArr = Array.from(salariesMap.entries()); | ||
|
|
||
| sortedArr.sort((a, b) => b[1] - a[1]); | ||
|
|
||
| return sortedArr; | ||
| } | ||
|
|
||
| const sortedList = sortList(list); | ||
|
|
||
| for (let i = 0; i < sortedList.length; i++) { | ||
| List.appendChild(sortedList[i][0]); | ||
| } |
There was a problem hiding this comment.
The getEmployees function is completely missing. Requirements specify this function must exist, take a list parameter, and return an array of employee objects with the schema: { name, position, salary, age }. This must be implemented.
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const List = document.querySelector('ul'); | ||
| const list = document.querySelectorAll('li'); | ||
| const salariesMap = new Map(); | ||
|
|
||
| function converter(item) { | ||
| const value = +item.dataset.salary.replace(/[$,]/g, ''); | ||
|
|
||
| return value; | ||
| } | ||
|
|
||
| function sortList(nodelist) { | ||
| nodelist.forEach((item) => { | ||
| salariesMap.set(item, converter(item)); | ||
| }); | ||
|
|
||
| const sortedArr = Array.from(salariesMap.entries()); | ||
|
|
||
| sortedArr.sort((a, b) => b[1] - a[1]); | ||
|
|
||
| return sortedArr; | ||
| } | ||
|
|
||
| const sortedList = sortList(list); | ||
|
|
||
| for (let i = 0; i < sortedList.length; i++) { | ||
| List.appendChild(sortedList[i][0]); | ||
| } |
There was a problem hiding this comment.
The requirements state 'Call both functions' but only sortList is called. The getEmployees function needs to be called as well.
2 participants
No description provided.