London10_Olha-Danylevska_Module-NodeJS-week2#285
London10_Olha-Danylevska_Module-NodeJS-week2#285OlhaDanylevska wants to merge 18 commits intoCodeYourFuture:masterfrom
Conversation
GergelyKI
left a comment
GergelyKI
left a comment
There was a problem hiding this comment.
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.
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.
@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.
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.
@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.
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.
Good use of functions for each of the endpoints.
3 participants
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?