Mickey Haile -Node-Coursework-Week2#269
Mickey Haile -Node-Coursework-Week2#269mickeyhaile2 wants to merge 1 commit intoCodeYourFuture:masterfrom
Conversation
JDysiewicz
left a comment
JDysiewicz
left a comment
There was a problem hiding this comment.
Routes look good and functional, some small stylistic changes and some error handling would be nice.
| @@ -1,5 +1,6 @@ | |||
| const express = require("express"); | |||
| const cors = require("cors"); | |||
| const bodyParser = require("body-parser"); | |||
There was a problem hiding this comment.
Nowadays you can just use express.json() (turns out the express package now includes bodyParser by default, and so express.json() is the same thing as bodyParser.json in middleware - this was news to me 😅 )
| }); | ||
|
|
||
| app.get("/messages/search", (req, res) => { | ||
| const { term } = req.query; |
There was a problem hiding this comment.
Always nice to try and cover off edge cases and handle them; e.g. what if term was an empty string, or undefined? How would you handle that?
|
|
||
| app.get("/messages/:id", function (req, res) { | ||
| const id = req.params.id; | ||
| messages = messages.filter((message) => message.id === Number(id)); |
There was a problem hiding this comment.
To convert to an integer, use parseInt rather than the Number constructor - in this case it'd work out the same, but we tend to prefer parseInt as you can pass in a second parameter to specify the numeral system to parse the string as (e.g. hexidecimal, decimal, oct, etc)
| res.json(messages); | ||
| }); | ||
|
|
||
| app.listen(process.env.PORT); |
There was a problem hiding this comment.
Would add a default value for this in case this env var is not set (e.g. const port = process.env.PORT ?? 3000)
2 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?