Code Review Comments Taran Singh

[tarannssingh]

1. I don’t think it is best practice to commit all of your node modules to github. It makes the repo large. I believe if you have the package-lock.json file, which you do, someone would be able to download exact dependencies via that file. Also, it's not clear why you are using NPM if you are not running node.
2. Why does the user only get their first and last name back when they signup/login. I think they should get their ID back. This can be done with “returning id” within the insert statement. Also, because there is no uniqueness constraint on first and last name, I could create as many users as I want with the same first name and last name. The code would not consistently give me back the same credentials for the specific first and last name that pertain to me.
3. Instead of if sets is not None: for your update_user_workout endpoint, I think checking if sets: would accomplish the same thing and is more readable
4. For the get_workouts_from_user, I think a doc string (e.g. “”” content “””) would better serve the purpose of housing multi line sql.
5. I think you are missing a lower() in the sql for your delete workout endpoint
6. I think someone who isn’t familiar with the gym would have a hard time understanding the magic numbers used to analyze workouts. I think providing comments and then an explanation in the response (i.e for strength more than 6 reps is suboptimal as you should aim for using the heaviest weight possible. If you are able to do more than 6 reps, that is not heavy enough) This way the user gets a better intuition on why certain magic numbers are being picked.
7. I saw 2 connections in your post_custom_workout function. I think one would have been good.
8. In your delete_workout_from_user method in the delete workout endpoint, I noticed a nested sql statement. I think a pivot to making decisions off of IDs rather than names might make certain sql statements like this more readable.
9. There was one instance where I saw an id being passed in as a String. I think it would be more appropriate to force the assumption that ids are ints.
10. I think comments would make the code more readable and easier to mental map for someone looking at the code for the first time
11. This might be a little nit-picky but there is inconsistent if else syntax with /{muscle_group} and /workouts/{workout_name}/{muscle_group}/{equipment} endpoints. I think the code should be one style throughout.
12. For the cleanliness of your code, I noticed an extra import for sqlalchemy in Auth that was not needed.
13. I don't think there is error handling for edge cases. For example, if a user inputs negative sets, that is not raised as an error.
14. Although this is not code, your schema file does not match with your ER diagram.

[BaldoHado]

8. I disagree. If someone calls this endpoint, they would likely know the workout name, not the hidden ID that the database assigns to the workout.

Thank you for all your comments!