-
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
jv-jdbc-intro #422
base: main
Are you sure you want to change the base?
jv-jdbc-intro #422
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.
It looks like you've done a great job overall, but there are a few important improvements that need to be made before we can merge this pull request. Let's make sure that all exception messages are informative and consistent with the checklist examples. Additionally, remember to use PreparedStatement
over Statement
for best practices, even in queries without parameters. Finally, double-check the logic where a book is being updated to ensure that the book exists in the database before attempting an update, and let the database handle ID generation for new books to avoid conflicts. Keep up the good work, and with these changes, your code will be even better! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/main/java/mate/academy/Main.java
Outdated
book.setId(1L); | ||
book.setPrice(new BigDecimal("100.00")); | ||
book.setTitle("Alice"); | ||
bookDao.create(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.
Creating a book with an already set ID may lead to conflicts since the ID should be auto-generated by the database. You should let the database handle ID generation to avoid potential issues.
src/main/java/mate/academy/Main.java
Outdated
if (foundBook.isPresent()) { | ||
Book bookToUpdate = foundBook.get(); | ||
bookToUpdate.setTitle("Alice in Wonderland"); | ||
bookToUpdate.setPrice(new BigDecimal("120.00")); | ||
bookDao.update(bookToUpdate); | ||
System.out.println("Book updated: " + bookToUpdate); | ||
} |
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.
This block of code is attempting to update a book that may not exist. Since 'foundBook' was retrieved before 'book' was created, it's possible that 'foundBook' is empty, and this block would be updating a non-existent book. You should ensure the book exists in the database before attempting to update it.
public Book create(Book book) { | ||
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement( | ||
sql, Statement.RETURN_GENERATED_KEYS)) { | ||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
if (statement.executeUpdate() < 1) { | ||
throw new RuntimeException("Expected to insert 1 record but inserted 0 rows"); | ||
} | ||
ResultSet generatedKeys = statement.getGeneratedKeys(); | ||
if (generatedKeys.next()) { | ||
Long id = generatedKeys.getObject(1, Long.class); | ||
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create 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
, even for a static query with no parameters in findAll()
method. It's the best practice, and it's slightly faster. Also, use informative messages for exceptions. Instead of 'Can't create book', it would be better to include the book that failed to be created, e.g., 'Can't create book ' + book.
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create 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 should be more informative. Follow the good practice example from the checklist: 'throw new DataProcessingException("Can't save a book " + book, e);'.
} | ||
return Optional.empty(); | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't find book from DB",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 should be more informative. Follow the good practice example from the checklist: 'throw new DataProcessingException("Can't get a book by id " + id, e);'.
public List<Book> findAll() { | ||
String sql = "SELECT * FROM books"; | ||
List<Book> books = new ArrayList<>(); | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(sql)) { | ||
ResultSet resultSet = statement.executeQuery(); | ||
while (resultSet.next()) { | ||
Long id = resultSet.getObject("id", Long.class); | ||
String title = resultSet.getObject("title", String.class); | ||
BigDecimal price = resultSet.getObject("price", BigDecimal.class); | ||
|
||
Book book = new Book(); | ||
book.setId(id); | ||
book.setPrice(price); | ||
book.setTitle(title); | ||
books.add(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't find books from DB",e); | ||
} | ||
return books; |
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.
This method should use PreparedStatement
instead of Statement
for consistency and best practices, even though there are no parameters in this query.
books.add(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't find books from DB",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 should be more informative and consistent with the checklist. Consider including the action being performed, e.g., 'throw new DataProcessingException("Can't find all books", e);'.
} | ||
return book; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't update book from DB",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 should be more informative. Follow the good practice example from the checklist: 'throw new DataProcessingException("Can't update book " + book, e);'.
return false; | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't delete book from DB",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 should be more informative. Follow the good practice example from the checklist: 'throw new DataProcessingException("Can't delete book by 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.
Great job on your Pull Request! You've followed the Java best practices pretty well and your classes are correctly implemented. Just remember to provide more informative exception messages and ensure that your database connection details are secure. Keep up the good work, and always strive to refine your code even further! 👍🎉
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
@Override | ||
public Book create(Book book) { | ||
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; |
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.
Remember about SQL style: use uppercase for SQL keywords in your queries.
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement( | ||
sql, Statement.RETURN_GENERATED_KEYS)) { |
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 Statement.RETURN_GENERATED_KEYS
only in create
statement, it's not needed in other methods.
String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement( | ||
sql, Statement.RETURN_GENERATED_KEYS)) { |
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
, even for a static query with no parameters in findAll()
method. It's the best practice, and it's slightly faster.
if (resultSet.next()) { | ||
String title = resultSet.getObject("title", String.class); | ||
BigDecimal price = resultSet.getObject("price", BigDecimal.class); | ||
|
||
Book book = new Book(); | ||
book.setId(id); | ||
book.setPrice(price); | ||
book.setTitle(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.
Try to avoid code duplication. Especially, when you are working with ResultSet. Move retrieving data from ResultSet into Entity object to a separate private method.
statement.setLong(1, id); | ||
ResultSet resultSet = statement.executeQuery(); | ||
if (resultSet.next()) { | ||
String title = resultSet.getObject("title", String.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.
Be attentive with retrieving the data from ResultSet: use resultSet.getObject("title", String.class)
instead of resultSet.getString("title")
to handle possible null
values correctly.
return false; | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't delete book by 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.
Use informative messages for exceptions. Include the id that caused the exception in the message.
|
||
public class ConnectionUtil { | ||
|
||
private static final String DB_URL = "jdbc:mysql://localhost:3306/test"; |
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 incomplete, it should contain the full JDBC URL to connect to the database. Make sure to specify the database name, and any other necessary configurations.
} | ||
|
||
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(DB_URL); |
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 pass the DB_PROPERTIES to the getConnection method to ensure the user and password are included in the connection.
private static final String DB_URL = "jdbc:mysql://localhost:3306/test"; | ||
private static final String USER = "root"; | ||
private static final String PASSWORD = "qwerty123"; |
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.
Storing database credentials directly in the code is a bad practice due to security reasons. It's better to use environment variables or configuration files that are not included in the version control.
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(DB_URL); | ||
} |
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.
According to the checklist, you should not use the schema's name in your queries. However, it's not clear if the DB_URL includes the schema name or not due to the incomplete URL. Make sure the schema name is not included in the DB_URL.
No description provided.