rcereno / REDX

0 stars 0 forks source link

Code Review Comments (Isaac Schiffler) #6

Open isaacschiffler opened 1 month ago

isaacschiffler commented 1 month ago
  1. Add more commenting within your code to make it more readable. This will help to organize the logic and also make it more clear what logic is for and what you are doing in your code.
  2. Add docstrings to the endpoint functions in which they do not already exist. This will help assert what the endpoints actually do. You can also include request and response specs to make it even more clear.
  3. For Catalog Search: clean up how you are using the input parameters to create your sql. Instead of using a bunch of if/elif statements, just do the order_by as the sort column they picked. This isn't incredibly essential, because the code works, but it could help clean up the code and make it simpler.
  4. Remove or flush out inventory/audit. Right now it is just an endpoint that doesn't do anything. If you want an audit endpoint, implement it, otherwise remove it from your src.
  5. For Cart View: instead of having two different sql queries to get the games and price for every item in the cart and then another to get the account name and checked_out value, combine it into one select query by using multiple joins in the sql text. Also, I may be missing something, but I am not sure what the data/use of checked_out is. Lastly, this endpoint does not seem to be consistent with your API spec.
  6. Add more try/except statements into your endpoint functions similar to implementation of set_item_quantity(), but make sure to also be consistent in how you are using try/except to maintain readability and understandability.
  7. For Set Item Quantity: Combine separate SELECT and INSERT sql queries into one sql query that inserts based on the select. This will clean up sql queries and make them more evidently intertwined. Also remove the UPDATE sql query for efficiency and cleanliness sake unless you really want it (more on why in API spec/Schema comments).
  8. For Cart Checkout: combine select and insert statement for games_bought and purchased into one sql query. Also, remove a lot of commented out code and redundancy.
  9. account_id = connection.execute(
                sqlalchemy.text(
                    """
                    SELECT account_id FROM carts
                    WHERE carts.id = :cart_id
                    ORDER BY created_at DESC
                    """
                ),
                [{
                    "cart_id": cart_id
                }]
            ).scalar_one()

    If this is only supposed to return one account_id, then the order by is either redundant or the cart_id and account_id logic is flawed. Think about maybe changing this to consider that idea. I assume cart_id should already be unique so order by is unnecessary?

  10. Still cart checkout: get the games_in_cart and total_cost directly from the cart_items table to remove the middle man of storing this data in the cart row of Carts (more in API spec/schema comments).
  11. Remove redundant code: there are quite a few commented out lines of code that should be reviewed and removed if not needed anymore. Also going through sql queries that have an order by but only want one returned value should be looked over to make sure to see if the order by is redundant or if the sql query hinges on the desired row being at the top of the order, then reformatting the query to make sure only the one you want is returned.
  12. Move sql metadata to the database.py file from api python files for better organization and so that it is only needed once in your source code. Then you can import the metadata into each individual api file as it is needed.