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

Jv jdbc intro solution #378

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

Conversation

mkazarian-1
Copy link

No description provided.

Book update(Book book);

boolean deleteById(Long id);

Choose a reason for hiding this comment

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

Suggested change


@Dao
public class BookDaoImpl implements BookDao {
private static final String URL = "jdbc:mysql://localhost:3306/test";

Choose a reason for hiding this comment

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

It's better to create ConnectionUtil class to establish connection to DB


try (ResultSet generatedKeys = preparedStatement.getGeneratedKeys()) {
if (generatedKeys.next()) {
long id = generatedKeys.getLong(1);

Choose a reason for hiding this comment

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

Suggested change
long id = generatedKeys.getLong(1);
long id = generatedKeys.getObject(1, Long.class);
book.setId(id);
return book;

long id = generatedKeys.getLong(1);
return new Book(id, book.getTitle(), book.getPrice());
} else {
throw new RuntimeException("Failed to retrieve the generated key");

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("Failed to retrieve the generated key");
throw new DataProcessingException("Failed to retrieve the generated key");

}
}
} catch (SQLException e) {
throw new RuntimeException("Cannot create a connection to the DB", e);

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("Cannot create a connection to the DB", e);
throw new DataProcessingException("Can't add a new book, " + book, e);

Check all places where you are using RuntimeEx


try (ResultSet resultSet = preparedStatement.executeQuery()) {
if (resultSet.next()) {
String title = resultSet.getObject("title", String.class);

Choose a reason for hiding this comment

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

Create separate method to map ResultSet to book to avoid duplication in findById and findAll methods

}
}
} catch (SQLException e) {
throw new RuntimeException("Cannot create a connection to the DB", e);

Choose a reason for hiding this comment

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

Change message according to the logic of method (see example above)
Check everywhere

@@ -0,0 +1,6 @@
CREATE TABLE `books`(
`id` INT NOT NULL AUTO_INCREMENT,

Choose a reason for hiding this comment

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

Suggested change
`id` INT NOT NULL AUTO_INCREMENT,
`id` BIGINT AUTO_INCREMENT,

}
}
} catch (SQLException e) {
throw new DataProcessingException("Cannot create a connection to the DB", e);

Choose a reason for hiding this comment

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

Let's change messages to describe more informative the issue like:

Can't add a book:
Can't find book by id ....

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for comment but I little bit disagree with you because in this case we make try catch for Connection field and if we catch some exception this exception will related to Connection field which is responsible for connection to the database.
Am I right?

Choose a reason for hiding this comment

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

SQLException is related not only to Connection, but to PreparedStatement also.
It's better to use try-catch for public static Connection getConnection() and provide informative message about connection issue, and in BookDaoImpl catch provide information about
Can't add a book:
Can't find book by id ....

@@ -0,0 +1,22 @@
package mate.academy.dao;

Choose a reason for hiding this comment

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

Suggested change
package mate.academy.dao;
package mate.academy.util;

@@ -0,0 +1,12 @@
package mate.academy.dao;

Choose a reason for hiding this comment

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

Suggested change
package mate.academy.dao;
package mate.academy.exception;

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