Osman Elsahib-Database-Week-3 #36
Conversation
| } | ||
| ); | ||
| } | ||
| }); |
There was a problem hiding this comment.
You could avoid repetition by concatenating the "where" clause to the main query, rather than having 2 completely separate strings.
| res.status(500).json(serverError); | ||
| } else { | ||
| res | ||
| .status(201) |
There was a problem hiding this comment.
Good use of this HTTP status - 200 would have been fine, but 201 is more specific.
| } else { | ||
| res.status(400).json("The price needs to be larger than 0"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
There are a lot of nested if statements here, which makes it less readable. You could reduce this if you reversed the conditions of some of the if clauses and added returns, i.e.
if (price < 0) {
res.status(400).json("The price needs to be larger than 0");
return;
}
However there is a school of thought that having multiple returns in a function is a bad thing. So your code isn't wrong, but it's good to start thinking about different ways of organising your code and which you prefer and why.
You could also avoid doing the queries to check that the product and supplier exist and instead rely on the foreign key constraints being set up correctly. The database should return an error and you can return that error, or a more user friendly version to the client. Again, what you've done isn't wrong, but it's good to be aware of different options.
| } | ||
| }); | ||
| } else { | ||
| res.status(404).json("Can't find a product with this ID"); |
There was a problem hiding this comment.
I don't think that 404 is the correct status code, as the client isn't trying to fetch something. I've googled this, and 409 seems like the best response to me, but there are other opinions on this, so feel free to google and make up your own mind! Your error message is good though.
| }); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The model answer we have been given assumes that the user provides the order reference, but I can see why you have assumed the user would not know this. You have come up with a good solution to this, and referenced where you found the code, which is good practice for substantial pieces of copied code..
| } | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Good use of different http status code here, to distinguish between user error and server error.
| throw error; | ||
| } else if (result.rows.length > 0) { | ||
| res | ||
| .status(200) |
There was a problem hiding this comment.
Maybe a 409 status here? See the previous comment.
| let id = parseInt(req.params.customerID); | ||
| db.query("SELECT * FROM customers where id = $1", [id], (error, result) => { | ||
| if (error) { | ||
| console.log("pg error code:", error.code); |
There was a problem hiding this comment.
Yes, it is a good idea to log any database errors.
| ); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
You haven't done the endpoint for getting a customer by ID - I'm assuming you missed this, because you clearly have the knowledge to do it.
KarenPudner
left a comment
KarenPudner
left a comment
There was a problem hiding this comment.
This is excellent work, some comments to look at, but some of these are just highlighting different ways of doing things, rather than corrections. You've clearly understood what you have learnt and applied it well.
2 participants
Hello Mentors,
Please review my work for week 3.
Many Thanks,