Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implemented dao, connected to db, tested all of this in main #317

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kaiiiseeel
Copy link

No description provided.

Copy link

@Nikname2303 Nikname2303 left a comment

Choose a reason for hiding this comment

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

Look great, only 1 minor comment

public static void main(String[] args) {
BookDao bookDao = (BookDao) injector.getInstance(BookDao.class);

Book book1 = new Book("Book #1", BigDecimal.valueOf(10));

Choose a reason for hiding this comment

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

why book1?

Copy link
Author

Choose a reason for hiding this comment

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

why book1?

Oh, at first I added several books, and then I decided to leave only one and forgot to rename it, I'll have to do that, thanks

Comment on lines 26 to 27
statement.setString(1, book.getTitle());
statement.setBigDecimal(2, book.getPrice());
Copy link

Choose a reason for hiding this comment

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

magic numbers, check other methods as well

Comment on lines 116 to 118
Long id = resultSet.getLong("id");
String title = resultSet.getString("title");
BigDecimal price = resultSet.getBigDecimal("price");
Copy link

Choose a reason for hiding this comment

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

replace literals with constants


ResultSet resultSet = statement.getGeneratedKeys();
if (resultSet.next()) {
book = new Book(resultSet.getLong(1), book.getTitle(), book.getPrice());
Copy link

Choose a reason for hiding this comment

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

use getBook() that you already have

Comment on lines 50 to 53
String title = resultSet.getString("title");
BigDecimal price = resultSet.getBigDecimal("price");

Book book = new Book(id, title, price);
Copy link

Choose a reason for hiding this comment

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

and here too

BigDecimal price = resultSet.getBigDecimal("price");

Book book = new Book(id, title, price);
return Optional.of(book);
Copy link

Choose a reason for hiding this comment

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

you can check the difference in behavior with the null if you want 😉

Suggested change
return Optional.of(book);
return Optional.ofNullable(book);

}

private static Book getBook(ResultSet resultSet) throws SQLException {
Long id = resultSet.getLong("id");
Copy link

Choose a reason for hiding this comment

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

check the common mistakes on that getLong()

Comment on lines 15 to 16
DB_PROPERTIES.put("user", "root");
DB_PROPERTIES.put("password", "$miskPass@");
Copy link

Choose a reason for hiding this comment

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

magic values, make them constant

`id` BIGINT AUTO_INCREMENT PRIMARY KEY,
`title` VARCHAR(255) NOT NULL,
`price` decimal NOT NULL
)
Copy link

Choose a reason for hiding this comment

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

Suggested change
)
)

try {
Class.forName(DRIVER);
} catch (ClassNotFoundException e) {
throw new RuntimeException("Can not load JDBC driver" + DRIVER, e);
Copy link

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Can not load JDBC driver" + DRIVER, e);
throw new RuntimeException("Can not load JDBC driver: " + DRIVER, e);

Comment on lines 25 to 27
public static Connection getConnection() throws SQLException {
return DriverManager.getConnection(DB_URL, DB_PROPERTIES);
}
Copy link

Choose a reason for hiding this comment

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

let's use try/catch here to get the error closer to it's reason

@kaiiiseeel kaiiiseeel requested a review from karma-o April 4, 2024 20:04
Copy link

@karma-o karma-o left a comment

Choose a reason for hiding this comment

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

LGTM 👍

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.

3 participants