Nrasool21 / note-taker-app

0 stars 0 forks source link

Homework areas for improvement #1

Open hynesy23 opened 3 years ago

hynesy23 commented 3 years ago

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/package.json#L9 If your application has no tests you can just delete this bit

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/server.js#L6 Delete any unused, commented out code

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L7 This would have worked fine instead of line 5 - why comment it out?

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L9 No need for path or fs here, can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L13 More commented out code that can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L5 https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L8 You're not using databse of BASE_URL here, can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L13 Would be nice to see th fs methods extracted out to their own modules

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/htmlRoutes.js#L11 Currently, if you hit an endpoint that is anything other than 'notes', the app breaks. You should add a star here (htmlRouter.get("/*", renderIndexPage); ) as a catch all, so if you enter an endpoint that is not /notes it will take you back to the landing/index page

Nrasool21 commented 3 years ago

Hello Cillian,

Thank you for the feedback. i have have made all the changes (Except https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L13 Would be nice to see th fs methods extracted out to their own modules). The app is working fine in my machine. Let me know if I should make any further changes.

Thank you.

On Wed, Jun 2, 2021 at 7:36 PM hynesy23 @.***> wrote:

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/package.json#L9 If your application has no tests you can just delete this bit

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/server.js#L6 Delete any unused, commented out code

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L7 This would have worked fine instead of line 5 - why comment it out?

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L9 No need for path or fs here, can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/apiRoutes.js#L13 More commented out code that can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L5

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L8 You're not using databse of BASE_URL here, can be removed

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/controllers/apiControllers.js#L13 Would be nice to see th fs methods extracted out to their own modules

https://github.com/Nrasool21/note-taker-app/blob/f796bd31a56da387f2d58a350a3c89fd744455e0/src/routes/htmlRoutes.js#L11 Currently, if you hit an endpoint that is anything other than 'notes', the app breaks. You should add a star here (htmlRouter.get("/*", renderIndexPage); ) as a catch all, so if you enter an endpoint that is not /notes it will take you back to the landing/index page

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Nrasool21/note-taker-app/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/ASP7P5YBFVHOUWIAS6ADWFTTQZ2Z3ANCNFSM457IIMYQ .

hynesy23 commented 3 years ago

Hi Nazia,

That was quick, nice one! Don't worry too much about the fs methods, that was just me being picky ;) This looks good, much neater. Just 2 things: you don't need line 5 in apiControllers.js, and in your apiRoutes.js, the purpose of destructuring the controller functions is that so you could delete line 5 and remove 'apiFunction.' from the start of these functions names (see screenshot below:) image