-
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
I did a task on working with JDBC. And implemented methods based on G… #421
base: main
Are you sure you want to change the base?
Conversation
…RUD principles!!!
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.
Hey there! 👋 It's great to see the effort you've put into following Java conventions and implementing the CRUD operations cleanly. However, there are a few improvements we can make to ensure your code is even more robust and maintainable. Let's enhance the exception messages to give more context when something goes wrong, and consider using PreparedStatement for all SQL queries to boost security and performance. Also, let's try to reduce code duplication by refactoring common logic into separate methods. Remember to manage resources like database connections carefully using try-with-resources, and keep sensitive information like database credentials out of the source code. Lastly, double-check that your database URL is complete. These changes will make your codebase solid and professional. Keep up the great work, and with these adjustments, your project will be top-notch! 🚀💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
import mate.academy.exception.DataProcessingException; | ||
|
||
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.
It looks like the DB_URL is not fully defined here. Make sure to provide the complete database URL string.
DB_PROPERTIES.put("user", "root"); | ||
DB_PROPERTIES.put("password", "Root1234"); |
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 credentials in code is a security risk. Consider using environment variables or a configuration file to keep sensitive information out of the source code.
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(DB_URL, DB_PROPERTIES); | ||
} |
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 best practices, it's recommended to use try-with-resources for getting the Connection to ensure that it is closed properly. This can prevent potential resource leaks.
book.setId(resultSet.getLong("id")); | ||
book.setTitle(resultSet.getString("title")); | ||
book.setPrice(resultSet.getBigDecimal("price")); |
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 are using setters to set the properties of the Book object after using the default constructor. According to the checklist, it is recommended to use either setters or the constructor to set all properties, but not both. Consider creating a constructor in the Book class that takes all the properties as parameters, or use the default constructor and setters consistently.
Book book = new Book(); | ||
book.setId(resultSet.getLong("id")); | ||
book.setTitle(resultSet.getString("title")); | ||
book.setPrice(resultSet.getBigDecimal("price")); |
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 code duplicates the logic of converting a ResultSet to a Book object which is present in both 'findById' and 'findAll' methods. To avoid code duplication, you could move this logic into a separate private method that constructs a Book object from a ResultSet and then call this method in both 'findById' and 'findAll'.
public List<Book> findAll() { | ||
List<Book> books = new ArrayList<>(); | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
Statement statement = connection.createStatement()) { |
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 instead of Statement for the 'findAll' method, even if the query does not have parameters. This is a best practice for several reasons, including better performance and security against SQL injection attacks.
return Optional.of(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't get book by 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 not informative enough. According to the checklist, it's recommended to include the id of the book that failed to be retrieved. For example, 'Can't get book by id ' + id.
int rowsAffected = statement.executeUpdate(); | ||
return rowsAffected > 0; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't delete book by 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.
Similarly, the exception message when deleting a book is not informative enough. Include the id of the book that failed to be deleted, for example, 'Can't delete book by ID ' + id.
return book; | ||
|
||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't insert book into 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 when inserting a book should be more informative. Include the book object that failed to be inserted, for example, 'Can't insert book ' + book + ' into DB'.
statement.executeUpdate(); | ||
return book; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't update 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 when updating a book should also be more informative. Include the book object that failed to be updated, for example, 'Can't update book ' + book.
…RUD principles!!!
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 your efforts so far! 😊 However, there are a few critical issues that need to be addressed before the pull request can be approved. Specifically, the handling of the DB_URL
needs correction to prevent connection issues, and the handling of the Book ID generation should be left to the database to ensure uniqueness. Also, please ensure that database credentials are securely managed and not hardcoded. 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 mate.academy.exception.DataProcessingException; | ||
|
||
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, which will cause a SQLException
when attempting to connect to the database. Make sure to provide the full JDBC URL in the format jdbc:mysql://<host>:<port>/<database_name>
.
DB_PROPERTIES.put("user", "root"); | ||
DB_PROPERTIES.put("password", "Root1234"); |
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.
Hardcoding the database credentials in the source code is not a secure practice. It's better to load these values from an external configuration file or environment variables.
try { | ||
Class.forName("com.mysql.cj.jdbc.Driver"); | ||
} catch (ClassNotFoundException e) { | ||
throw new DataProcessingException("Can not load JDBC driver", 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.
While it's good to load the JDBC driver class, consider adding an informative message about the JDBC driver that was not found to make the exception more helpful.
} | ||
} | ||
|
||
public static Connection getConnection() throws SQLException { |
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.
Do not use schema's name in the DB_URL
as you are configuring it while establishing a connection with the database. Ensure that the schema is specified in the URL if necessary, or configured through the DB_PROPERTIES
.
src/main/java/mate/academy/Main.java
Outdated
Book book = new Book(1L, "Java Programming", new BigDecimal("29.99")); | ||
Book book1 = new Book(2L, "Java", new BigDecimal("30.25")); |
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's not recommended to set the ID for the Book object manually when creating a new Book. The ID should be auto-generated by the database to ensure uniqueness and to prevent conflicts. Consider removing the ID parameter from the Book constructor and letting the database handle the ID generation.
@Override | ||
public Book create(Book book) { | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = | ||
connection.prepareStatement(CREATE_BOOK_QUERY, | ||
Statement.RETURN_GENERATED_KEYS)) { | ||
|
||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
statement.executeUpdate(); | ||
|
||
ResultSet resultSet = statement.getGeneratedKeys(); | ||
if (resultSet.next()) { | ||
book.setId(resultSet.getLong(1)); | ||
} | ||
return book; | ||
|
||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to insert book " + book + " into 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.
Use PreparedStatement
over Statement
for the CREATE_BOOK_QUERY
. Also, consider using Statement.RETURN_GENERATED_KEYS
only in the create
statement, which you are already doing correctly, but remember that it's not needed in other methods.
public Optional<Book> findById(Long id) { | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(FIND_BY_ID_QUERY)) { | ||
statement.setLong(1, id); | ||
ResultSet resultSet = statement.executeQuery(); | ||
if (resultSet.next()) { | ||
Book book = mapToBook(resultSet); | ||
return Optional.of(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to retrieve book with id " + id, e); | ||
} | ||
return Optional.empty(); |
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 if
or while
with resultSet.next()
to avoid a potential NullPointerException if the result set is empty. You are doing this correctly here, but keep it in mind for consistency across all methods.
public List<Book> findAll() { | ||
List<Book> books = new ArrayList<>(); | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(FIND_ALL_QUERY); | ||
ResultSet resultSet = statement.executeQuery()) { | ||
while (resultSet.next()) { | ||
books.add(mapToBook(resultSet)); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't get all 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.
It's good that you used PreparedStatement
over Statement
in the findAll()
method, following the best practice.
public boolean deleteById(Long id) { | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(DELETE_BY_ID_QUERY)) { | ||
statement.setLong(1, id); | ||
int rowsAffected = statement.executeUpdate(); | ||
return rowsAffected > 0; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to 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.
Don't return true
all the time in the deleteById
method. You're correctly returning a boolean value depending on the preparedStatement.executeUpdate()
result, which is a good practice.
Book mapToBook(ResultSet resultSet) throws SQLException { | ||
return new Book(resultSet.getLong("id"), | ||
resultSet.getString("title"), | ||
resultSet.getBigDecimal("price")); | ||
} |
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.
When converting ResultSet
to Book
, it's good practice to create an object using a constructor or setters, but not both. Here, you are using the constructor, which is consistent and recommended.
…RUD principles!!!
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 the progress! Remember to address the incomplete DB_URL and consider the feedback provided on using consistent object initialization, more informative exception messages, and SQL style conventions. Keep up the good work and make sure to refine your code according to the suggestions. You're doing well, and with these improvements, your code will be even better! Keep learning and coding! 🚀💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
import mate.academy.exception.DataProcessingException; | ||
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://localhost:3306/test?serverTimezone=UTC"; |
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 needs to be a full JDBC URL. For example: jdbc:mysql://localhost:3306/yourdatabasename?serverTimezone=UTC
private static final String CREATE_BOOK_QUERY | ||
= "INSERT INTO books (title, price) VALUES (?, ?)"; | ||
private static final String FIND_BY_ID_QUERY = "SELECT * FROM books WHERE id = ?"; | ||
private static final String FIND_ALL_QUERY = "SELECT * FROM books"; | ||
private static final String UPDATE_BOOK_QUERY | ||
= "UPDATE books SET title = ?, price = ? WHERE id = ?"; | ||
private static final String DELETE_BY_ID_QUERY = "DELETE FROM books WHERE id = ?"; |
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 to use uppercase for SQL keywords in your queries to follow SQL style best practices.
} | ||
} | ||
|
||
Book mapToBook(ResultSet resultSet) throws SQLException { |
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.
Consider moving the logic for mapping the ResultSet
to a Book
object into a separate private method to avoid code duplication as suggested in the checklist. This method should be used in findById
and findAll
methods.
} | ||
} | ||
|
||
Book mapToBook(ResultSet resultSet) throws SQLException { |
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.
When you are converting ResultSet
to Book
, it's better to use either setters or a constructor consistently rather than both. In this case, you are using the constructor to set title and price, and then using a setter to set the id. Choose one approach for consistency.
} | ||
return book; | ||
|
||
} catch (SQLException 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 exception messages. Instead of 'Failed to insert book into DB', include the book details to make the exception message more informative.
return Optional.of(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to retrieve 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.
Use informative exception messages. Instead of 'Failed to retrieve book with id', include the book id to make the exception message more informative.
statement.executeUpdate(); | ||
return book; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't update 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.
Use informative exception messages. Instead of 'Can't update book', include the book details to make the exception message more informative.
int rowsAffected = statement.executeUpdate(); | ||
return rowsAffected > 0; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to 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.
Use informative exception messages. Instead of 'Failed to delete book with id', include the book id to make the exception message more informative.
public Book(String title, BigDecimal price) { | ||
this.title = title; | ||
this.price = price; | ||
} |
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.
When you convert ResultSet
to Book
better create an object using setters or constructor but not both of them, because it's not consistent to use both ways of initialization of object. Since you have setters for all fields, consider removing the constructor that takes parameters and use setters instead, or add an id
parameter to the constructor if you want to initialize all fields at once.
…RUD principles!!!