kghalamb / AirStage

0 stars 0 forks source link

Code Review Comments Sabawoon Hakimi #2

Open SabaHakimi opened 11 months ago

SabaHakimi commented 11 months ago
  1. book.py: There is massive commented code block that should be removed.

  2. book.py, book_venue: There is repeated use of the variables names result and row. This hurts readability and makes debugging more difficult; variables should have more meaningful names.

  3. book.py, book_venue: I recommend labeling the time_finish, time_end, time_available, time_start, variables by the relevant entity (venue, performer) for the sake of readability; this if statement is especially difficult to follow: if not (time_available >= time_start and time_available <= time_finish) or (time_end >= time_start and time_end <= time_finish):

  4. book.py, book_venue: The time_works variable is redundant and adds unnecessary complication. You can just return { “success”: False } instead of setting the time_works variable to false.

  5. book.py: There are two book_venue functions with almost completely duplicate code. The duplicate code should be abstracted out into a helper function.

  6. There are several select statements throughout the code that are of the form …WHERE some_id_column = :parameter, which return a result that is then iterated. An ID should be unique, so only one row should be returned here. Instead of iterating through the result, use fetchone() or some other alternative method call to get the single row. For verification purposes, you can make another call to fetchone() and verify that it returns none to be sure that only one row is returned.

  7. book.py, modify_booking: The booking_exists variable is unnecessary, just check if the return value of fetchone() is none to see if the booking exists.

  8. book.py, book_venue: The return value of a connection.exec call is assigned to a variable that is never used. That assignment should be removed for clarity.

  9. user.py, signup: A ternary expression can be used for the boolean value assignment (the 0 and 1), and when checking the user type, the actual value can be checked instead before assigning for better readability/code clarity.

  10. user.py: There is a second function named signup. I believe it was intended to be named signin. That change should be made.

  11. I would avoid using SELECT * statements in general for clarity.

  12. The code is lacking in documentation all throughout. Functions should have docstrings explaining their purpose.