add task solution#2000
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
The implementation is missing a required helper function. The task description explicitly states you need to "write a helper function for the number conversion," but the salary parsing logic replaceAll(/\D/g, '') is currently embedded directly inside the sortList function. You need to extract this into a separate named function (e.g., parseSalary) and call it from sortList instead. This is a critical requirement violation, not a preference or style issue.
✨ 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
| function sortList(list) { | ||
| return list.sort((person1, person2) => { | ||
| return ( | ||
| Number(person2.dataset.salary.replaceAll(/\D/g, '')) - | ||
| Number(person1.dataset.salary.replaceAll(/\D/g, '')) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Missing requirement: Create a separate helper function to convert salary string to number. Currently the conversion replaceAll(/\D/g, '') is embedded directly in the sort function. Extract this logic into a helper function as required by the task description.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has a critical bug that breaks the salary sorting functionality. The parseSalary function in src/scripts/main.js evaluates the Number() conversion but doesn't actually return the result: function parseSalary(salary) { Number(salary.replaceAll(/\D/g, '')); // ❌ Missing 'return' }. This causes the function to return undefined, which means when sortList tries to compare salaries, it gets undefined - undefined = NaN instead of actual numbers. That's why only 5 items are being displayed instead of the expected 10. To fix this, simply add return before the Number() call: function parseSalary(salary) { return Number(salary.replaceAll(/\D/g, '')); // ✅ Now returns the value }. There's also a secondary issue: the DEMO LINK in your README still contains the placeholder <your_account> instead of your actual GitHub username. Replace that so the deployment link works correctly. Once you add the missing return keyword, the sorting test should pass. Good job on correctly extracting the helper function—that part of the implementation is correct.
✨ 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
| function parseSalary(salary) { | ||
| Number(salary.replaceAll(/\D/g, '')); |
There was a problem hiding this comment.
The parseSalary function doesn't return the converted number. The Number() call result is discarded, so the function returns undefined. Add return before Number() to actually return the parsed salary value.
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
Your solution is approved. The critical parseSalary bug has been successfully fixed - it now correctly returns the converted number value with the return keyword. All requirements are met: sortList sorts employees by salary in descending order, getEmployees returns properly structured objects with name, position, salary, and age properties, and both functions work together to append sorted results to the DOM. Great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK