Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

solution #306

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/Random.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
function generateRandomId() {
return Math.floor(10000000 + Math.random() * 90000000);
}

module.exports = {
generateRandomId,
};
122 changes: 122 additions & 0 deletions src/controller/expenses.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
const expensesService = require('../services/expenses.service');
const userService = require('../services/user.service');

const get = async (req, res) => {
const { userId, from, to, categories } = req.query;
const expenses = await expensesService.getAllExpenses();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!userId && !categories && (!from || !to)) {
return res.send(expenses);
}

let filteredExpenses = expenses;

if (userId) {
filteredExpenses = expensesService.filterExpensesById(
filteredExpenses,
userId,
);
}

if (categories) {
const normalizedCategories = expensesService.normalizeCategory(categories);

filteredExpenses = expensesService.filterExpensesByCategory(
filteredExpenses,
normalizedCategories,
);
}

if (from && to) {
const fromDate = new Date(from);
const toDate = new Date(to);

filteredExpenses = expensesService.filterExpensesByDate(
filteredExpenses,
fromDate,
toDate,
);
}

res.send(filteredExpenses);
};

const getOne = async (req, res) => {
const { id } = req.params;

const expense = await expensesService.getById(id);

if (!expense) {
res.sendStatus(404);

return;
}

res.send(expense);
};

const create = async (req, res) => {
const { userId, spentAt, title, amount, category, note } = req.body;

if (!userId || !spentAt || !title || !amount) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

res.sendStatus(400);

return;
}

const user = await userService.getById(userId);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (!user) {
res.sendStatus(400);

return;
}
Comment on lines +67 to +73

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 updatedExpense = await expensesService.create({
userId,
spentAt,
title,
amount,
category,
note,
});

res.status(201).send(updatedExpense);
};

const remove = async (req, res) => {
const { id } = req.params;

const expenses = await expensesService.getAllExpenses();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


if (expenses.length === updatedExpenses.length) {
return res.sendStatus(404);
}
Comment on lines +90 to +98

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


res.sendStatus(204);
};

const update = async (req, res) => {
const { id } = req.params;
const chosenExpense = await expensesService.getById(id);

if (!chosenExpense) {
return res.sendStatus(404);
}

const expense = await expensesService.updateExpense(chosenExpense, req, id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'updateExpense' function is called with 'req' and 'id' as arguments, but it's unclear what the function expects these arguments to be. It would be better to pass only the necessary data to the function, rather than the entire request object. This would make the code easier to understand and maintain.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


res.status(200).send(expense);
};

module.exports = {
get,
getOne,
create,
remove,
update,
};
95 changes: 95 additions & 0 deletions src/controller/user.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
const userService = require('../services/user.service');

const get = async (req, res) => {
const users = await userService.getAll();

if (!users) {
res.send(users);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

res.sendStatus(404);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are sending the users in the response and then immediately sending a 404 status code. This would result in an error because headers cannot be sent after the response is sent. You should send a 404 status code if no users are found.


return;
}

res.send(users);
};

const getOne = async (req, res) => {
const { id } = req.params;

const user = await userService.getById(+id);

if (!user) {
res.sendStatus(404);
Comment on lines +19 to +22

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the getOne function, when a user is not found, you're only sending a 404 status code. It would be helpful to also send a message to give more context about the error.


return;
Comment on lines +19 to +24

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are converting the id to a number using the unary plus operator. While this works, it could lead to unexpected results if the id is not a number. It would be safer to check if the id is a number before trying to convert it.

}

res.send(user);
};

const update = async (req, res) => {
const { id } = req.params;
const { name } = req.body;
const user = await userService.getById(+id);

if (!user) {
res.sendStatus(404);
Comment on lines +33 to +36

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the update function, when a user is not found, you're only sending a 404 status code. It would be helpful to also send a message to give more context about the error.


return;
Comment on lines +33 to +38

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, you are converting the id to a number using the unary plus operator. Better to check if the id is a number before trying to convert it.

}

if (typeof name !== 'string') {
res.sendStatus(422);

return;
Comment on lines +41 to +44

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you are checking if the name is a string, which is good. However, you should also check if the string is empty. An empty string is still a string, but it's not a valid name.

}

await userService.updateUser({ id, name });

const updatedUser = await userService.getById(+id);

if (!updatedUser) {
res.sendStatus(404);
Comment on lines +49 to +52

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the update function, after updating the user, you're checking if the updatedUser is not found. This is unnecessary because if the user was not found, you would have already sent a 404 status code.


return;
Comment on lines +49 to +54

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, you are converting the id to a number using the unary plus operator. Better to check if the id is a number before trying to convert it.

}

res.send(updatedUser);
};

const create = async (req, res) => {
const { name } = req.body;

if (!name) {
res.sendStatus(400);
Comment on lines +63 to +64

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the create function, when a name is not provided, you're only sending a 400 status code. It would be helpful to also send a message to give more context about the error.


return;
Comment on lines +63 to +66

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you are checking if the name exists, which is good. However, you should also check if the name is a string and if it's not empty. An empty string is still a string, but it's not a valid name.

}

const user = await userService.createUser(name);

res.statusCode = 201;
res.send(user);
};

const remove = async (req, res) => {
const { id } = req.params;
const userId = +id;

if (await userService.getById(userId)) {
await userService.deleteUser(userId);
res.sendStatus(204);

Comment on lines +79 to +82

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the remove function, when a user is found, you're sending a 204 status code. This is correct, but it would be more informative to also send a message indicating that the user was deleted successfully.

return;
Comment on lines +76 to +83

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above, you are converting the id to a number using the unary plus operator. Better to check if the id is a number before trying to convert it.

}

res.sendStatus(404);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the remove function, when a user is not found, you're only sending a 404 status code. It would be helpful to also send a message to give more context about the error.

};

module.exports = {
get,
getOne,
update,
create,
remove,
};
17 changes: 16 additions & 1 deletion src/createServer.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,22 @@
'use strict';

const express = require('express');
const cors = require('cors');

const { router: userRouter } = require('./routes/users.route');
const { router: expensesRoute } = require('./routes/expenses.route');

const createServer = () => {
// your code goes here
const app = express();

app.use(cors());

app.use(express.json());

app.use('/users', userRouter);
app.use('/expenses', expensesRoute);

return app;
};

module.exports = {
Expand Down
7 changes: 1 addition & 6 deletions src/db.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,13 @@ const {
POSTGRES_DB,
} = process.env;

/*
All credentials setted to default values (exsept password - it is exapmle)
replace if needed with your own
*/

const sequelize = new Sequelize({

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are creating a new Sequelize instance directly in this file. This is not a good practice as it makes the code less modular and more difficult to test. It would be better to do this in a separate factory function, which you then import in this file.

database: POSTGRES_DB || 'postgres',
username: POSTGRES_USER || 'postgres',
host: POSTGRES_HOST || 'localhost',
dialect: 'postgres',
port: POSTGRES_PORT || 5432,
password: POSTGRES_PASSWORD || '123',
password: POSTGRES_PASSWORD || '123123',

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hardcoding default values for your database connection parameters (like 'postgres', 'localhost', '123123') directly in the code is not a good practice. It can lead to security issues and makes the code less flexible. It's better to handle these defaults in your environment or configuration setup. You can use a package like dotenv to manage your environment variables in a .env file.

});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are using the '||' operator to provide default values for your environment variables. This is not a good practice as it can lead to unexpected behavior if the environment variables are not set correctly. It would be better to validate the environment variables and throw an error if they are not set correctly.


module.exports = {
Expand Down
36 changes: 35 additions & 1 deletion src/models/Expense.model.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,43 @@
'use strict';

const { sequelize } = require('../db.js');
const { DataTypes } = require('sequelize');

const Expense = sequelize.define(
// your code goes here
'Expense',
{
id: {
type: DataTypes.INTEGER,
primaryKey: true,
},
userId: {
type: DataTypes.INTEGER,
allowNull: false,
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'userId' field suggests a relationship with a 'User' model. This relationship should be defined using Sequelize's association methods (like 'belongsTo', 'hasOne', 'hasMany', etc.) in the model files. This allows Sequelize to automatically handle the foreign key constraints and provides various utility methods to work with associated data.

spentAt: {
type: DataTypes.TEXT,
allowNull: false,
},
Comment on lines +17 to +20

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The 'spentAt' field is currently defined as a TEXT type. This might not be the best choice if you're planning to store dates, as it doesn't allow for date-related operations and comparisons. Consider changing the type to DATE or DATETIME, depending on your needs.

title: {
type: DataTypes.TEXT,
allowNull: false,
},
amount: {
type: DataTypes.INTEGER,
allowNull: false,
},
category: {
type: DataTypes.TEXT,
},
note: {
type: DataTypes.TEXT,
},
},
{
tableName: 'Expense',
updatedAt: false,
createdAt: false,
},
);

module.exports = {
Expand Down
19 changes: 18 additions & 1 deletion src/models/User.model.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,26 @@
'use strict';

const { sequelize } = require('../db.js');
const { DataTypes } = require('sequelize');

const User = sequelize.define(
// your code goes here
'User',
{
id: {
type: DataTypes.INTEGER,
primaryKey: true,
allowNull: false,
autoIncrement: true,
},
name: {
type: DataTypes.TEXT,
allowNull: false,
},
},
{
updatedAt: false,
createdAt: false,
},
);

module.exports = {
Expand Down
17 changes: 17 additions & 0 deletions src/routes/expenses.route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
const expensesController = require('../controller/expenses.controller');
const express = require('express');
const router = express.Router();

router.get('/', expensesController.get);

router.get('/:id', expensesController.getOne);

router.post('/', expensesController.create);

router.delete('/:id', expensesController.remove);

router.patch('/:id', express.json(), expensesController.update);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using express.json() middleware in a specific route, it's better to use it in your main server file where you initialize your express application. This way, it will be applied to all routes and you won't forget to add it when necessary. It's a common practice to parse incoming requests with a middleware, and you usually want to do it for all routes.


module.exports = {
router,
};
19 changes: 19 additions & 0 deletions src/routes/users.route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
const express = require('express');

const userController = require('../controller/user.controller');

const router = express.Router();

router.get('/', userController.get);

router.get('/:id', userController.getOne);

router.patch('/:id', userController.update);

router.post('/', userController.create);

router.delete('/:id', userController.remove);

module.exports = {
router,
};
Loading
Loading