-
Notifications
You must be signed in to change notification settings - Fork 44
Buchra Atkeh week3 Homework #33
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
base: master
Are you sure you want to change the base?
Conversation
| ' from products p join product_availability pa on(p.id=pa.prod_id)'+ | ||
| 'join suppliers s on (pa.supp_id=s.id) ;', (error, result) => { | ||
| res.json(result.rows); | ||
| }); |
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 could avoid repetition by concatenating the "where" clause to the main query, rather than having 2 completely separate strings.
| res.status(400).send("wrong request "+error) | ||
| } | ||
| }); | ||
| } |
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.
Your idea of creating an array up front to check if the product id exists in the product table is interesting. The model answers we have been given suggest either making a query to the database inside the function or relying on foreign key constraints. I think that relying on foreign key constraints is the most common approach - if they have been set up correctly then pool.query will return a foreign key constraint error, which you can then return in the response. But what you have done is very similar to an in-memory cache which Postgres and other databases can use. It's not a bad idea, but in a real life scenario, you would probably rely on improving the inbuilt database cache to improve performance rather than creating the in-memory array yourself.
| }); | ||
| }else | ||
| res.status(400).send("This id "+customerId +" is not existed ") | ||
| }); |
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.
See my previous comment about making a query to the database rather than using an in-memory array.
|
|
||
| app.listen(3000, function() { | ||
| console.log("Server is listening on port 3000. Ready to accept requests!"); | ||
| }); No newline at end of file |
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 have missed out the last but one task - to delete a customer.
Also, ideally you should handle the error case in each function and also return an appropriate status code for the successful responses and the error responses. You have used a 400 error in some cases - if the database has returned an error this is usually a 500 error, unless the error message indicates that the user has made a mistake, i.e. violating a foreign key constraint.
KarenPudner
left a comment
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.
This is generally very good - you have clearly understood how to build up queries and insert variables. Just a few comments to look at
No description provided.