-
-
Notifications
You must be signed in to change notification settings - Fork 447
London10_Olha-Danylevska_Module-NodeJS-week2 #285
base: master
Are you sure you want to change the base?
Conversation
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.
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}` |
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.
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.
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.
@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 |
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 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.)
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.
@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") |
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.
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 |
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.
Good use of functions for each of the endpoints.
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 repositoryYour Details
Homework Details
Notes
What did you find easy?
What did you find hard?
What do you still not understand?
Any other notes?