Conversation
…or GET /api/me/cart
app/server.js
Outdated
| } | ||
|
|
||
| for (let i = 0; i < users.length; i++) { | ||
| if (users[i].email === currentValidatedUser.email) { |
There was a problem hiding this comment.
this logic can be extracted to a function
app/server.js
Outdated
| return response.status(404).json({ message: "Invalid product id" }); | ||
| } | ||
|
|
||
| const { currentValidatedUser, isValidToken } = authenticate(request); |
There was a problem hiding this comment.
this should be before anything. Everything after this line is irrelevant, if the user is not authenticated
There was a problem hiding this comment.
I moved this to to top of all the relevant api routes
app/server.js
Outdated
| } | ||
|
|
||
| for (let i = 0; i < users.length; i++) { | ||
| if (users[i].email === currentValidatedUser.email) { |
app/server.js
Outdated
|
|
||
| for (let i = 0; i < users.length; i++) { | ||
| if (users[i].email === currentValidatedUser.email) { | ||
| const productIndex = users[i].cart.findIndex( |
There was a problem hiding this comment.
nesting loops is ok for this level but its not maintainable for larger datasets of either users of products. This will grow exponentially the iterations. This area is a very important part of the interview questions.
Check suggestion:
app/server.js
Outdated
| (item) => item.id === product.id | ||
| ); | ||
|
|
||
| if (productIndex === -1) { | ||
| return response | ||
| .status(404) | ||
| .json({ message: "Product not found in cart" }); | ||
| } | ||
|
|
||
| users[i].cart.splice(productIndex, 1); | ||
|
|
||
| return response.status(200).json({ | ||
| message: `Item with product id ${product.id} deleted from cart`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
const user = users.find(u => u.email === currentValidatedUser.email);
if (!user) {
return response.status(404).json({ message: "User not found" });
}
const productIndex = user.cart.findIndex(item => item.id === product.id);
if (productIndex === -1) {
return response.status(404).json({ message: "Product not found in cart" });
}
user.cart.splice(productIndex, 1);
return response.status(200).json({
message: `Item with product id ${product.id} deleted from cart`,
});
| }); | ||
|
|
||
| describe('Login', () => {}); | ||
| it("Should return an error message for invalid login credentials", function (done) { |
There was a problem hiding this comment.
you should test both scenarios of no user name and no password in different tests
test/server.test.js
Outdated
| .end((err, res) => { | ||
| res.status.should.equal(200); | ||
| res.body.should.be.an("array"); | ||
| res.body.length.should.be.above(0); |
There was a problem hiding this comment.
this is not really testing anything, if you have a bug and return a bad item for a different brand, this test will pass and you will have a bug in production.
You should use .equals and test agains a mock.
test/server.test.js
Outdated
| .end((err, res) => { | ||
| res.status.should.equal(200); | ||
| res.body.should.be.an("array"); | ||
| res.body.length.should.be.above(0); |
There was a problem hiding this comment.
same as above, this test is not testing anything
|
@ronyrosenberg Thanks Rony, really appreciate this. Some of this feels like it should be more instinctive by now (e.g. using .find rather than for loops, and general DRY principles). Not planning to resubmit for grading but I did just push a more recent commit with many of these changes. |
2 participants
No description provided.