charm-13 / petcafe

https://cute-creature-cafe.onrender.com
0 stars 0 forks source link

Code Review Comments Alejandro Espinoza #16

Open comp-scipain opened 2 days ago

comp-scipain commented 2 days ago

carts.py

  1. set_item_quantity() should have a check for if the user has enough gold to make a purchase. However since users start off with 0 gold if they buy anything they end up having negative gold. Unless you plan on implementing something about the user going into debt. Since you can currently buy any amount of the most expensive item essentially for free.

  2. The set_item_quantity() function should return a much more human readable error message when the user enters an item_sku that doesn't exist in the catalog. For example, you could return something like "We don't sell {item_sku} here." or "{item_sku} does not exist in the snack catalog".

  3. (Similar to the previous comment) checkout() should also return a more human readable error when it fails. For example if the user enters a cart_id that doesn't belong to any existing cart it can return something like this

    {
    "error": "No cart with this id ({cart_id})  exists"
    }
  4. Carts aren't being deleted after checkout() is called meaning that you can repeatedly purchase what ever gets added to the user's cart. After the user purchases the treats they want there is no need to keep their cart around.

creatures.py

  1. feed_creature() doesn't check if the user even has any treats to feed creatures. So buying snacks is pointless if your know what snacks are in the catalog. You should return an error message like "You don't have any snacks to feed them" if the user doesn't have any snacks.

  2. In feed_creature() you pass in an item_sku, but you don't check if the user actually have that specific item in their inventory. Although with the way this function is currently implemented the user can pass in any item_sku that is the snack catalog and be able to feed the creatures without having that specific item in their inventory. For example, if the user buys a STEAK_SKEWER right after creating their then calls POST /users/{user_id}/creatures/{creature_id}/feed/{treat_id} while passing in RAZZ_BERRY as the treat_id they can feed the creature a razz berry without having one in their inventory

  3. feed_creature() Also doesn't check how many of the passed in snack the user has in their inventory. So they could just buy one random snack and continuously call POST /users/{user_id}/creatures/{creature_id}/feed/{treat_id} until the creature's hunger level reaches 100 where it stops them.

  4. Although this might just be a personal preference, you could make it more explicit that if the creature liked the treat it was just fed when you call POST /users/{user_id}/creatures/{creature_id}/feed/{treat_id}. You could return a message with a response from the creature. For example, if the creature hated the you could return a message like "{creature_name} tossed the {treat_name} aside" or if they where feed their favorite treat "{creature_name} devoured the {treat_name}"

  5. play_with_creature() doesn't check if the user enters an invalid user_id or creature_id and returns and Internal Server Error if either of these values aren't in the table in this query

stats = connection.execute(sqlalchemy.text(""" SELECT happiness, COALESCE(user_creature_connection.affinity, 0) AS affinity
                                                FROM creatures 
                                                JOIN creature_types 
                                                    ON creatures.type = creature_types.type AND creatures.id = :creature   
                                                LEFT JOIN user_creature_connection 
                                                    ON creatures.id = user_creature_connection.creature_id AND user_id = :user_id"""),
                                                {"creature": creature_id, "user_id":user_id}).mappings().fetchone()

users.py

  1. The DELETE query in delete_user() doesn't need to be formatted. Since it can easily fit in one line and just a readable on most displays.

  2. create_user() Can accept an empty string ("") as a username. Which doesn't seem to affect anything right now, but you should keep that in mind if you want to implement anything using the username in the future. So it would be a good idea to come up with some guidelines for creating a username.

More general comments

  1. You could move all your BaseModel (i.e NewUser and CartItem) Classes to the top of each file to improve readability.