Luke-Fanguna / FoodForLesser

0 stars 0 forks source link

Code Review Comments (Dennis Phun) #27

Open dphun123 opened 1 year ago

dphun123 commented 1 year ago
  1. For ease of access, the API key can be left blank. There is no reason for it, unless you want to set up protected endpoints.
  2. The files with no endpoints in them should be removed.
  3. In your create_list, set_item_quantity, and some other functions, there are "SELECT" with no "FROM". The queries would be more concise with "INSERT INTO … VALUES …".
  4. The endpoint to delete an item from a list is not consistent with the comments nor API spec.
  5. For your grocery lists, why even introduce a posting_id at all? Each post is just a list_id, item_id, and quantity. Unless you are planning to have repeat items (e.g. 5 carrots and 3 carrots as separate items), you can just delete by a list_id and item_id/name match. It also makes your endpoints more consistent.
  6. Consider renaming variables to names that match what they represent (not just "res" or "result").
  7. You don't need to insert a value for crowdsourced_entries(created_at) for your upload_entry function, since it is already defaulted to run now().
  8. Rename the function for updating crowdsource postings. It is currently named "get_bottle_plan".
  9. The store endpoints don't seem to be set up correctly. You have it under the lists prefix and included /stores/ in each.
  10. Consider getting values through their keys rather than their indices. It makes the code more readable.
  11. The code for the store endpoints is very messy. Some ways you can clean it up (at least for the distribute_list endpoint) include: changing the items to items_id and ending it with fetchall().item_id, rather than looping through the zeroth index; choosing the best with a query that returns one row with the lowest price; making the query that fetches items select distinct items only, so you wouldn't need the not in basket check; and doing the last two queries in a single one with a join.
  12. Your find_best_item endpoint has an error.