arnenoori
commented
11 months ago -
Database Queries: A bit of personal preference but the database queries could be optimized. For example, in the get_venues function in catalog.py, the SELECT * FROM venues query retrieves all columns from the venues table. If only certain columns are needed, it would be more efficient to only select those columns.
-
Input Validation: The API could benefit from more input validation. For example, in the book_venue function in book.py, there is no validation to check if the venue_id and performer_id exist in the database before trying to create a booking. This could lead to errors if a client tries to book a venue or performer that doesn't exist. Something I ran into while testing the API
Error Handling: In the book.py file, the functions book_venue doesn't handle the case where the venue or performer does not exist in the database. This could lead to a NoneType error when trying to access properties of the venue or performer. To solve this you should add more robust add error handling and checks if venue or performer exists in the database.
Input Validation: In book.py, there is no validation of the input data in the book_venue functions. You should add checks to ensure that the input data is valid and of the correct type.
Database Transactions: In book.py, the database transactions are not handled properly. If an error occurs during the transaction, the database session is not rolled back, which could leave the database in an inconsistent state. You should add error handling to roll back the transaction in case of errors.
Code Duplication: In book.py, there is a lot of duplicated code in the book_venue functions. You could create a helper function to handle the more common logic.
Consistent Function Naming: In catalog.py, the function to get performers is named get_venues(). This could be confusing and should be renamed to get_performers() for clarity.
User Authentication: In user.py, the user authentication is commented out. This means that anyone can make requests to the API without needing to authenticate. You should implement user authentication to secure the API or at least just add an API key to render.
Password Security: In user.py, the user passwords are stored in plain text in the database. This is a major security risk. You should hash the passwords before storing them in the database (I'm sure you'll change this eventually though).
Code Formatting: Personal preference, but seems like the codebase doesn't always follow a consistent code format.
Logging: There is no logging in the application. You should add logging to help with debugging and monitoring the application (especially when testing the API results)
Test Coverage: The codebase has no tests. You should add unit tests and integration tests to ensure that the application works as expected. These are not needed in the scope fo the project, but they're good to have.
Pagination: The get_venues() and get_performers() endpoints in catalog.py return all venues or performers at once. This could be inefficient if there are many venues or performers. Consider adding pagination to these endpoints especially when your database gets larger.
Environment Variables: The database connection string is hard-coded in the database.py file. You should use environment variables to store sensitive information like this.
Use of ORM: The code uses raw SQL queries, which might leave you guys open to SQL injection attacks. Consider using SQLAlchemy's ORM for safer code.