philipsquire / express-api

0 stars 0 forks source link

Project Feedback #1

Open philipsquire opened 3 years ago

philipsquire commented 3 years ago

Create a todo list API

@egillespie Can you take a look at this? It's hosted here and meets the following criteria:

Got most of the heavy lifting done, but had to implement a workaround. I don't understand why there is no error when I type in a false id.

egillespie commented 3 years ago

Hi Philip, I can explain what's going on when you type a false id: there are two routes for get('/todos/:id' (one on line 19 and again on line 60). Express will use the first route handler it can find that matches the path, so the line 19 version that doesn't have the error handling is running instead of the one below it. If you remove the handler on line 19, you'll see the results you expect. 😁

Beyond that, your routes are looking really good and I have a couple of tips I can offer to help make the API a little more reliable. Can you try these out?

Add 404 error to PUT and DELETE routes

Would you mind adding a 404 response to the PUT and DELETE routes just like the one in the GET /todos/:id handler?

This will enforce a consistent API that won't allow any operation to perform (accidentally or otherwise) on a non-existent todo item.

Never decrease the todosNum value

Your approach using todosNum to generate a unique identifier for each new todo is pretty good and this is a common approach in SQL databases to generate IDs called sequences.

There's a catch to sequences though: they should only ever increase in value. If you decrease the value, there's a chance that you will use the same number twice and create a conflict of IDs, or worse, overwrite information. 😱

This will happen in your API now. If you add three todos they'll get the IDs of 0, 1, 2 and todosNum will be set to 3. If you delete todo 0, todosNum will be decreased to 2. Adding another todo will then overwrite the existing todo with ID of 2.

The good news is that the solution is easy: never decrease the value of todosNum and always increase it when it's used.

Can you make the touch-up to fix this issue in your API?


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! 🎸

philipsquire commented 3 years ago

Done!

Get Outlook for iOShttps://aka.ms/o0ukef


From: Erik Gillespie @.> Sent: Tuesday, April 20, 2021 8:09:02 PM To: philipsquire/MI-449-js-create-an-api-with-express @.> Cc: Squire, Philip David @.>; Author @.> Subject: Re: [philipsquire/MI-449-js-create-an-api-with-express] Project Feedback (#1)

Hi Philip, I can explain what's going on when you type a false id: there are two routes for get('/todos/:id' (one on line 19 and again on line 60). Express will use the first route handler it can find that matches the path, so the line 19 version that doesn't have the error handling is running instead of the one below it. If you remove the handler on line 19, you'll see the results you expect. 😁

Beyond that, your routes are looking really good and I have a couple of tips I can offer to help make the API a little more reliable. Can you try these out?

Add 404 error to PUT and DELETE routes

Would you mind adding a 404 response to the PUT and DELETE routes just like the one in the GET /todos/:id handler?

This will enforce a consistent API that won't allow any operation to perform (accidentally or otherwise) on a non-existent todo item.

Never decrease the todosNum value

Your approach using todosNum to generate a unique identifier for each new todo is pretty good and this is a common approach in SQL databases to generate IDs called sequences.

There's a catch to sequences though: they should only ever increase in value. If you decrease the value, there's a chance that you will use the same number twice and create a conflict of IDs, or worse, overwrite information. 😱

This will happen in your API now. If you add three todos they'll get the IDs of 0, 1, 2 and todosNum will be set to 3. If you delete todo 0, todosNum will be decreased to 2. Adding another todo will then overwrite the existing todo with ID of 2.

The good news is that the solution is easy: never decrease the value of todosNum and always increase it when it's used.

Can you make the touch-up to fix this issue in your API?


After you’ve made your changes and pushed them to GitHub and your hosted site, give it a once-over to make sure it looks right, then comment back here and I’ll take another look.

Thanks! 🎸

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/philipsquire/MI-449-js-create-an-api-with-express/issues/1*issuecomment-823680607__;Iw!!HXCxUKc!lfDRbWn_8r8B2DaPf5qj5S0qNCrWibn2mvtLNZ-nm9NjnrieFyYYYdiSFGQZYfoi$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AONQCAQ4MCCLTP6X6USO6LDTJYJR5ANCNFSM43JIFFDA__;!!HXCxUKc!lfDRbWn_8r8B2DaPf5qj5S0qNCrWibn2mvtLNZ-nm9NjnrieFyYYYdiSFAcAvVav$.

egillespie commented 3 years ago

Awesome, those changes looks and work really well! I noticed a couple of new issues this time around as well. I think these are the last ones. Can you touch these up?

Call app.use after all other route handlers

Now that the first app.get('todos/:id' has been removed, I'm getting a "not found" message on all todos, even ones that exist. This is happening because the generic catch-all handler — app.use — is being called before app.get in your server.js file so it's also taking priority over your GET handler like line 19 was previously.

Would you mind making sure app.use is called after all of the other route handlers so I can retrieve individual todos again?

Replace toString(todosNum) with todosNum

I didn't notice this during my first pass, but the toString(todosNum) code on line 24 is trying to treat todosNum as a Number object instead of a primitive integer value. This results in the redirect sending me to todos/[object%20Undefined] instead of /todos/0 like I would expect.

Luckily JavaScript will automatically convert primitive integers like todosNum to strings automatically so if you replace toString(todosNum) with todosNum then that part of the API will start working as expected.


After you make these changes, comment back and I'll give it another once over. Keep up the good work! 🤘

philipsquire commented 3 years ago

Should finally be good now!

Get Outlook for iOShttps://aka.ms/o0ukef


From: Erik Gillespie @.> Sent: Tuesday, April 20, 2021 8:32:57 PM To: philipsquire/MI-449-js-create-an-api-with-express @.> Cc: Squire, Philip David @.>; Author @.> Subject: Re: [philipsquire/MI-449-js-create-an-api-with-express] Project Feedback (#1)

Reopened #1https://urldefense.com/v3/__https://github.com/philipsquire/MI-449-js-create-an-api-with-express/issues/1__;!!HXCxUKc!nNmdk-9wfiKPqHPoaExNm8LLIslGXUr09PVxE1kd69XwJmLZ0hxxD_IZ0h2i_8S_$.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://urldefense.com/v3/__https://github.com/philipsquire/MI-449-js-create-an-api-with-express/issues/1*event-4622159049__;Iw!!HXCxUKc!nNmdk-9wfiKPqHPoaExNm8LLIslGXUr09PVxE1kd69XwJmLZ0hxxD_IZ0sbIhFx3$, or unsubscribehttps://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/AONQCAW7YC2HGHKUSLH3MULTJYMLTANCNFSM43JIFFDA__;!!HXCxUKc!nNmdk-9wfiKPqHPoaExNm8LLIslGXUr09PVxE1kd69XwJmLZ0hxxD_IZ0pWab3oO$.

egillespie commented 3 years ago

Totally! Everything's working as expected now. Awesome job on this API, it turned out great. Looks like you're getting a 4.0 with this project too. Nice! 🎉 🥳 :shipit: