Develop #307
Develop #307
Conversation
There was a problem hiding this comment.
Great job on the code! You've followed good practices for a Node.js application. There are some minor improvements suggested in the comments, but they do not significantly impact the functionality or security of your code. Keep up the good work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/controllers/expenseController.js
Outdated
| const user = await userService.getUserById(userId); | ||
|
|
||
| if (!user) { | ||
| res.sendStatus(400); |
There was a problem hiding this comment.
In the create function, you are sending a 400 status code if the user is not found. However, 400 Bad Request is typically used when the server could not understand the request due to invalid syntax. In this case, it would be more appropriate to send a 404 Not Found status code, as the user does not exist.
src/controllers/expenseController.js
Outdated
| const expense = await expensesService.getExpenseById(id); | ||
|
|
||
| if (!expense) { | ||
| res.sendStatus(404); |
There was a problem hiding this comment.
In the update function, you are sending a 404 status code if the expense is not found. This is correct, but you should also consider handling the case where the update operation fails for reasons other than the expense not being found. For example, if the update operation throws an error, you could catch that error and send a 500 Internal Server Error status code.
| const { id } = req.params; | ||
| const expense = await expensesService.getExpenseById(id); |
There was a problem hiding this comment.
In the getOne function, you are fetching the expense and then checking if it exists. If it doesn't exist, you send a 404 status code. This is correct, but you could improve the code by handling the case where the getExpenseById operation throws an error. For example, if the operation throws an error, you could catch that error and send a 500 Internal Server Error status code.
src/controllers/expenseController.js
Outdated
| const { userId, spentAt, title, amount, category, note } = req.body; | ||
| const user = await userService.getUserById(userId); |
There was a problem hiding this comment.
In the create function, you are fetching the user and then checking if it exists. If it doesn't exist, you send a 400 status code. This is correct, but you could improve the code by handling the case where the getUserById operation throws an error. For example, if the operation throws an error, you could catch that error and send a 500 Internal Server Error status code.
| return; | ||
| } | ||
|
|
||
| if (typeof name !== 'string') { |
There was a problem hiding this comment.
It seems like you're checking the type of the 'name' after trying to update the user. This could lead to problems if the name is not a string. You should move this check before calling the 'updateUser' method.
| @@ -26,7 +26,7 @@ const sequelize = new Sequelize({ | |||
| host: POSTGRES_HOST || 'localhost', | |||
| dialect: 'postgres', | |||
| port: POSTGRES_PORT || 5432, | |||
| password: POSTGRES_PASSWORD || '123', | |||
| password: POSTGRES_PASSWORD || '1234qwer', | |||
| }); | |||
There was a problem hiding this comment.
It's not a good practice to hardcode sensitive data like database password. Even though you've used it as a default value, it might pose a security risk. Consider using a more secure way to handle sensitive data. For example, you could throw an error if the password is not provided in the environment variables.
| const getAllExpenses = async (userId, categories, from, to) => { | ||
| const whereClause = {}; | ||
|
|
||
| if (userId) { | ||
| whereClause.userId = userId; | ||
| } | ||
|
|
||
| if (categories && categories.length > 0) { | ||
| whereClause.category = categories; | ||
| } | ||
|
|
||
| if (from && to) { | ||
| whereClause.spentAt = { [Op.between]: [from, to] }; | ||
| } else if (from) { | ||
| whereClause.spentAt = { [Op.gte]: from }; | ||
| } else if (to) { | ||
| whereClause.spentAt = { [Op.lte]: to }; | ||
| } | ||
|
|
||
| return Expense.findAll({ | ||
| where: whereClause, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This function could benefit from some error handling. Consider adding a try/catch block to handle any potential errors that might arise when interacting with the database. This will help to ensure that your application is robust and can handle unexpected situations gracefully.
| const createExpense = async ( | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| note, | ||
| ) => { | ||
| return Expense.create({ | ||
| userId, | ||
| spentAt, | ||
| title, | ||
| amount, | ||
| category, | ||
| note, | ||
| }); | ||
| }; |
There was a problem hiding this comment.
This function could also benefit from some error handling. Consider adding a try/catch block here as well.
| const updateExpense = async (id, data) => { | ||
| await Expense.update({ ...data }, { where: { id } }); | ||
|
|
||
| return Expense.findByPk(id); | ||
| }; |
There was a problem hiding this comment.
In this function, you are updating an expense and then fetching it again. To optimize it, you can use the returning: true option in the update method, which will return the updated expense. This way, you don't need to make an extra call to the database.
| const deleteExpense = async (id) => { | ||
| return Expense.destroy({ where: { id } }); | ||
| }; |
There was a problem hiding this comment.
In this function, you are deleting an expense but not checking if the expense exists before deleting it. It would be good to first check if the expense exists, and if it doesn't, throw an error. This will prevent potential issues where a non-existent expense is attempted to be deleted, which might cause unexpected behavior in your application.
No description provided.