developed dao logic #427
developed dao logic #427
Conversation
There was a problem hiding this comment.
There are several critical issues that need to be addressed, such as incorrect table names in SQL statements and improper handling of Optional in the findById method. Additionally, storing database credentials directly in the code poses a security risk. Please address these issues to ensure the functionality and security of the application. Keep up the good work, and once these changes are made, your code will be in great shape! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.
| public static void main(String[] args) { | ||
| private static final Injector injector = Injector | ||
| .getInstance("mate.academy.lib"); | ||
| private static final BookDao dao = (BookDao) injector.getInstance(BookDao.class); |
There was a problem hiding this comment.
The BookDao interface specifies that the findById method should return an Optional<Book>, but the current code calls dao.findById(1L) without handling the Optional. Consider using Optional methods like ifPresent, orElse, or orElseThrow to handle the result properly.
| public class BookDaoImpl implements BookDao { | ||
|
|
||
| @Override | ||
| public Book create(Book book) { |
There was a problem hiding this comment.
In the create method, ensure that the SQL statement uses the correct table name books as specified in the task description. The current statement uses book, which may lead to errors if the table name is incorrect.
| } | ||
|
|
||
| @Override | ||
| public Optional<Book> findById(Long id) { |
There was a problem hiding this comment.
In the findById method, ensure that the SQL statement uses the correct table name books as specified in the task description. The current statement uses book, which may lead to errors if the table name is incorrect.
|
|
||
| @Override | ||
| public List<Book> findAll() { | ||
| String sqlFindAllStatement = "SELECT * FROM book"; |
There was a problem hiding this comment.
In the findAll method, ensure that the SQL statement uses the correct table name books as specified in the task description. The current statement uses book, which may lead to errors if the table name is incorrect.
| } | ||
|
|
||
| @Override | ||
| public Book update(Book book) { |
There was a problem hiding this comment.
In the update method, ensure that the SQL statement uses the correct table name books as specified in the task description. The current statement uses book, which may lead to errors if the table name is incorrect.
| @Override | ||
| public boolean deleteById(Long id) { | ||
| int idPlace = 1; | ||
| String sqlDeleteStatement = "DELETE FROM book WHERE id = ? "; |
There was a problem hiding this comment.
In the deleteById method, ensure that the SQL statement uses the correct table name books as specified in the task description. The current statement uses book, which may lead to errors if the table name is incorrect.
| import mate.academy.exception.DataProcessingException; | ||
|
|
||
| public class ConnectionUtils { | ||
| private static final String DB_URL = "jdbc:mysql://localhost:3306/test"; |
There was a problem hiding this comment.
Ensure that the database URL in DB_URL is correct and matches the schema you have set up for this task. The current URL points to jdbc:mysql://localhost:3306/test, which may not be the intended schema for your application.
| DB_PROPERTIES.put("user", "root"); | ||
| DB_PROPERTIES.put("password", "4321"); |
There was a problem hiding this comment.
Storing database credentials directly in the code is not a secure practice. Consider externalizing these properties to a configuration file or environment variables to enhance security.
| import java.util.Objects; | ||
|
|
||
| public class Book { | ||
| private long id; |
There was a problem hiding this comment.
The id field should be of type Long instead of long to match the task description and to allow for null values when the ID is not yet assigned.
There was a problem hiding this comment.
Great job on implementing the core functionalities! 🎉 While the code is generally well-structured and follows good practices, there are a few areas for improvement. Pay attention to the comparison of Long objects in the equals method, enhance security by managing database credentials more securely, and consider logging connection attempts for better traceability. Also, ensure consistency in error handling by using specific exceptions. Keep up the good work and use these suggestions to refine your skills further!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| statement.setLong(idPlace,book.getId()); | ||
| statement.setString(titlePlace,book.getTitle()); | ||
| statement.setBigDecimal(pricePlace,book.getPrice()); |
There was a problem hiding this comment.
Consider setting the parameters in the order they appear in the SQL statement to improve readability. This means setting titlePlace and pricePlace before idPlace, as they appear first in the SQL statement.
| if (affectedRows > 0) { | ||
| return book; | ||
| } else { | ||
| throw new RuntimeException("Failed to update book with id: " + book.getId()); |
There was a problem hiding this comment.
Instead of throwing a generic RuntimeException, consider using a more specific exception type or the existing DataProcessingException to maintain consistency in error handling.
| private static final String PASSWORD = "4321"; | ||
| private static final String USER = "root"; |
There was a problem hiding this comment.
Storing database credentials directly in the code is not secure. Consider using environment variables or a secure vault to manage sensitive information like database passwords.
| } | ||
|
|
||
| public static Connection getConnection() throws SQLException { | ||
| return DriverManager.getConnection(DB_URL,DB_PROPERTIES); |
There was a problem hiding this comment.
It's a good practice to log the connection attempts and any exceptions that occur during the connection process for better traceability and debugging.
| return false; | ||
| } | ||
| Book book = (Book) o; | ||
| return id == book.id && Objects |
There was a problem hiding this comment.
When comparing Long objects, it's safer to use Objects.equals(id, book.id) instead of id == book.id to avoid potential issues with object references and null values.
No description provided.