solution #306
solution #306
Conversation
There was a problem hiding this comment.
Great work so far! 🎉 However, there are a few issues that need to be addressed before we can merge this PR. Most importantly, please make sure to validate and sanitize the input before updating the expense and to let the database handle the generation of IDs. Additionally, please ensure the relationships are properly set in the Sequelize model definitions. Keep up the good work, you're almost there! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| const get = async (req, res) => { | ||
| const { userId, from, to, categories } = req.query; | ||
| const expenses = await expensesService.getAllExpenses(); |
There was a problem hiding this comment.
This line retrieves all expenses without considering the user's query. This could be a performance issue if the database contains a large number of expense records. It would be more efficient to apply the filters in the database query, rather than retrieving all expenses and then filtering them in the application code.
| const create = async (req, res) => { | ||
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
|
|
||
| if (!userId || !spentAt || !title || !amount) { |
There was a problem hiding this comment.
You are checking for the presence of certain fields in the request body but not checking their types or values. This could lead to unexpected errors or incorrect data being stored in the database. It would be a good idea to add some validation to ensure that the fields are of the expected types and meet any other constraints (e.g., 'amount' should be a positive number).
| return; | ||
| } | ||
|
|
||
| const user = await userService.getById(userId); |
There was a problem hiding this comment.
This line queries the database for the user associated with the provided userId. However, if the user does not exist, the code will send a 400 status code. It would be more appropriate to send a 404 status code in this case, as the user resource was not found.
| const remove = async (req, res) => { | ||
| const { id } = req.params; | ||
|
|
||
| const expenses = await expensesService.getAllExpenses(); |
There was a problem hiding this comment.
This line retrieves all expenses from the database, but the returned data is not used anywhere in the function. It would be better to remove this unnecessary database query to improve performance.
|
|
||
| await expensesService.removeExpense(id); | ||
|
|
||
| const updatedExpenses = await expensesService.getAllExpenses(); |
There was a problem hiding this comment.
This line retrieves all expenses again after one has been removed. This could be a performance issue if the database contains a large number of expense records. It would be more efficient to check the result of the 'removeExpense' function to determine whether the removal was successful.
| }); | ||
| }; | ||
|
|
||
| const updateExpense = async (exp, req, id) => { |
There was a problem hiding this comment.
In the 'updateExpense' function, you are directly spreading the request body into the 'updatedFields'. This could lead to potential security risks as it allows the client to update any field in the expense. It would be better to validate and sanitize the input before updating the expense.
| where: { id }, | ||
| }); | ||
|
|
||
| return getById(id); |
There was a problem hiding this comment.
After updating the expense, you are fetching it again from the database. This is unnecessary and can be avoided by using the 'returning: true' option in the 'update' method, which will return the updated expense.
| return User.findAll(); | ||
| }; | ||
|
|
||
| const getById = async (id) => { |
There was a problem hiding this comment.
In the 'getById' function, you are using the Unary Plus (+) operator to convert the 'id' parameter to a number. While this is not necessarily an issue, it could lead to unexpected results if a non-numeric string is passed as 'id'. This could lead to a bug in your application. Consider adding error handling to check if the 'id' is a valid number before using it.
| }; | ||
|
|
||
| const createUser = async (name) => { | ||
| const id = generateRandomId(); |
There was a problem hiding this comment.
In the 'createUser' function, you are generating a random 'id' for the user. If your database is set up to auto-increment the 'id' field, this could potentially lead to conflicts. Consider letting the database handle the 'id' generation, unless there is a specific need for this approach.
| }; | ||
|
|
||
| const deleteUser = async (id) => { | ||
| await User.destroy({ |
There was a problem hiding this comment.
In the 'deleteUser' function, there is no error handling or feedback to indicate whether the deletion was successful or not. Consider returning the result of the 'User.destroy' function to give feedback to the caller of this function.
There was a problem hiding this comment.
Great job on your code! The structure is well-organized, and the logic is clear. There are some minor issues and suggestions for improvement, which I've noted in the comments. Please go through them and try to implement the changes where possible. Keep in mind that these are not critical issues and your code is generally good. I believe you can make it even better with a little more attention to detail. Keep up the good work! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| const get = async (req, res) => { | ||
| const { userId, from, to, categories } = req.query; | ||
| const expenses = await expensesService.getAllExpenses(); |
There was a problem hiding this comment.
Performance issue: You are fetching all expenses from the database without any filters. This could be a performance bottleneck when the number of expenses grows. Try to apply filters directly in the database query.
| const user = await userService.getById(userId); | ||
|
|
||
| if (!user) { | ||
| res.sendStatus(400); | ||
|
|
||
| return; | ||
| } |
There was a problem hiding this comment.
Code quality issue: You are fetching the user from the database after validating the request body. It would be more efficient to first check if the user exists before validating the request body. This way, you can avoid unnecessary validation if the user does not exist.
| const expenses = await expensesService.getAllExpenses(); | ||
|
|
||
| await expensesService.removeExpense(id); | ||
|
|
||
| const updatedExpenses = await expensesService.getAllExpenses(); | ||
|
|
||
| if (expenses.length === updatedExpenses.length) { | ||
| return res.sendStatus(404); | ||
| } |
There was a problem hiding this comment.
Performance issue: You are fetching all expenses twice when removing an expense. This is unnecessary and could lead to performance issues. Instead, you can check the result of the removeExpense function to determine if the removal was successful.
| return res.sendStatus(404); | ||
| } | ||
|
|
||
| const expense = await expensesService.updateExpense(chosenExpense, req, id); |
There was a problem hiding this comment.
Bug: You are passing the entire request object to the updateExpense function. This could lead to unexpected behavior if the request object contains properties that are not expected by the updateExpense function. Instead, pass only the necessary data to the function.
| const users = await userService.getAll(); | ||
|
|
||
| if (!users) { | ||
| res.send(users); |
There was a problem hiding this comment.
In the get function, you're sending the users object even if it's falsy or empty. This could lead to unexpected behaviours. Instead, you should send a 404 status code directly if users is empty.
| return User.findAll(); | ||
| }; | ||
|
|
||
| const getById = async (id) => { |
There was a problem hiding this comment.
It's a good practice to handle possible errors that might occur when searching for a user by ID. For instance, if no user with the provided ID exists, you should return a meaningful error message to the client.
| return User.findByPk(+id); | ||
| }; | ||
|
|
||
| const updateUser = async ({ id, name }) => { |
There was a problem hiding this comment.
In the updateUser function, you are not checking if the user with the provided ID exists before attempting to update. It's recommended to first check if the user exists and then proceed with the update. Also, consider returning the updated user object to the client.
| await User.update({ name }, { where: { id } }); | ||
| }; | ||
|
|
||
| const createUser = async (name) => { |
There was a problem hiding this comment.
In the createUser function, you are generating a random ID for the user. However, it's better to let the database handle the generation of unique IDs to ensure data integrity and avoid possible collisions.
| return User.create({ id, name }); | ||
| }; | ||
|
|
||
| const deleteUser = async (id) => { |
There was a problem hiding this comment.
In the deleteUser function, similar to the updateUser function, it's recommended to first check if the user with the provided ID exists before attempting to delete. If the user doesn't exist, return a meaningful error message to the client.
| User.sync({ force: true }); | ||
| Expense.sync({ force: true }); |
There was a problem hiding this comment.
The sync() method with the force: true option will drop the existing table and recreate it. This can lead to data loss in a production environment. If you want to keep the existing data, you can use alter: true which will only add new fields to the table. However, be aware that it won't remove old fields or change the data type of existing fields.
No description provided.