Skip to content

J.Sorge HW2 - Express#26

Open
Jiansorge wants to merge 3 commits into
pce-uw-jscript400:masterfrom
Jiansorge:master
Open

J.Sorge HW2 - Express#26
Jiansorge wants to merge 3 commits into
pce-uw-jscript400:masterfrom
Jiansorge:master

Conversation

@Jiansorge
Copy link
Copy Markdown

Complete.

Curious on best method to approach IE with ES6 Array.prototype.filter(), .findIndex(), etc.

Copy link
Copy Markdown
Contributor

@bwreid bwreid left a comment

Choose a reason for hiding this comment

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

Well done! Just a few comments for you.

Comment thread app.js
}
return result;
};
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Both of these should not be necessary. Were you having issues calling either of these functions?

Comment thread app.js
const { vegetables } = data
const { id } = req.params

const veg = vegetables.filter(el => el.id.includes(id))[0]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.find() is a good substitute here.

Comment thread app.js
const veg = vegetables.filter(el => el.id.includes(id))[0]
if (!veg) {
const message = `Could not find vegetable with ID of ${id}`
next({ status: 404, message })
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll want to add a return statement here as well, otherwise the next few lines will also get executed.

Comment thread app.js
res.status(200).json(veg)
})

app.put('/vegetables/:id', helpers.validate, (req, res, next) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think helpers.validate should perform the same function as the lines you have below.

Comment thread app.js
const { id } = req.params

const fruit = fruits.filter(el => el.id.includes(id))[0]
console.log(fruit)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove console.log() statements before committing!

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