Skip to content

Stephen Swaringin - Mongo/Express Eval#158

Open
sswaringin wants to merge 36 commits intoprojectshft:masterfrom
sswaringin:master
Open

Stephen Swaringin - Mongo/Express Eval#158
sswaringin wants to merge 36 commits intoprojectshft:masterfrom
sswaringin:master

Conversation

@sswaringin
Copy link

No description provided.

sswaringin added 30 commits July 2, 2024 06:56
const products = require('./routes/products');
const fakeData = require('./routes/fakeData');

mongoose.connect('mongodb://localhost/products');

Choose a reason for hiding this comment

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

next time add some connection helpers to see in the console when connections was established or not, example:

ongoose.connection.on("connected", () => {
	console.log("Connected to MongoDB successfully");
  });
  
  mongoose.connection.on("error", (err) => {
	console.error(`Failed to connect to MongoDB: ${err.message}`);
  });

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback. It would have been nice for the Parsity material to include recommendations such as this if it is best practice.

Choose a reason for hiding this comment

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

Stephen, your submission is excellent and this review and comments are just for your learning!
pinging Brian as indeed as well in case he thinks it should be added to the program

@BrianJenney !

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, Rony!

}

async function getProducts(req, res, next) {
let page = Number(req.query.page);

Choose a reason for hiding this comment

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

Suggested change
let page = Number(req.query.page);
let page = Number(req.query.page) ?? 1

Should work the same as the check you are doing in line 38.

const resultsLimited = results.slice(start, stop);

// Sends the paginated results to the client
res.send({ resultsCount, totalPages, page, resultsLimited, query, category });

Choose a reason for hiding this comment

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

I think the division of responsibility is great, BUT!
The helper should not be sending the response back. A helper in general is a service that is actually helping you, having reusable functionality, etc. Here is way more than that, this service is reading the DB and even sending the responses back.
I would have either called this helper something else or separate even more the responsibilities. Specially leaving the response to be sent from the route.

// A callback function to pull all categories
async function getCategories(req, res, next) {
// Query all products from MongoDB
const results = await Product.find({});

Choose a reason for hiding this comment

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

// A callback function to pull all categories
async function getCategories(req, res, next) {
// Query all products from MongoDB
const results = await Product.find({});

Choose a reason for hiding this comment

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

be more specific, results of what?

Suggested change
const results = await Product.find({});
const allProducts = await Product.find({});

// A function to pull the reviews array from a product document and log review count
function getReviews(productObj) {
const reviews = productObj.reviews;
console.log({reviews: reviews.length});

Choose a reason for hiding this comment

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

do not submit code with console logs

}

// A function to remove the _id field from each review
function filterReviews(productObj) {

Choose a reason for hiding this comment

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

the function name is not really accurate, also why do you need it? seems not to be in use.
Clean your code clean!

return res.status(401).send('Please provide a name, price, category, and image.');
}

let product = new Product({name, price, category, image});

Choose a reason for hiding this comment

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

should be const

const { name, price, category, image } = req.body;

if (!name || !price || !category || !image) {
return res.status(401).send('Please provide a name, price, category, and image.');

Choose a reason for hiding this comment

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

wrong error, this should be 400. 401 and 403 are for authentication.

const { username, text } = req.body;

if (!username || !text) {
return res.status(401).send('Missing required values: username, text');

Choose a reason for hiding this comment

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

400 error

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