search

AhmedJamal93 / basketball-api

0 stars 0 forks source link

Overall Comments #1

Open aidanbell opened 4 years ago

aidanbell commented 4 years ago

I know that you hit a lot of issues in the development of this project, so well done getting it all done by the deadline! Here are my comments/notes/issues:

  1. App is Currently Broken Unfortunately, at the time of testing, your deployed app is not working. It could be an issue with your API being down, or something with your data parsing. Check your heroku logs for more info on exactly what the cause is.
  2. Routes/Controllers: You have zero organization between your routes and controllers. This is a very important when developing full stack applications. For example, I wanted to see if I could spot why your app was currently crashing on search. The first stop I would check to troubleshoot would be your controllers. I expected to see some sort of player controller, but all you had was an index controller with one function in it. When I then checked your index router, not only was every player controller function in there, but also all of your OAuth controller functions. I know that in the lessons, we built most of our OAuth functions within the router, but we still separated them into a users router. Remember, you should separate routes that handle similar things to their own routers (players, team, users, etc), and then match those to their own controller. This makes troubleshooting and adding functionality a breeze.
  3. Duplicate Routes: You have these two routes in your index router that are identical. We want to avoid this whenever possible, because having identical routes can confuse our server, and lead to very unexpected results. https://github.com/AhmedJamal93/basketball-api/blob/a524eb6c5b9c5164e7984cd7626e3fa68ea305fa/routes/index.js#L48-L66
  4. Handling of XML data: This isn't something that we really covered in class, but XML is another way that APIs can return their data. While the package that you used to get around your incoming XML data (jsondom) works, you could have probably saved a lot of time by converting it to JSON, and operating on it that way. The other (probably quicker) way to do it, would be to seed all of that data into your database. Database filters/searches are always the quickest and easiest way to sort through data (that's what they're made for!)
aidanbell commented 4 years ago

Edit:

  1. App is currently NOT broken: Thanks for letting me know that queries only worked when typed in lowercase. This could be an easy fix by calling toLowerCase() on your req.body before passing it into the rest of your function. This is also a good usecase for error handling. After that though, everything works out fine. Your adding and deleting players is successful.