-
-
Notifications
You must be signed in to change notification settings - Fork 306
nw5-Shimen-Afshar-Node-Week3 #200
base: master
Are you sure you want to change the base?
Conversation
|
|
||
| app.get("/bookings/:id", (request, response) => { | ||
| const id = Number(request.params.id); | ||
| const findId = bookings.find((bookings) => bookings.id === id); |
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.
the find method works well here, nice job
|
|
||
| app.post("/bookings", (request, response) => { | ||
| if ( | ||
| request.body.firstName === "" || |
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 always do some destructuring here to avoid having to type request.body, for example:
const {body} = request
| request.body.email === "" || | ||
| request.body.surname == "" || | ||
| request.body.title === "" || | ||
| request.body.checkInDate === "" || | ||
| request.body.checkOutDate === "" || | ||
| request.body.roomId === null | ||
| ) { | ||
| response.status(400).json({ message: "please fill all the fields" }); | ||
| return; |
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 works & 400 makes sense. You could have a think about creating a function to deal with the above (for example the function could take an array of required fields & the request body and return true/false depending on if all required fields are present )
| roomId: request.body.roomId, | ||
| checkInDate: request.body.checkInDate, | ||
| checkOutDate: request.body.checkOutDate, | ||
| }; |
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.
new bookings are posted successfully, well done
| if (requestId < 0) { | ||
| return response.status(404).json({ msg: "message not found" }); | ||
|
|
||
| } |
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 isn't working as expected at the moment - if I put in an id that doesn't exist (that is larger or equal to 0) I don't get the 404 - have a think what array method could be used here to check if the id exists in the bookings array.
Also really good that you're filtering the bookings so you end up with an array without the given id, but at the moment you're just sending the new array back in the response, remember to actually update the bookings array :)
Nomes27
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.
just a quick note on node_modules, make sure you add a .gitignore file and add node_modules to this, so that they don't get committed :)
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?