Code Review Comments (Hunter Lathery)

[hlathery]

1. In chore_assignment, it seems a little redundant to have a whole class for just the status of a chore when you only use it once and it's just a string input.

2. In endpoints bill and chore_assignment, you import other endpoints but never use them, so you can probably clear that from the list of imports if not planning to use anymore

3. Creating a chore with priority < 1 or > 5 currently throws a 500 error. I see your schema currently requires that it be priority >= 1 and <= 5 But maybe inside your endpoint file you can add a more user friendly check to let the user know, and allow retry ability without throwing a 500 error. Similar to how you did with HTTP exception in assign_chore

4. I’m assuming WIP but the admin/reset is currently not implemented yet

5. Add a check in create roommate to not allow for duplicate roommate if they are already created, your initial population of the 3 example roommates accomplishes this, but just add for others

6. Thought most endpoint names are self explanatory, it might be nice to add a docstring to each of your endpoints

7. A big improvement to readability I think would be in your assign chore method, instead of individually querying if each check exists, and then doing another query. You could instead do the insert plus the desired WHERE, and then just do a if result.rowcount == 0: raise httpexception and handle accordingly.

8. In update chore status, currently you can enter any chore id or roommate id you wish, and not include a status and it will say the chore status is successfully updated. Consider also setting the status as an enum like you do in your bill endpoint (unpaid = 'unpaid' etc…)

9. For update bill status, if the user provides an incorrect bill_id OR roomate_id it will send back the same message saying No bill found with specific ID, even if you give it a correct bill_id but incorrect roommate id, consider the AND case as well.

10. Line 57 inside create_bill, instead of assigning a new variable to len(roommates), you could just do if roommates.rowcount == 0: raise exception.

11. Line 60 create_bill, since we’re dealing with dollars and cents, probably round to 2 decimal so you don't have this: “amount”: 9.3333 , instead 9.33 ($9 and 33 cents)

12. Add a check for creating bill to not allow negative cost of bill

[s6hib]

1. Looks like someone already addressed this. The part Hunter refers to is commented out and chore status is handled as a simple string with direct validation in the update_chore_status endpoint.

2. In chore_assignment.py it looks like we don't use from src.api.roommate import Roommate and from src.api.chore import Chore and in bill.py it looks like we don't use from src.api.roommate import Roommate so I removed them.

3. Addressed this to handle instances with priority numbers not 1-5.

4. Did this to do the following:

   Resets all chores to initial state:

   1. Deletes all chore assignments
   2. Deletes all chores
   3. Re-inserts default chores

5. Currently allows for people with the same email. Implemented change so that if you try to create a roommate with an email that already exists, you get an error.

6. Added doc strings to some endpoints where further clarification is needed. But like Hunter said, most are self explanatory.

7. Decided not to do this because:

• It provides better error messages (can tell exactly if chore not found vs roommate not found)
• More readable and maintainable code
• The performance difference would be negligible for this use case
• Current implementation follows separation of concerns

8. Implemented

9. Implemented. If one of the values is wrong it will tell you that one specifically is wrong and if both are wrong it'll tell you both of them are wrong. If both are right then it will work as intended.

10. Implemented

11. Done

12. Done