Solution#1825
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one critical requirement is not fully met: the table_name parameter passed into ActorManager.__init__ is not actually used in your SQL queries, which can break functionality when a different table name is provided. In app/managers.py, every query (SELECT, INSERT, UPDATE, DELETE) still hard-codes the table name actors instead of using self.table_name, so the manager is not truly configurable as intended. This conflicts with the task design and also partially violates the style requirement that the manager should work with a plural table name chosen at construction time ([CHECKLIST ITEM #2]).
To fix this, update all SQL statements in ActorManager to interpolate self.table_name for the table name, for example by building the SQL with an f-string for the table name while still using ? placeholders for values (so only the table name is interpolated, not user data). For instance, your current all query "SELECT id, first_name, last_name FROM actors" should be changed to use self.table_name instead of the literal actors, and similarly for the INSERT, UPDATE, and DELETE statements. Once you do that, your existing logic (including the list comprehension in all that naturally returns an empty list) will satisfy the functional requirements. Your overall structure with the Actor dataclass, correct method signatures, and connection handling is solid, and with these query adjustments your solution will be in good shape.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| self.connection = sqlite3.connect(db_name) | ||
| self.table_name = table_name |
There was a problem hiding this comment.
table_name is accepted as an argument and stored, but all SQL below still uses the hard-coded table name "actors". This makes table_name ineffective; you should build your queries using self.table_name to honor the constructor parameter.
| cursor = self.connection.execute( | ||
| "select * from actors" |
There was a problem hiding this comment.
This query hard-codes actors instead of using self.table_name. That conflicts with the purpose of passing table_name into __init__ and will break if a different table name is provided.
| self.connection.execute( | ||
| "delete from actors where id = ?", (pk,) |
There was a problem hiding this comment.
The delete statement also hard-codes actors; update it to use self.table_name so the manager truly works for the table specified in the constructor.
| self.connection.execute( | ||
| "insert into actors (first_name, last_name)" |
There was a problem hiding this comment.
The insert statement should refer to self.table_name instead of the literal actors, otherwise changing the table name in __init__ has no effect.
| self.connection.execute( | ||
| "update actors set first_name = ?, last_name = ?" |
There was a problem hiding this comment.
Similarly, this update statement should use self.table_name instead of actors to match the dynamic table name passed to the manager.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it fully meets the functional and structural requirements of the task. You correctly defined the Actor dataclass in models.py with id, first_name, and last_name, and implemented ActorManager in managers.py with the required CRUD methods and signatures. The manager’s __init__ properly accepts db_name and table_name, establishes the SQLite connection, and all SQL statements consistently use self.table_name, matching the configurability requirement and avoiding hard-coded table names. The all() method returns a list of Actor instances and naturally returns an empty list when there are no rows, which aligns with the expected behavior. There are no critical blockers or missing features with respect to the description or checklist, and the default table name being plural ("actors") also follows the naming guidance. Nice work implementing a clean, requirement-compliant solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
3 participants
No description provided.