-
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
Devolepment #412
base: main
Are you sure you want to change the base?
Devolepment #412
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.
Great job on following the Java conventions and implementing the necessary classes and interfaces! 🌟 However, there are a few important improvements to make regarding exception handling and best practices. Please take a look at the comments for details on how to enhance your code. Remember, clear and informative exception messages are crucial for debugging and maintenance, and using PreparedStatement
consistently can help prevent SQL injection attacks. Keep up the good work, and with these adjustments, your code will be even better! 💪
✨ 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 (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = | ||
connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) { | ||
|
||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
|
||
int affectedRows = statement.executeUpdate(); | ||
if (affectedRows < 1) { | ||
throw new RuntimeException("Expected to insert at least one row " | ||
+ ", 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 RuntimeException("can not add new book to DB: " + 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.
It's recommended to use informative exception messages. Instead of throwing a generic RuntimeException
in the create
method, throw a DataProcessingException
with a message that includes the book details for better context.
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = | ||
connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) { | ||
|
||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
|
||
int affectedRows = statement.executeUpdate(); | ||
if (affectedRows < 1) { | ||
throw new RuntimeException("Expected to insert at least one row " | ||
+ ", but inserted 0 rows."); | ||
} | ||
|
||
ResultSet generatedKeys = statement.getGeneratedKeys(); | ||
if (generatedKeys.next()) { | ||
Long id = generatedKeys.getObject(1, Long.class); | ||
book.setId(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.
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.
} catch (SQLException e) { | ||
throw new RuntimeException("can not add new book to DB: " + 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 DataProcessingException
instead of RuntimeException
to maintain consistency across the DAO layer when handling SQL exceptions.
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 result = statement.executeQuery(); | ||
if (result.next()) { | ||
String title = result.getString("title"); | ||
BigDecimal price = result.getObject("price", BigDecimal.class); | ||
Book book = new Book(); | ||
book.setId(id); | ||
book.setTitle(title); | ||
book.setPrice(price); | ||
return Optional.of(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new RuntimeException("Cannot create a connection to the DB", 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.
Move the logic for retrieving a Book
from a ResultSet
into a separate private method to avoid code duplication. This applies to the findById
method.
} catch (SQLException e) { | ||
throw new RuntimeException("Cannot create a connection to the 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 here is misleading. It should indicate that the retrieval of the book by id failed, not the creation of the connection. Use DataProcessingException
with a more informative message, such as 'Can't get a book by id ' + id.
private Book getBookFromResultSet(ResultSet resultSet) throws SQLException { | ||
Long id = resultSet.getObject("id", Long.class); | ||
String title = resultSet.getString("title"); | ||
BigDecimal price = resultSet.getObject("price", BigDecimal.class); | ||
|
||
Book book = new Book(); | ||
book.setId(id); | ||
book.setTitle(title); | ||
book.setPrice(price); | ||
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.
The method getBookFromResultSet
is correctly extracting the book from the result set. However, ensure that the findAll
method uses PreparedStatement
over Statement
as per best practices.
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
, it's better to create an object using setters or constructor but not both of them. Currently, you have a constructor that sets the title and price, but the id is set via a setter. Consider adding an id
parameter to the constructor or initializing all fields using setters.
return "Car(" | ||
+ "id=" + id | ||
+ ", title=" + title | ||
+ ", 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.
There is a typo in the toString
method. You're returning a string that starts with "Car(" instead of "Book(". This should be corrected to accurately represent the Book
object.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://127.0.0.1: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. For example, "jdbc:mysql://localhost:3306/myDatabase", where myDatabase
is the name of your database.
DB_PROPERTIES.put("user","root"); | ||
DB_PROPERTIES.put("password", "Nero0410"); |
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 the code is a bad practice due to security reasons. It's better to use environment variables or a configuration file that is not included in your version control.
after attemt 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.
Hey there! It looks like you've put in a lot of effort, and you're definitely on the right track. However, I noticed a few critical issues that should be addressed to make sure your code is secure and robust. For instance, hardcoding credentials can be a security risk, and not properly closing database connections could lead to resource leaks. Also, making sure your SQL queries are clear and using PreparedStatement
can help prevent SQL injection attacks. Don't worry, you're doing great! Just make these small tweaks, and your code will be even better. Keep up the good work! 👍🔒💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("can not add new book to DB: " + 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 a more informative message for the exception that includes the book's details. For example: 'Can't save a book ' + book
return Optional.of(getBookFromResultSet(resultSet)); | ||
} | ||
} catch (SQLException e) { | ||
throw new RuntimeException("Cannot create a connection to the 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 specific. It's better to use 'Can't get a book by id ' + id instead of a generic 'Cannot create a connection to the DB'.
+ book.getId()); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create a connection to the DB"); |
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 'Can't create a connection to the DB' is misleading here. It should be more informative and specific to the operation being performed, like 'Can't update the book with id: ' + book.getId().
public List<Book> findAll() { | ||
List<Book> books = new ArrayList<>(); | ||
String sql = "SELECT * FROM books"; | ||
try (Connection connection = ConnectionUtil.getConnection(); |
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.
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.
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. For example, 'Can't delete the book with id: ' + id.
|
||
@Override | ||
public boolean deleteById(Long id) { | ||
String sql = "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.
Use uppercase for SQL keywords in your queries. For example, use 'DELETE FROM' instead of 'delete from'.
} | ||
} | ||
|
||
private Book getBookFromResultSet(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 retrieving data from the ResultSet into a separate private method to avoid code duplication.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://127.0.0.1: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 specify the database server's URL, the port, and the database name you want to connect to.
String userName = "root"; | ||
String password = "Nero0410"; |
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 credentials in the source code is a security risk. Consider using environment variables or a configuration file to store sensitive information securely.
} | ||
|
||
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.
The getConnection method should use try-with-resources or ensure that the Connection is closed properly elsewhere in the code to prevent potential resource leaks.
Attempt 2 |
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 implementing the main class and handling CRUD operations via the BookDao interface! 🌟 Your use of BigDecimal for monetary values is spot on. Just remember to handle exceptions gracefully, secure sensitive information like database passwords, and refine your use of constructors and setters. Keep up the good work, and remember that addressing the feedback will help you write even more robust and secure code! 👍🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
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. However, in this case, you are already using PreparedStatement
, so just ensure you do so consistently across all methods.
PreparedStatement statement = | ||
connection.prepareStatement(sql, Statement.RETURN_GENERATED_KEYS)) { | ||
|
||
statement.setString(1, book.getTitle()); |
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 avoid code duplication. Since you're setting parameters for PreparedStatement
in multiple methods (create
, update
, etc.), consider creating a private method to handle this.
Long id = generatedKeys.getObject(1, Long.class); | ||
book.setId(id); | ||
} | ||
} 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 messages for exceptions. Instead of a generic message, include the book's details for better debugging. For example: "Can't save a book: " + book
return Optional.of(getBookFromResultSet(resultSet)); | ||
} | ||
} catch (SQLException e) { | ||
throw new RuntimeException("Can't get a 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 book's ID for better debugging. For example: "Can't get a book by id: " + id
books.add(getBookFromResultSet(resultSet)); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't get all books from the database"); |
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 more details if possible for better debugging.
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.
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 book's ID for better debugging. For example: "Can't delete the book with id: " + id
} | ||
} | ||
|
||
private Book getBookFromResultSet(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 converting ResultSet
to Book
, choose either setters or constructor for object creation, but not both. Since you are using setters here, consider creating a constructor in Book
class that takes id
, title
, and price
as parameters and use it here.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://127.0.0.1:3306/test?user=root"; |
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 include the JDBC URL specifying the database to connect to, for example 'jdbc:mysql://localhost:3306/yourDatabaseName'.
private static final String DB_URL = "jdbc:mysql://127.0.0.1:3306/test?user=root"; | ||
private static final Properties DB_PROPERTIES; | ||
private static final String USERNAME = "root"; | ||
private static final String PASSWORD = "Nero0410"; |
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 the password in the source code is a security risk. Consider using environment variables or a configuration file that is not included in the version control system.
} | ||
} | ||
|
||
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.
It's good practice to use try-with-resources or closing the connection explicitly in the calling code to prevent potential resource leaks. Ensure that connections are closed properly after use.
Attempt 3 |
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.
one comment
src/main/resources/init_db.sql
Outdated
@@ -0,0 +1,5 @@ | |||
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, |
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! 🔥
Was added DAO and N-tier architecture and checked in main presentation