spruhaN / nutrition_pal

1 stars 1 forks source link

Code Review Comments Rhoyalinn Cereno #2

Open rcereno opened 1 month ago

rcereno commented 1 month ago

The code base looks great, overall. Although, here are a few suggestions for improvements:

  1. I would recommend splitting the functions/methods from the main.py file into separate files based on the endpoints/API calls, similar to the Potions Shop Project.

  2. For more organization for the project files, based on different file types and categories, I would recommend placing files into folders based on categories, such as v[]_manual_test_result files into a respective folder for test results, the data files into a folder for data sets, and database.py and schema.sql into a database folder. (Also there are two schema.sql files, should only have one).

  3. Although, the description of the API call/endpoint are in the API spec, some functions/methods have comment descriptions and some do not. I would recommend having the comment descriptions in all other functions/methods for consistency and also to let the reader know what is expected of the function or what its functionality is.

  4. There are two getWorkoutsByDay asynch functions in lines 74 and 96. I would suggest changing out the names, or renaming one of them so they aren't confused between the two, since one is based on a single person and the other is based on muscle groups, unless it's needed or intentional.

  5. For getWorkoutsByDay, the one that returns a list of workouts that target the given type, and getWorkoutMuscleGroups, the one that returns a given description of the given workout provided, I'm not sure what exactly to recommend but perhaps to rename the functions to relate more to the descriptions but also trying to distinct the two. It makes sense of the difference between the two, personally, but they may look similar or a shorter version of the other since it's based on the same muscle group but one is a designated workout and the other lists all types of exercises of the muscle group. May not be a needed change but something to think about.

  6. There are some instances where a variable is not accessed or used. To have better readability or shorten the code for unnecessary writings, maybe remove those instances, such as when assigning a result variable that is not being accessed or returned, such as in lines 70 and 136.

  7. I could be wrong about this since still getting used to writing SQL queries/code but for line 58, I think you can just leave it as "SELECT id" instead of having "as id" since it would return just the same. (Unless that is what is being referred to in line 64 and that is the only way it will work as expected).

  8. In some of the functions, such as getWorkoutsByDay (both of them) and getWorkoutMuscleGroups, there is a bit on inconsistency in the capitalization with the SQL statements, such as SELECT and FROM, for example. It doesn't affect the code functionality but it is a good idea to be consistent with the capitalization.

  9. In the API spec file, I think line 114 isn't supposed to be there, since it is referring to a GET method from earlier (the previous one).

  10. There are several functions that should account to only have positive values, as users shouldn't be able to input negative values of reps, sets, calories, weight, height, etc.

  11. ^ In addition with this suggestion, maybe also have the user be able to add units, since different places/countries use different measures (in terms of weight, for example) - unless limited to certain group then at least just be able to include a unit with each integer type, if applicable.

  12. In getDailyCalories(), there is a variable "sql" used twice, overwritten once, which may be best to have two different variable names for each SQL query calls, just for better readability and organization purposes, such as with postWorkout(), where instead of using "sql" variable, like the rest of the functions, it is split into find_qry and insert_sql variables of different queries/actions.

Overall, great job!