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

London10_Olha-Danylevska_Module-NodeJS-week2 #285

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

OlhaDanylevska
Copy link

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?

Copy link

@GergelyKI GergelyKI left a comment

Choose a reason for hiding this comment

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

In overall it looks good. I left a few comments on the file server.js.

console.log(console.log(console.error()))
throw new Error("400")
} else {
newMessage.timeSent = `${hours}:${minutes}:${seconds}`

Choose a reason for hiding this comment

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

I think if you have the hours, minutes and seconds defined outside of this function (and the new date as well), then after the server started to run, it will add the exact same time to all posted messages. To capture the actual creation time, the new date have to be created within the function, so it always reflects the time when the function is called.

Copy link
Author

Choose a reason for hiding this comment

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

@GergelyKI Thank you very much, I've fixed it. I'm not sure why bat it shows time 1 hour less than UK TIME.

server.js Outdated
throw new Error("400")
} else {
newMessage.timeSent = `${hours}:${minutes}:${seconds}`
newMessage.id = messages.length

Choose a reason for hiding this comment

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

This way of defining ids might not be the best because the length is not always growing because you can deleted messages as well, and you can end up having messages with the same ids.

e.g. you have to following array: [{id: 0}, {id: 1}]. Then you delete the first one you're left with the array: [{id: 1}].

Now you want to add another one. The length is 1 , then the next id is also 1, and your array will be [{id: 1}, {id: 1}].

Then the ids become ambiguous. Now if you want to delete again one with id: 1 or find one with id: 1, it will return the first one, but maybe you wanted the second one.

(You can use some sort of id generator library like uuid, that will ensure you're ids are always different).

I think the takeaway doesn't have to be about the ids themselves, just that it's always worth keeping in mind the length of the array can change if you come across any indexing problem.)

Copy link
Author

Choose a reason for hiding this comment

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

@GergelyKI thank you very much for your comment, Yes I'll definitely to think about how to fix it) true, not the best way

let newMessage = request.body
if (newMessage.from === "" || newMessage.text === "") {
console.log(console.log(console.error()))
throw new Error("400")

Choose a reason for hiding this comment

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

is this error going to be displayed as the response when I do a POST to /messages without a body.from or body.text?

console.log(`listening on PORT ${process.env.PORT}...`);
});

// functions

Choose a reason for hiding this comment

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

Good use of functions for each of the endpoints.

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.

3 participants