hw is done, please check #420
Conversation
There was a problem hiding this comment.
It looks like you've put in a solid effort on this project, and you're almost there! However, there are a few critical issues that need to be addressed before we can merge this Pull Request. The missing executeUpdate for the update operation and the incorrect use of RETURN_GENERATED_KEYS for SELECT queries are particularly important to fix, as they will affect the functionality of your application. Also, make sure to correct the exception messages to provide more accurate information, which is essential for debugging and maintenance. Keep up the good work, and with a few adjustments, your code will be ready to go! 👍💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| import java.util.Properties; | ||
|
|
||
| public class ConnectionUtil { | ||
| private static final String DB_URL = "jdbc:mysql://localhost:3306/jdbc_intro"; |
There was a problem hiding this comment.
The DB_URL string is not complete, it should contain the full JDBC URL, including the database server's IP, port, and the specific database name you want to connect to.
| public class ConnectionUtil { | ||
| private static final String DB_URL = "jdbc:mysql://localhost:3306/jdbc_intro"; | ||
| private static final String USERNAME = "root"; | ||
| private static final String PASSWORD = "Nk102030!"; |
There was a problem hiding this comment.
Storing passwords in source code is a security risk. Consider using environment variables or a configuration file for sensitive data.
| public class BookDaoImpl implements BookDao { | ||
| @Override | ||
| public Book create(Book book) { | ||
| String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; |
There was a problem hiding this comment.
According to the checklist, don't use schema's name in your queries. You should configure the schema while establishing a connection with the DB, so there is no need to specify it in the SQL query.
| if (affectedRows < 0) { | ||
| throw new DataProcessingException("Expected to insert at least one row," | ||
| + " but inserted 0 rows"); |
There was a problem hiding this comment.
The executeUpdate method returns the number of rows affected by the query. It will return 0 if no rows were updated, not a negative number. You should check if affectedRows == 0 to determine if no rows were inserted.
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can' add a new book: " + book); |
There was a problem hiding this comment.
Use more informative messages for exceptions. Include details like the book id when an exception occurs during insertion to help with debugging. For example: "Can't add a new book with id: " + book.getId().
| statement.setString(1, book.getTitle()); | ||
| statement.setBigDecimal(2, book.getPrice()); | ||
| statement.setLong(3, book.getId()); | ||
| return book; |
There was a problem hiding this comment.
You're missing the statement.executeUpdate() call to actually perform the update operation on the database. Without it, the update will not be executed.
| statement.setLong(3, book.getId()); | ||
| return book; | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can't update book: " + book); |
There was a problem hiding this comment.
Use a more informative exception message. Include the book id when an exception occurs during an update. For example: "Can't update the book with id: " + book.getId().
| statement.setLong(1, id); | ||
| return statement.executeUpdate() > 0; | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can't delete book"); |
There was a problem hiding this comment.
Use a more informative exception message. Include the id of the book when an exception occurs during deletion. For example: "Can't delete the book with id: " + id.
| private Book getBookFromResultSet(ResultSet resultSet) throws SQLException { | ||
| Long id = resultSet.getLong("id"); | ||
| String title = resultSet.getString("title"); | ||
| BigDecimal price = resultSet.getBigDecimal("price"); | ||
| return new Book(id, title, price); |
There was a problem hiding this comment.
It's good practice to check if the values retrieved from the ResultSet are not null before using them to create a new object. Use resultSet.getObject for 'id' and check if it's not null before creating the Book object.
| public static Object getInstance(Class<?> certainInterface) { | ||
| Class<?> clazz = findClassExtendingInterface(certainInterface); | ||
| return createInstance(clazz); | ||
| } |
There was a problem hiding this comment.
The method 'getInstance' is a bit misleading because it suggests that you can get an instance of any class, while it actually expects an interface that has an implementation marked with the @dao annotation. Consider renaming this method to reflect that it's specifically for getting Dao implementations, such as 'getDaoInstance'.
There was a problem hiding this comment.
Hey there! It seems like we have a few areas to improve, especially with SQL style and exception handling. Let's make sure SQL keywords are in uppercase to follow the conventions, and work on catching specific exceptions rather than a generic one. Also, consider using PreparedStatement for all queries and refining the exception messages to be more informative. 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! ✨
| public class BookDaoImpl implements BookDao { | ||
| @Override | ||
| public Book create(Book book) { | ||
| String sql = "INSERT INTO books (title, price) VALUES (?, ?)"; |
There was a problem hiding this comment.
Use uppercase for SQL keywords to follow SQL style conventions. For example, use 'INSERT INTO books (title, price) VALUES (?, ?)'.
| book.setId(id); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can' add a new book with id: " + book.getId()); |
There was a problem hiding this comment.
Use informative messages for exceptions. Include the book object in the message to provide more context. For example, 'Can't add a new book: ' + book.
|
|
||
| @Override | ||
| public Optional<Book> findById(Long id) { | ||
| String sql = "SELECT * FROM books WHERE id = ?"; |
There was a problem hiding this comment.
Use uppercase for SQL keywords to follow SQL style conventions. For example, use 'SELECT * FROM books WHERE id = ?'.
| @Override | ||
| public List<Book> findAll() { | ||
| List<Book> listOfBooks = new ArrayList<>(); | ||
| String sql = "SELECT * FROM books;"; |
There was a problem hiding this comment.
Use uppercase for SQL keywords to follow SQL style conventions. For example, use 'SELECT * FROM books'. Also, remove the semicolon at the end of the query string as it is not necessary in JDBC.
| List<Book> listOfBooks = new ArrayList<>(); | ||
| String sql = "SELECT * FROM books;"; | ||
| try (Connection connection = ConnectionUtil.getConnection(); | ||
| PreparedStatement statement = connection.prepareStatement(sql)) { |
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public Book update(Book book) { | ||
| String sql = "UPDATE books SET title = ?, price = ? WHERE id = ?"; |
There was a problem hiding this comment.
Use uppercase for SQL keywords to follow SQL style conventions. For example, use 'UPDATE books SET title = ?, price = ? WHERE id = ?'.
|
|
||
| @Override | ||
| public boolean deleteById(Long id) { | ||
| String sql = "DELETE FROM books WHERE id = ?"; |
There was a problem hiding this comment.
Use uppercase for SQL keywords to follow SQL style conventions. For example, use 'DELETE FROM books WHERE id = ?'.
| private Book getBookFromResultSet(ResultSet resultSet) throws SQLException { | ||
| Long id = resultSet.getLong("id"); | ||
| String title = resultSet.getString("title"); | ||
| BigDecimal price = resultSet.getBigDecimal("price"); | ||
| return new Book(id, title, price); |
There was a problem hiding this comment.
To avoid code duplication, move retrieving data from ResultSet into the Book entity object to a separate private method.
| } | ||
|
|
||
| private Book getBookFromResultSet(ResultSet resultSet) throws SQLException { | ||
| Long id = resultSet.getLong("id"); |
There was a problem hiding this comment.
Use 'resultSet.getObject("id", Long.class)' instead of 'resultSet.getLong("id")' to handle null values correctly.
There was a problem hiding this comment.
Great job on demonstrating the CRUD operations in the Main class and adhering to Java conventions in your classes and interfaces! 👍 Remember to address the mentioned issues to ensure your code is robust and follows best practices. Keep up the good work, and make sure to review the comments to further enhance your code. It's important to continue refining your skills and learning from feedback. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
|
||
| public class ConnectionUtil { | ||
| private static String password; | ||
| private static final String DB_URL = "jdbc:mysql://localhost:3306/jdbc_intro"; |
There was a problem hiding this comment.
The DB_URL seems to be incomplete. Make sure to provide the full database URL including the database name, parameters, and avoid using schema names directly in the URL as per the checklist.
| try (BufferedReader bufferedReader = Files.newBufferedReader(Paths.get(PASSWORD_PATH))) { | ||
| password = bufferedReader.readLine(); | ||
| } catch (IOException e) { | ||
| throw new RuntimeException("Can't read file", e); |
There was a problem hiding this comment.
Consider using a more informative message for the exception that includes the path to the file that couldn't be read.
| try { | ||
| Class.forName("com.mysql.cj.jdbc.Driver"); | ||
| } catch (ClassNotFoundException e) { | ||
| throw new RuntimeException("Can not load JDBC driver", e); |
There was a problem hiding this comment.
For the exception message, include the name of the JDBC driver that could not be loaded to provide more context.
| Book book = new Book(); | ||
| book.setTitle("NewTitle"); | ||
| book.setPrice(BigDecimal.valueOf(40)); | ||
| Book bookUpdated = bookDao.create(book); |
There was a problem hiding this comment.
Ensure that the 'create' method in 'BookDao' uses 'Statement.RETURN_GENERATED_KEYS' to retrieve and set the generated id for the book. This is crucial for the subsequent findById operation to work correctly.
| Book bookUpdated = bookDao.create(book); | ||
| System.out.println("Created: " + bookUpdated); | ||
|
|
||
| Optional<Book> findById = bookDao.findById(book.getId()); |
There was a problem hiding this comment.
When you retrieve a book using 'findById', it's good practice to check if the book is present before trying to use it. Use 'findById.isPresent()' to check if the book was found.
| public Optional<Book> findById(Long id) { | ||
| String sql = "SELECT * FROM books WHERE id = ?"; | ||
| try (Connection connection = ConnectionUtil.getConnection(); | ||
| PreparedStatement statement = connection.prepareStatement(sql)) { | ||
| statement.setLong(1, id); | ||
| ResultSet resultSet = statement.executeQuery(); | ||
|
|
||
| if (resultSet.next()) { | ||
| return Optional.of(getBookFromResultSet(resultSet)); | ||
| } | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can't find a book with id: " + id); | ||
| } | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Use resultSet.getObject() to retrieve the id from the ResultSet to handle null values correctly, as per the checklist.
| @Override | ||
| public List<Book> findAll() { | ||
| List<Book> listOfBooks = new ArrayList<>(); | ||
| String sql = "SELECT * FROM books;"; |
There was a problem hiding this comment.
SQL keywords should be written in uppercase for consistency and readability, as per the checklist.
| List<Book> listOfBooks = new ArrayList<>(); | ||
| String sql = "SELECT * FROM books;"; | ||
| try (Connection connection = ConnectionUtil.getConnection(); | ||
| PreparedStatement statement = connection.prepareStatement(sql)) { |
There was a problem hiding this comment.
Even though the findAll method does not have parameters, it is still recommended to use PreparedStatement over Statement as per the checklist.
| statement.executeUpdate(); | ||
| return book; | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can't update the book with id: " + book.getId()); |
There was a problem hiding this comment.
The error message in the DataProcessingException should be more informative. Include the book's details in the message to provide more context about the failure.
| statement.setLong(1, id); | ||
| return statement.executeUpdate() > 0; | ||
| } catch (SQLException e) { | ||
| throw new DataProcessingException("Can't delete the book with id: " + id); |
There was a problem hiding this comment.
The error message in the DataProcessingException should be more informative. Include the id of the book in the message to provide more context about the failure.
No description provided.