Conversation
composix
left a comment
composix
left a comment
There was a problem hiding this comment.
Nice work that is not only technically correct, but also done with care and a good eye for detail. The schema is well structured, the table relationships are clear, and the use of keys, constraints, and indexes shows a strong understanding of database design.
There was a problem hiding this comment.
Bonus points for this docker compose file. It helps a lot to be able to validate the work in this PR.
|
|
||
|
|
||
|
|
||
| - |
There was a problem hiding this comment.
This - makes that some tables are missing after running this script due to a syntax error. Deleting the - fixed the problem, so not really a big deal. Running it second time was a bit of a challenge using CREATE TABLE IF NOT EXISTS to make the script a bit more idempotent.
There was a problem hiding this comment.
Good catch, thank you! Fixed the stray - that was breaking the cards table creation. Also added CREATE TABLE IF NOT EXISTS across the schema so re-running the script won't fail if some tables already exist — makes it more forgiving during testing/debugging.
| card_number TEXT UNIQUE NOT NULL, | ||
| card_issued_on DATE NOT NULL, | ||
| card_expires_on DATE NOT NULL, | ||
| CHECK (card_expires_on > card_issued_on) |
| member_address TEXT NOT NULL, | ||
| CONSTRAINT fk_card_id | ||
| FOREIGN KEY (card_id) | ||
| REFERENCES cards (card_id) ON DELETE CASCADE |
There was a problem hiding this comment.
Should members whose card is deleted be deleted as well? Or is using ON DELETE SET NULL also an option?
There was a problem hiding this comment.
Great question — you're right, CASCADE didn't make sense here. Deleting a member's card (e.g. reissuing a lost one) shouldn't wipe out the member and their borrow history. Switched it to ON DELETE SET NULL, so a member can exist without a card temporarily, and their record (and any borrow history) stays intact.
There was a problem hiding this comment.
Using a .gitignore file with a line "target/" could be helpful to prevent build output to end up under version control.
There was a problem hiding this comment.
Added target/ (and *.class, plus out/ for IntelliJ's non-Maven build output) to .gitignore. Also removed target/ from version control since it had already been committed. Thanks for flagging it!
…DELETE SET NULL; update .gitignore for target/ and *.class
|
@composix , thank you so much for the thorough review and the kind words — really appreciate you taking the time to go through the schema in detail! Learned a lot from these — especially the point about cascade semantics, that one made me think more carefully about what "deleting" actually means for each relationship in the schema. |
2 participants
Implemented Task 1 and Task 2 for the Library Management System module.
Task 1
Task 2