solution#939
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is well-structured with proper separation of concerns and meets most requirements, but there's a critical bug in the category filtering logic that must be fixed. In src/services/expenses.service.js, the filter callback in the getAll function is missing a return statement. The expression categories.includes(expense.category) evaluates correctly but implicitly returns undefined since arrow functions with curly braces require an explicit return. This causes all expenses to be filtered out when using multiple categories instead of returning matches. The same issue appears in src/services/users.service.js and src/controllers/users.controller.js. Add return before categories.includes(expense.category) in each filter callback to fix this. Once fixed, the filtering will work as intended and your implementation will satisfy all 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 user = usersService.getById(id); | ||
|
|
||
| if (!user) { |
There was a problem hiding this comment.
The filter callback doesn't return a value - it evaluates categories.includes(expense.category) but doesn't return it, so the filter always returns falsy. This means filtering by multiple categories as an array will return an empty array instead of matching expenses.
|
|
||
| return user; | ||
| }; |
There was a problem hiding this comment.
The filter callback is missing a return statement. The expression categories.includes(expense.category) evaluates but the result isn't returned, causing the filter to always return falsy and empty the results when categories is an array.
| if (Array.isArray(categories)) { | ||
| filteredExpenses = filteredExpenses.filter((expense) => { |
There was a problem hiding this comment.
The filter callback is missing a return statement. The expression categories.includes(expense.category) evaluates correctly but returns undefined (nothing is returned from the arrow function), causing the filter to always fail. This means when categories is an array, all expenses will be filtered out instead of returning matching ones.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved. The critical category filtering bug in src/services/expenses.service.js has been successfully fixed with the proper return statement, and all core requirements are now met. The application correctly implements 2 collections (users and expenses) with 5 REST endpoints each, proper 404/400 error handling, in-memory data persistence that resets on server restart, and Express app setup without calling listen(). The separation of concerns across routes, controllers, and services is well-structured. Great work on addressing the previous feedback and getting this project across the finish line!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.