solution#938
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
✨ 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
| 'use strict'; | ||
|
|
||
| const { | ||
| getExpenses, | ||
| getExpensesById, | ||
| creatEexpenses, | ||
| updatEexpenses, | ||
| deletEexpenses, | ||
| } = require('../services/expenses.service'); | ||
| const { isValid } = require('../services/helpers'); | ||
|
|
||
| function listExpenses(req, res) { | ||
| const expenses = getExpenses(req.query); | ||
|
|
||
| if (expenses) { |
There was a problem hiding this comment.
The isValid function returns true when payload IS empty (via isEmpty(payload)) and false when it has content. This is inverted - valid data should return true. This breaks expense creation validation since isValid will return false for any non-empty body.
|
|
||
| if (!isValid(Object.values(body))) { |
There was a problem hiding this comment.
The validation !isValid(Object.values(body)) will return 400 for valid expense data (non-empty body values), which is incorrect. The logic should check if required fields exist, not if values are empty.
| } else { | ||
| res.status(404).end(); |
There was a problem hiding this comment.
In listExpenses, the 404 response is triggered when getExpenses returns falsy. However, getExpenses returns an empty array [] (falsy) for valid queries with no results, not just when the user doesn't exist. This will incorrectly return 404 for empty result sets.
| 'use strict'; | ||
|
|
||
| // const express = require('express'); | ||
| const express = require('express'); | ||
| const cors = require('cors'); | ||
|
|
||
| const { notFound } = require('./middleware/notFound'); | ||
| const { usersRoute } = require('./routes/users'); | ||
| const { expensesRoute } = require('./routes/expenses'); | ||
|
|
There was a problem hiding this comment.
The isValid function treats empty arrays as valid (isEmpty([]) returns false) and non-empty arrays as invalid. This appears inverted from the intended behavior where validation should pass when values are present. Consider reviewing the logic - for valid input, isValid should return true.
| const { notFound } = require('./middleware/notFound'); | ||
| const { usersRoute } = require('./routes/users'); | ||
| const { expensesRoute } = require('./routes/expenses'); |
There was a problem hiding this comment.
The isEmpty function doesn't recognize empty arrays as 'empty'. An empty array [] is truthy, so isEmpty([]) returns false. This causes isValid([]) to incorrectly return false, making the validation logic inconsistent.
| 'use strict'; | ||
|
|
||
| function notFound(req, res) { | ||
| res.status(404); | ||
| res.statusMessage = 'Not Found'; | ||
| res.end(); | ||
| } | ||
|
|
There was a problem hiding this comment.
The isValid function returns true for empty objects. For {}, Object.values({}) returns [], and ![].length is !0 which is true. This means empty request bodies pass validation instead of returning 400 as required by the task. Consider checking if all values are non-empty: payload.every(v => !isEmpty(v)).
| 'use strict'; | ||
|
|
||
| const { Router } = require('express'); | ||
| const { | ||
| listExpenses, | ||
| getExpense, | ||
| createExpense, | ||
| handleUpdate, | ||
| deleteExpense, | ||
| } = require('../controllers/expenses.controller'); | ||
|
|
There was a problem hiding this comment.
The isValid function returns true when the payload IS empty. For Object.values({}) it returns [], then isEmpty([]) returns false (array is not null/undefined/empty-string), then the final return isEmpty(payload) returns false, but due to short-circuit with !payload.length, it returns true for empty. This inverted logic will cause createExpense to send 400 for valid expense data.
| getExpense, | ||
| createExpense, | ||
| handleUpdate, |
There was a problem hiding this comment.
The isEmpty function treats empty arrays [] as NOT empty (returns false). However, an empty array has no elements and should arguably be considered empty. This causes isValid([]) to return false when checking !payload.length since [].length === 0.
| /** Next assignable id. */ | ||
| let nextId = 1; | ||
|
|
There was a problem hiding this comment.
The isEmpty function doesn't handle empty arrays. When isEmpty([]) is called, it returns false because [] is not undefined, null, or ''. This causes isValid([]) to return false (since ![].length is true), making the validation reject empty arrays when it should accept them. Add || (Array.isArray(value) && value.length === 0) to properly identify empty arrays as 'empty'.
| nextId = 1; | ||
| } | ||
|
|
||
| function parseId(raw) { | ||
| const id = typeof raw === 'string' ? Number(raw) : raw; |
There was a problem hiding this comment.
The if (expenses) check treats empty arrays as falsy, returning 404. However, getExpenses({}) returns [] when no userId is provided, not null. This means GET /expenses without filters would incorrectly return 404 instead of 200 with []. Consider checking if (expenses === null) to only return 404 for the 'user not found' case.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation successfully meets all core requirements for the expense tracking app. Both Users and Expenses collections are fully implemented with all 5 endpoints each, proper error handling (404 for missing entities, 400 for invalid input), and correct in-memory storage that persists during runtime. The server architecture is correctly split between createServer.js (which creates and returns the Express app without calling listen()) and main.js (which starts the server on the configured PORT). All tests pass with no failures detected. As a minor note, you have some typos in function names like creatEexpenses and updatEexpenses - these work correctly but could be cleaned up for better readability.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.