Skip to content

Shadi A.#5

Open
shmoonwalker wants to merge 5 commits into
HackYourAssignment:mainfrom
shmoonwalker:main
Open

Shadi A.#5
shmoonwalker wants to merge 5 commits into
HackYourAssignment:mainfrom
shmoonwalker:main

Conversation

@shmoonwalker

Copy link
Copy Markdown

Complete all tasks

@cometbroom cometbroom left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
""";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice use of multiline strings!

+ " | returned: "
+ resultSet.getTimestamp("returned_at")
+ " | fine: €"
+ resultSet.getBigDecimal("fine_eur");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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";

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Comment thread task-1/schema.sql
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  • CASCADE only when the child row is meaningless without the parent (e.g. bridge rows like book_author/book_tag, or card belonging 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.

Comment thread task-1/schema.sql
CREATE TABLE member (
id SERIAL PRIMARY KEY,
name VARCHAR(255) NOT NULL,
email VARCHAR(255) NOT NULL UNIQUE,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,}$')

Comment thread task-1/schema.sql
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

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment thread task-1/schema.sql

CHECK (fine_eur >= 0),
CHECK (returned_at IS NULL OR returned_at >= borrowed_at),
CHECK (due_date >= borrowed_at::DATE)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants