-
Notifications
You must be signed in to change notification settings - Fork 449
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 #260
base: master
Are you sure you want to change the base?
solution #260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job.
I left some minor comments for fix.
Thanks)
const { userId, categories, from, to } = req.query; | ||
const all = await expensesService.getAll(+userId, categories, from, to); | ||
|
||
return res.status(200).send(all); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move All status to constants
const get = async (req, res) => { | ||
try { | ||
const { userId, categories, from, to } = req.query; | ||
const all = await expensesService.getAll(+userId, categories, from, to); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const all = await expensesService.getAll(+userId, categories, from, to); | |
const all = await expensesService.getAll(Number(userId), categories, from, to); |
as alternative
src/routes/expenses.route.js
Outdated
const expensesRoute = express.Router(); | ||
const controller = require('../controllers/expenses.controller'); | ||
|
||
expensesRoute.get('/', controller.get); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move all routes to constants
src/routes/users.route.js
Outdated
|
||
const usersRoute = express.Router(); | ||
|
||
usersRoute.get('/', controller.get); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, in this place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job.
All good, but you have duplication.
I left comment, but approved your solution.
Thanks)
const HTTP_OK = 200; | ||
const HTTP_BAD_REQUEST = 400; | ||
const HTTP_NOT_FOUND = 404; | ||
const HTTP_NO_CONTENT = 204; | ||
const HTTP_CREATED = 201; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move all constants in separate file ( because you have duplication)
const HTTP_OK = 200; | ||
const HTTP_BAD_REQUEST = 400; | ||
const HTTP_NOT_FOUND = 404; | ||
const HTTP_NO_CONTENT = 204; | ||
const HTTP_CREATED = 201; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move all constants in separate file ( because you have duplication)
No description provided.