Develop

[predator-bites]

No description provided.

[mateacademy-ai-mentor]

Review Summary

Hello! I'm requesting a few changes before this can be approved. You've built a solid application structure, which is great! However, there are several critical issues that conflict with the project's core requirements and cause incorrect behavior.

Here are the main points to address:

• Initial Data: In src/users.controller.js and src/expenses.controller.js, the data arrays must be initialized as empty (let users = []) as required by the task description.
• Server Start: The app.listen() call in src/createServer.js must be removed. The task specifies that the server is started by the test environment, not this file.
• User Deletion Logic: In src/users.routes.js, the DELETE /:id endpoint incorrectly sends a 204 status even when a user isn't found. You need to ensure the function exits after setting the 404 status, for example, with a return.
• Expense Filtering: The logic in GET /expenses in src/expenses.routes.js is flawed. The check for query parameters is always true, making the code to get all expenses unreachable. Also, a search that yields no results should return a 200 OK with an empty array, not a 404.

Please fix these issues, and your project will be ready for approval. Keep up the great work!

---

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

The condition if (searchParams) will always be true because new URLSearchParams() always returns an object, even for an empty query string. This makes the logic to get all expenses (lines 34-35) unreachable. You should check if any query parameters were actually provided, for example by checking the number of keys in req.query.

[mateacademy-ai-mentor]

There's a bug in how the userId is parsed. If the userId query parameter is not provided, searchParams.get('userId') is null, and +null evaluates to 0. This causes the code to incorrectly filter for expenses of user 0 instead of not filtering by user at all.

[mateacademy-ai-mentor]

Returning a 404 status code when a filtered search returns no results is not standard REST API practice. A successful search that finds no matching items should return a 200 OK status with an empty array []. A 404 Not Found should be used when the requested resource itself (like /expenses/some-invalid-id) doesn't exist.

[mateacademy-ai-mentor]

This if block is always true because searchParams is always an object, making the else block unreachable. The logic for handling requests with and without query parameters needs to be reconsidered. A simple check like Object.keys(req.query).length > 0 would be more appropriate for distinguishing between these cases.

[mateacademy-ai-mentor]

The task requirements explicitly state that you should not call app.listen() within this file. The server is started by the test runner or the main application file (main.js). Including this line will interfere with the automated tests and proper application startup.

[mateacademy-ai-mentor]

According to the task requirements, the data should be empty when the server starts. This array should be initialized as empty, like let users = [];.

[mateacademy-ai-mentor]

According to the task requirements, the data should be empty initially when the server starts. This array should be initialized as an empty array, like in the commented-out line below.

[mateacademy-ai-mentor]

There's a logic error in this DELETE handler. When deleteStatus is false (meaning the user was not found), you correctly set the status code to 404. However, the function continues to execute, and the status code is immediately overwritten to 204 on line 54. You need to ensure the function exits after handling the 404 case, for example, by adding a return statement inside the if block.

[mateacademy-ai-mentor]

Review Summary

I am requesting changes because there are still a few critical bugs in src/expenses.routes.js that will break required functionality and test cases. The GET /expenses query filtering branch never runs because URLSearchParams is used incorrectly (no .length property), and the request URL is parsed manually instead of using req.query, so filtered queries (e.g. by userId, category, from, to) won’t work at all. Also in that same route, parsing userId with +searchParams.get('userId') || null mistakenly turns a valid id of 0 into null, which conflicts with the controller’s expectation that userId === 0 is a valid user id. In the POST /expenses route, when the referenced user doesn’t exist, you currently return status 400, but the requirements specify returning 404 when an expected entity (like a user or expense) is missing.

The main fixes you need to make are:

• Replace the manual new URLSearchParams(req.url.slice(2)) logic with Express’ req.query, and check for filters with something like if (Object.keys(req.query).length > 0) { ... } (this will make the filtering branch reachable and safer).
• When deriving userId (and other params) from req.query, preserve 0 by checking presence explicitly, e.g. if ('userId' in req.query) { userId = Number(req.query.userId); } instead of using +value || null.
• In POST /expenses, when userController.getById(userId) returns nothing, respond with res.status(404) instead of 400 to match the “404 for missing entity” rule.

Everything else (users routes/controller, expenses controller, createServer) looks solid and aligned with the requirements; once you fix the expenses query parsing and the 404 status, your solution should meet the task expectations. Keep going—you’re very close, and these changes will make your expenses endpoints both correct and easier to maintain.

---

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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

1. Rate AI review example ↩

2. Re-request AI review example ↩

3. Add AI review comments example ↩

[mateacademy-ai-mentor]

Avoid manually slicing req.url and constructing URLSearchParams here. Prefer using Express-provided req.query which is already parsed and avoids fragile slicing logic. Using req.query will simplify this route and remove edge-case bugs. Reference: this file's query parsing on the next line.

[mateacademy-ai-mentor]

if (searchParams.length) { ... } is incorrect: URLSearchParams doesn't have a length property, so this condition will always be falsy and the filtering branch will never run. Use Object.keys(req.query).length, searchParams.toString() !== '', or, better, switch to req.query and check Object.keys(req.query).length instead.

[mateacademy-ai-mentor]

Parsing userId like +searchParams.get('userId') || null will convert a valid 0 to null (because 0 || null is null). Preserve zeros by checking presence explicitly, e.g. const userId = searchParams.has('userId') ? +searchParams.get('userId') : null or use if ('userId' in req.query) ... when switching to req.query. The controller expects userId === 0 to be valid.

[mateacademy-ai-mentor]

When the referenced user doesn't exist you currently respond with status 400. Per the project/tests requirements, missing expected entities should return 404 (not 400). Change this status to 404 for the user-not-found case in POST / (create expense).

[mateacademy-ai-mentor]

Small suggestion: after switching to req.query you can remove the whole new URLSearchParams(req.url.slice(2)) approach and directly derive userId, category, from, and to from req.query. This will fix presence checks, preserve falsy numeric ids like 0, and make the code much easier to read and test.

[mateacademy-ai-mentor]

Review Summary

I am approving your solution as an educational override so you can keep progressing while you refine a few remaining details.

You did a solid job implementing the in-memory controllers for both users and expenses, wiring up all required endpoints, returning 404s correctly when entities are missing, and structuring createServer so it mounts the routers and returns the Express app without calling listen.

There are, however, some important fixes to make next:

• In src/expenses.routes.js, the GET /expenses handler currently uses if (req.query) which is always truthy; this makes the filtered branch always run and the unfiltered getAll() path unreachable. Update it to something like if (Object.keys(req.query).length > 0) and, for userId, explicitly check key presence and convert with Number(...) so 0 is preserved as a valid id.
• Also in expenses.routes.js, consider applying the same explicit presence and type handling to from and to so they are consistently parsed and easier to reason about.
• In src/users.routes.js, both POST and PATCH endpoints call name?.trim() before checking the type; this can throw if name is not a string. Swap the order to if (typeof name !== 'string' || !name.trim()) { ... } so invalid names yield a 400 instead of an exception.

Overall, your structure and route/controller separation are strong; with these validation and query-handling fixes, your implementation will be much more robust and aligned with typical Express best practices. Keep going—you’re very close to a polished solution.

---

✨ Please rate this review in the Platform's notification center¹. I will be grateful for your feedback! ✨

Footnotes

1. Rate AI review example ↩

[mateacademy-ai-mentor]

if (req.query) is always truthy in Express (it will be an object even when empty). That makes the filtered branch execute for every request and prevents the fallback getAll() path from being used. Change this to an explicit presence check such as if (Object.keys(req.query).length > 0) { ... } so you only apply filters when query params exist.

[mateacademy-ai-mentor]

Parsing userId with req.query['userId'] || null mixes types and relies on truthiness. Explicitly check for the presence of the key and convert to Number to preserve 0 as a valid id, e.g. let userId = null; if ('userId' in req.query) { userId = Number(req.query.userId); }. This keeps types consistent and avoids edge-cases.

[mateacademy-ai-mentor]

Because the filtered branch currently always runs, the unfiltered fallback that calls expensesController.getAll() (below) is effectively unreachable. After fixing the presence check on req.query, the fallback will work as intended.

[mateacademy-ai-mentor]

Consider applying the same explicit-presence parsing approach to from and to if you rely on their presence vs absence. Right now they are left as strings or null, and although getByParams handles them, making presence and types explicit improves clarity and avoids subtle bugs.

[mateacademy-ai-mentor]

The POST validation calls name?.trim() before checking type. If name is a non-string (for example a number), calling .trim() may throw. Swap the checks and verify the type first, e.g. if (typeof name !== 'string' || !name.trim()) { ... } so you return 400 safely for invalid/missing name.

[mateacademy-ai-mentor]

The PATCH validation only checks !name?.trim() and does not verify name is a string. If name is present but not a string this may throw. Add an explicit type check like if (typeof name !== 'string' || !name.trim()) { ... } to ensure you return 400 rather than causing an exception.