Code Review Comments Krishnanshu Gupta

[Krishnanshu-Gupta]

1. Consider removing admin.py since it’s just an empty file with no functionality.

2. Some of the commit messages didn’t have the appropriate information to describe the change. The best practice is to usually have an action-specific message, which some of your commits do have, but then there are others with titles like “try test again” and “done maybe??” which are harder to decipher.

3. Within peepcoins.py, there doesn't seem to be a valid reason to define the 'add_peepcoins_query' as a global variable outside the scope of the put_add_peepcoins method. Since the query is only being used once within that method, it should be defined in the same format as all of your other queries.

   def put_add_peepcoins(request: PeepCoinRequest):
   with db.engine.begin() as connection:
       add_peepcoins_query = text(
           """
           INSERT INTO user_peepcoin_ledger (user_id, change)
           VALUES (:user_id, :change)
           """
       )
       connection.execute(
           add_peepcoins_query, {"user_id": request.user_id, "change": request.change}
       )
   return "OK"

4. In reviews.py, you're currently importing add_peepcoins_query from peepcoins.py within the post_add_review. Since the query text is quite short, there is no performance or memory improvement, so for the sake of comprehensibility, it would be more appropriate to just duplicate it within both files.

5. Within users.py, this query: """ SELECT id FROM user_test WHERE username = :name """ is being repeated several times. Instead of duplication here, you can either make a new method that performs this function, example: "get_user_id_from_username" or make that query a global variable.

6. The structure and operation of the user_follows_other_user and update_followers methods in users.py seem to be almost identical, there is definitely potential here to refactor the code to avoid the unnecessary duplication.

7. Additionally, in all 3 methods within users.py, within the update statements you perform a query for finding the ID of the user within the where clause. You have already found the id's of both users through the previous Select queries, so you should be passing in one of those id's as a parameter rather than finding it again.

8. Within post_add_review in reviews.py, there is no functionality to check whether a submitted review was valid or not. Since the user will receive peep coins for doing this operation, they could simply send unlimited duplicate requests of the same review to get unlimited peep coins.

9. Within several of the files, like coupons.py, I'm not sure why the parameters for queries are being passed in as lists. [{"name": request.name, "address": request.address}], this should be changed to just {"name": request.name, "address": request.address} unless there's a good motivation for keeping them in the list format.

10. Within routes.py, for the get_followers_routes method, it might be appropriate to have some form of limit or pagination set in case there's too many routes to return.

11. In peepcoins.py, this if is_valid == "FALSE": should be changed to be a boolean comparison, ex: if is_valid == False:

12. Many of the queries in the code could be prone to error faulting and exceptions, and it would be important to add error checking in the future to prevent this.

[Krishnanshu-Gupta]

Also, a lot of the endpoints don't take in the proper information or return the necessary info to handle the necessary operation. For example, when creating a user, business, coupon, etc, no id is returned so there's no way to know what id to use while testing since there's also no GET methods in your implementation. Also, in /routes/followers for example, it doesn't take in the user's id, so how could it know whether the friend_username and the user are actually friends in the app or not. Several cases like this through the endpoints.