Conversation
ryanbuui
force-pushed
the
project_reviews
branch
from
1261cbb to
767ac2f
Compare
…d total_stars to users tables, Modified get routes for contributors and users to include average_rating
typokign
left a comment
typokign
left a comment
There was a problem hiding this comment.
Great! Very clean, just a few suggestions below. Also, please run the linter, npm run lint:fix should auto-fix most lint issues for you.
app/api/contributors/index.js
Outdated
| var i; | ||
| for(i = 0; i < users.length; i++){ |
There was a problem hiding this comment.
nit: There's no need to define i before the loop, and we prefer let/const to var. Try: for (let i = 0; i < users.length; i++) {
| tbl.integer('total_stars'); | ||
| tbl.integer('total_reviews'); |
There was a problem hiding this comment.
issue: We can't edit existing migrations, they're designed to only run once. Create a new migration with npx knex migrate:make and add these fields in there.
| table.enum('skill', null, { useNative: true, enumName: 'enum_skills', existingType: true }) | ||
| .notNullable(); |
There was a problem hiding this comment.
nit: Nothing was really changed here but can you please revert? Nice to keep the git history clean on migrations.
…ented requested changes and revisions
| const users = (await query).rows; | ||
|
|
||
| let i; | ||
| for (i = 0; i < users.length; i += 1) { |
There was a problem hiding this comment.
Can you move the let into the loop body here? Otherwise it doesn't really help with scoping.
for (let i = 0; i < users.length; i++)
| }); | ||
|
|
||
| router.get('/:id', restricted, async (req, res) => { | ||
| router.get('/:id'/* , restricted */, async (req, res) => { |
There was a problem hiding this comment.
can you uncomment this please?
app/api/reviews/index.js
Outdated
| let correctBool = true; | ||
| for (let i = 0; i < reviews.length; i += 1) { | ||
| if (!(reviews[i].isInteger() && reviews[i] >= 1 && reviews[i] <= 5)) { | ||
| correctBool = false; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
You can simplify this a bit by using some functional programming concepts: map and reduce; map the reviews array to an array of booleans (true if the element is an integer between 1 and 5 inclusive) and reduce it using the and operator. Or maybe you could filter the reviews array by if the elements are not integers between 1 and 5 inclusive and then check length. No loop needed and you can make correctBool a constant.
const validMetrics = reviews
.map((r) => r.isInteger() && r >= 1 && r <= 5)
.reduce((a,r) => a && r);
const validMetrics = reviews.filter((r) => !r.isInteger() || r < 1 || r > 5).length === 0;
app/api/reviews/index.js
Outdated
| if (correctBool) { | ||
| const projectReview = await ProjectReviews.insert(req.body); | ||
| res.status(201).json({ projectReview }); | ||
| } else { | ||
| res.status(400).json({ message: 'Incorrect input for ratings' }); | ||
| } |
There was a problem hiding this comment.
Use a guard clause here.
if (!constantBool) return res.status(400).json({ message });
const projectReview = await ProjectReviews.insert(req.body);
return res.status(201).json({ projectReview });
app/api/reviews/index.js
Outdated
| if (projectReview) { | ||
| res.status(200).json({ projectReview, msg: 'Review was found' }); | ||
| } else { | ||
| res.status(404).json({ message: 'Review not found in the database' }); | ||
| } |
app/api/reviews/index.js
Outdated
| } | ||
| } | ||
| if (correctBool) { | ||
| const projectReview = await ProjectReviews.insert(req.body); |
There was a problem hiding this comment.
This currently returns an array of inserted objects, even if you only pass in one parameter. You can use array destructuring to easily get the first element:
const [projectReview] = await ProjectReviews.insert(req.body);3 participants
Description
Created a migration table called project_reviews containing:
Closes #182.