London9-Mohamed Abdi-Node-Coursework-Week3 #242
Conversation
There was a problem hiding this comment.
Overall looks good. A couple points are to give verbose error messages to sent to the front-end, and also you're missing a .gitignore file which is why this PR has included the node_modules folder. We never want to commit this, because it is generated from the package.json file anyways and adds thousands of unnecessary files to the PR.
| }); | ||
|
|
||
| app.get("/booking/:id", function (req, res) { | ||
| const bookingId = +req.params.id; |
There was a problem hiding this comment.
This is a nice way to convert to an int when you know the incoming value will resemble an int, however when you can't be sure it's nice to have some error checking here (e.g. what if the user went to /booking/abc?)
| const bookingId = +req.params.id; | ||
| const booking = bookings.find((booking) => booking.id === bookingId); | ||
| if (!booking) { | ||
| res.status(404).send("404 error"); |
There was a problem hiding this comment.
Nice use of status codes here, however you could be more descriptive with the error you send back. This error will be sent to the front-end, so something like res.status(404).json({"message": "Invalid booking id ${bookingId}"}) might be nicer
|
|
||
| app.post("/booking", function (req, res) { | ||
| const createBooking = req.body; | ||
| createBooking.id = bookings.length + 1; |
There was a problem hiding this comment.
This will work currently, however you can't rely on the length of an array to give you a unique identifier in real applications; for instance, what if a booking is deleted then recreated? You now have bookingId = 2 referring to different bookings at different times (therefore it is not a good identifier). Also if multiple bookings come in at the same time, you could end up with multiple bookingId = 2 as they will all see the current length of the array could be 1 or something. A uuid is what you're looking for here.
| const valid = | ||
| !!title && | ||
| !!firstName && | ||
| !!surname && | ||
| !!email && | ||
| (roomId === 0 || !!roomId) && | ||
| !!checkInDate && | ||
| !!checkOutDate; | ||
| if (valid) { | ||
| bookings.push(createBooking); | ||
| res.json(bookings); | ||
| } else { | ||
| res.status(400).send("Missing Information"); |
There was a problem hiding this comment.
nice error checking, I like it. You could be more informative with what information is missing though in the response.
Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in
HOW_TO_MARK.mdin the root of this repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?
@JDysiewicz