Shadi A.#5
Conversation
…bers, books, authors, publishers, tags, borrowings, and relations.
…g data insertion and retrieval logic
cometbroom
left a comment
There was a problem hiding this comment.
Nicely done. The schema includes the important entities from the assignment, uses primary keys and foreign keys consistently, and correctly introduces bridge tables for the many-to-many relationships between books/authors and books/tags. The Java part also shows good JDBC fundamentals: it opens a connection, uses PreparedStatement throughout, separates insert and fetch logic into methods, and includes a joined query that produces useful console output.
I do have some comments on the code where relevant. Please take the time to read but do note that it's optional to implement the suggestions.
One more important thing to add, while this was not a strict assignment requirement, it would make the project much easier to review if the README included clear run instructions.
For example, after reading the schema and Java code, I should be able to quickly answer:
- Which database name/user/password should I create?
- How do I run
schema.sql? - Which command runs the Java demo?
- Are there any prerequisites, such as PostgreSQL running locally?
For a course submission this is helpful, but for a GitHub profile project it becomes very important. Reviewers, recruiters, or other developers often decide quickly whether they can run a project. A short section like this would make the work much more accessible:
Even if the code is good, missing setup/run instructions can make the project feel unfinished because the quickest way to verify the work is unclear.
| JOIN book b ON bc.book_id = b.id | ||
| JOIN publisher p ON b.publisher_id = p.id | ||
| ORDER BY br.id | ||
| """; |
There was a problem hiding this comment.
Very nice use of multiline strings!
| + " | returned: " | ||
| + resultSet.getTimestamp("returned_at") | ||
| + " | fine: €" | ||
| + resultSet.getBigDecimal("fine_eur"); |
There was a problem hiding this comment.
You can also do the below on multiline strings!
String record = """
%s | member_card: %s
borrowed_book: %s | copy: %s | shelf: %s
borrowed at: %s
due: %s
returned: %s
fine: %s
""".formatted(
resultSet.getString("member_name"),
resultSet.getString("card_number"),
resultSet.getString("book_title"),
resultSet.getString("barcode"),
resultSet.getString("shelf_code"),
resultSet.getTimestamp("borrowed_at"),
resultSet.getTimestamp("due_date"),
resultSet.getTimestamp("returned_at"),
resultSet.getBigDecimal("fine_eur"));The parsing code itself though doesn't really belong to fetchData method, please see my other comment.
| ResultSet resultSet = statement.executeQuery()) { | ||
|
|
||
| while (resultSet.next()) { | ||
| String record = resultSet.getString("member_name") |
There was a problem hiding this comment.
Parsing a ResultSet is something that will most likely be reused and is not the concern of fetchData which should just fetch the data without operating on it.
You could extract this into a method named parseBorrowingData.
|
|
||
| List<String> records = fetchData(connection); | ||
|
|
||
| for (String record : records) { |
There was a problem hiding this comment.
Minor issue. "record" is a keyword in Java for record types. Keywords such as "class", "int" etc... can't be used as names for variables. Exception to the rule has been made for "record", "var" and "yield" because these are modern Java features (After Java 8) and to maintain backwards compatibility with older Java versions, are still allowed.
Nevertheless, while it's technically possible, better to name variables differently like "aRecord" to minimize possible confusion.
|
|
||
| private static final String URL = "jdbc:postgresql://localhost:5432/library_db"; | ||
| private static final String USER = "hyfuser"; | ||
| private static final String PASSWORD = "hyfpassword"; |
There was a problem hiding this comment.
Can understand hardcoded local credentials in an assignment. Just a hint, the next professional step is to read connection details from environment variables so secrets do not live in source code:
private static final String URL = System.getenv().getOrDefault(
"DB_URL",
"jdbc:postgresql://localhost:5432/library_db"
);
private static final String USER = System.getenv().getOrDefault("DB_USER", "hyfuser");
private static final String PASSWORD = System.getenv("DB_PASSWORD");| CHECK (fine_eur >= 0), | ||
| CHECK (returned_at IS NULL OR returned_at >= borrowed_at), | ||
| CHECK (due_date >= borrowed_at::DATE) | ||
| ); No newline at end of file |
There was a problem hiding this comment.
Nice, clean schema overall. Good normalization, sensible bridge tables, and the CHECK constraints are spot on! One thing to note though: you've used ON DELETE CASCADE on every foreign key, and in several places that's dangerous.
Take the borrowing table:
member_id INT NOT NULL REFERENCES member(id) ON DELETE CASCADE,
book_copy_id INT NOT NULL REFERENCES book_copy(id) ON DELETE CASCADE,
Borrowings are the library's financial/bookkeeping records (loans, due dates, fines). With CASCADE, deleting a member or even a single damaged book copy, silently wipes out the entire loan history attached to it, including unpaid fines.
A library can never explain "where did €1.75 of fines go?" if the records vanish with the row.
Historical/transactional records should almost never cascade away.
The same applies to book.publisher_id:
publisher_id INT NOT NULL REFERENCES publisher(id) ON DELETE CASCADE
Deleting one publisher would delete all of its books, which cascades further into book_copy, book_author, book_tag… one DELETE FROM publisher could empty half the catalog. Here you want the database to protect you instead:
CASCADEonly when the child row is meaningless without the parent (e.g. bridge rows likebook_author/book_tag, orcardbelonging to a member).RESTRICT(or default:NO ACTION) when deleting the parent would destroy valuable data:borrowing.member_id,borrowing.book_copy_id,book.publisher_id,book_copy.book_id.
| CREATE TABLE member ( | ||
| id SERIAL PRIMARY KEY, | ||
| name VARCHAR(255) NOT NULL, | ||
| email VARCHAR(255) NOT NULL UNIQUE, |
There was a problem hiding this comment.
If you want to enforce a basic shape at the database level, you could add a small sanity CHECK constraint like below, but don’t try to fully validate every possible email format in SQL. The important database constraint here is UNIQUE, because each member email should identify one member.
email VARCHAR(255) NOT NULL UNIQUE
CHECK (email ~* '^[A-Z0-9._%+-]+@[A-Z0-9.-]+\.[A-Z]{2,}$')
| member_id INT NOT NULL UNIQUE REFERENCES member(id) ON DELETE CASCADE, | ||
| number VARCHAR(255) NOT NULL UNIQUE, | ||
| issued_on DATE NOT NULL, | ||
| expires_on DATE NOT NULL |
There was a problem hiding this comment.
For data integrity need to validate that expiry date is not before issued_on date. You could add a constraint like
CHECK (expires_on > issued_on)
|
|
||
| CHECK (fine_eur >= 0), | ||
| CHECK (returned_at IS NULL OR returned_at >= borrowed_at), | ||
| CHECK (due_date >= borrowed_at::DATE) |
There was a problem hiding this comment.
Because borrowed_at::DATE removes the time part, this can allow a due date that is earlier than the actual borrowing timestamp on the same day. For example, a book borrowed at 10:00 could have a due date of 09:00 on the same date and still pass the date-only comparison.
Use the full timestamp comparison instead:
CHECK (due_date >= borrowed_at)| } | ||
|
|
||
| public static void insertData(Connection connection) throws SQLException { | ||
| int publisherId = insertPublisher(connection, "O'Reilly"); |
There was a problem hiding this comment.
I noticed that the insertData() method only adds one row for the publishers table. The assignment requires at least two rows in each table.
This is an easy thing to miss, especially when you're focused on getting the code working. For future assignments, I recommend adding a quick final checklist or doing a quick pass against the assignment requirements as the last thing to finalize.
Complete all tasks