Skip to content
This repository was archived by the owner on Aug 17, 2024. It is now read-only.

Conversation

@Adibab
Copy link

@Adibab Adibab commented Jan 21, 2023

Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in HOW_TO_MARK.md in the root of this repository

Your Details

  • Your Name:
  • Your City:
  • Your Slack Name:

Homework Details

  • Module:
  • Week:

Notes

  • What did you find easy?

  • What did you find hard?

  • What do you still not understand?

  • Any other notes?

Comment on lines +31 to +42
const foundId = bookings.some(
(eachbooking) => eachbooking.id === parseInt(request.params.id)
);
console.log(foundId);
if (foundId) {
response
.status(200)
.json(
bookings.filter(
(eachmessage) => eachmessage.id === parseInt(request.params.id)
)
);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.some works, but then you're having to use another array method to get the message further down, you could think about combining these and using something like .find: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/find

Comment on lines +44 to +48
response
.status(404)
.json({
message: `No bookings with the id of ${request.params.id} is found`,
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice, this works well for if the id is not found

// to create bookings & level :02


app.post("/createNewBookings", (request, response) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to use the same route you already have - for example should be able to POST and GET from /bookings rather than creating a new route

// console.log("hello hahha")
// response.send(request.body)
let createNewBookings = {
id: uuid.v4(),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good that you're exploring what packages are out there to help with this!

});
}

bookings.push(createNewBookings);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your new booking creation works well, nice job. You could extract the logic above into a function (for example passing in an array of required fields and the request body, returning true/false depending on whether all required fields are present)


// Delete a booking, specified by an ID

app.delete("/bookings/delete/:id", (request, response) => {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to include the delete verb within the endpoint (as we can already get this information from the method) , so it should just be app.delete("/bookings/:id" - this way it follows the same pattern as the GET endpoint app.get("/bookings/:id
this article goes over some REST api best practises: https://stackoverflow.blog/2020/03/02/best-practices-for-rest-api-design/

Comment on lines +97 to +98
bookings: bookings.filter(
(eachmessage) => eachmessage.id !== parseInt(request.params.id)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job - filter works well here to remove the deleted item, but remember to update your bookings array so that on subsequent get requests the item is removed (at the moment the updated array is just getting sent back in the response body)

Copy link

@Nomes27 Nomes27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remember to add node_modules to your .gitignore at the start, noticed you've added them to your .gitignore now which is good, but they've already been committed - if you do some googling there's a way to remove them once committed :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants