Skip to content

James C PR#103

Open
JDC-horizons wants to merge 3 commits intoboolean-uk:mainfrom
JDC-horizons:main
Open

James C PR#103
JDC-horizons wants to merge 3 commits intoboolean-uk:mainfrom
JDC-horizons:main

Conversation

@JDC-horizons
Copy link

unfinished

src/server.js Outdated

module.exports = app
app.post("/contacts", function (req, res) {
const postContact = req.body;
Copy link

Choose a reason for hiding this comment

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

it's better practice to extract out the variables, to avoid accepting any input that may come from the POST body:

const { title, content } = req.body

If for example req.body = {title, content, someUnknownProperty} with the code in my example, the someUnknownProperty is ignored, but with your code it would be included.

module.exports = app
app.post("/contacts", function (req, res) {
const postContact = req.body;
const currentHighId = contacts.reduce((max, obj) => {
Copy link

Choose a reason for hiding this comment

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

nice, you could make this a generic function in a utils.js file :

function getNextId(items) {
   return items.reduce(...) + 1
}

and then call your function when needed, makes the code tidier

});

app.get("/contacts/:id", function (req, res) {
const toFind = parseInt(req.params.id, 10);
Copy link

Choose a reason for hiding this comment

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

the name of this var is not great, it'd be better to use targetId

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants