-
Notifications
You must be signed in to change notification settings - Fork 438
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
Added task solution #408
base: main
Are you sure you want to change the base?
Added task solution #408
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting your Pull Request! There are some critical issues that need to be addressed before we can merge your code. Specifically, the incomplete DB_URL
in the ConnectionUtil
needs to be fixed to ensure proper database connectivity. Additionally, the exception handling can be improved by using more specific exceptions rather than a generic RuntimeException
, and the update
method's behavior should be adjusted to handle cases where no book is found. Keep up the good work, and once these changes are implemented, we'll be ready to approve your PR! 😊 Remember, these fixes are crucial for maintaining the functionality and security of your application.
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public Book create(Book book) { | ||
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
|
||
try (PreparedStatement statement = ConnectionUtil.getConnection().prepareStatement(sql, | ||
Statement.RETURN_GENERATED_KEYS)) { | ||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
|
||
int affectedRows = statement.executeUpdate(); | ||
|
||
if (affectedRows == 0) { | ||
throw new SQLException("Creating book failed, no rows affected."); | ||
} | ||
|
||
try (ResultSet generatedKeys = statement.getGeneratedKeys()) { | ||
if (generatedKeys.next()) { | ||
book.setId(generatedKeys.getLong(1)); | ||
} else { | ||
throw new SQLException("Creating book failed, no ID obtained."); | ||
} | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create book " + book, e); | ||
} | ||
|
||
return book; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use PreparedStatement
over Statement
for the create
method, and save the query in a separate variable before the try block to maintain consistency and readability.
public Book create(Book book) { | ||
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
|
||
try (PreparedStatement statement = ConnectionUtil.getConnection().prepareStatement(sql, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should close the PreparedStatement
explicitly after use. The try-with-resources statement is already doing that, but it's a good practice to close it explicitly if you're going to reuse the connection.
int affectedRows = statement.executeUpdate(); | ||
|
||
if (affectedRows == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of checking if affectedRows == 0
, it's more flexible to check if affectedRows > 0
as it allows for batch updates where more than one row can be affected.
Book book = new Book(); | ||
book.setId(resultSet.getObject("id", Long.class)); | ||
book.setPrice(resultSet.getObject("price", BigDecimal.class)); | ||
book.setTitle(resultSet.getString("title")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using both setters and constructors for initializing objects. Choose one approach for consistency. Also, consider moving ResultSet processing into a separate private method to avoid code duplication.
while (resultSet.next()) { | ||
Book book = new Book(); | ||
book.setId(resultSet.getObject("id", Long.class)); | ||
book.setPrice(resultSet.getObject("price", BigDecimal.class)); | ||
book.setTitle(resultSet.getString("title")); | ||
books.add(book); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, consider moving ResultSet processing into a separate private method to avoid code duplication.
} | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create book " + book, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message is good, it's informative and includes the book that failed to be created.
} | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't find by id for id = " + id, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message is good, it's informative and includes the id that was not found.
new RuntimeException()); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't update book with id = " + book.getId(), e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message is good, it's informative and includes the id of the book that failed to update.
statement.setLong(1, id); | ||
return statement.executeUpdate() > 0; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't delete book with id = " + id, e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exception message is good, it's informative and includes the id of the book that failed to be deleted.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://localhost:3306/bookstore"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The DB_URL
is not complete. It should contain the full JDBC URL, including the database name and other required parameters for the connection.
import mate.academy.lib.Dao; | ||
import mate.academy.model.Book; | ||
|
||
@Dao |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dao |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without @dao annotation injector cant find class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotation Dao should be only under BookDaoImpl
@Dao | ||
public class BookDaoImpl implements BookDao { | ||
|
||
public Book create(Book book) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add Override annotation (check all methods)
|
||
int affectedRows = statement.executeUpdate(); | ||
|
||
if (affectedRows == 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (affectedRows == 0) { | |
if (affectedRows < 1) { |
int affectedRows = statement.executeUpdate(); | ||
|
||
if (affectedRows == 0) { | ||
throw new SQLException("Creating book failed, no rows affected."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw new SQLException("Creating book failed, no rows affected."); | |
throw new DataProcessingException("Creating book failed, no rows affected."); |
|
||
try (ResultSet generatedKeys = statement.getGeneratedKeys()) { | ||
if (generatedKeys.next()) { | ||
book.setId(generatedKeys.getLong(1)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
book.setId(generatedKeys.getLong(1)); | |
Long id = generatedKeys.getObject(1, Long.class); | |
book.setId(id); |
src/main/resources/init_db.sql
Outdated
USE BookStore; | ||
|
||
CREATE TABLE Books ( | ||
id BIGINT NOT NULL AUTO_INCREMENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id BIGINT NOT NULL AUTO_INCREMENT, | |
id BIGINT AUTO_INCREMENT, |
import mate.academy.lib.Dao; | ||
import mate.academy.model.Book; | ||
|
||
@Dao |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotation Dao should be only under BookDaoImpl
No description provided.