-
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
implemented dao and model for book #402
base: main
Are you sure you want to change the base?
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.
Hi there! 👋 It looks like you've put in a lot of effort into your code, and you're on the right track! However, there are a few important areas that need a bit of tweaking before we can move forward:
- Security: Let's make sure to keep sensitive data like database credentials secure by using environment variables or a configuration file.
- Code correctness: There are some issues with how null values and result sets are handled, as well as the return values of some methods not reflecting the actual operation result.
- Best practices: Using uppercase for SQL keywords and
PreparedStatement
for all queries, even static ones, will improve readability and consistency.
Don't get discouraged, these changes are part of the learning process and will make your code much more robust and professional. Keep up the great work, and I look forward to seeing your revised submission! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
new Book(null, "Book1", new BigDecimal(100)), | ||
new Book(null, "Book2", new BigDecimal(150)), | ||
new Book(null, "Book3", new BigDecimal(50)) |
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 Book
objects with null
id is a bit misleading since it's not immediately clear why the id is set to null
. If the id
is meant to be generated by the database, then it would be better to have a constructor that does not require an id
parameter at all, which would make the intent clearer.
bookDao.findAll().forEach(System.out::println); | ||
System.out.println(); | ||
|
||
Book anyBook = books.stream().findAny().orElseThrow(); |
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.
Using findAny()
without any filtering or additional context is non-deterministic and may lead to inconsistent behavior during different runs of the application. If the goal is to update a random book, consider adding a comment to clarify this choice. Otherwise, use a more deterministic approach, such as selecting the first element or using a specific criterion for selection.
String query = "INSERT INTO books(title, price) VALUES (?, ?)"; | ||
try (Connection connection = ConnectionUtil.getConnection()) { | ||
PreparedStatement statement = | ||
connection.prepareStatement(query, Statement.RETURN_GENERATED_KEYS); | ||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
int affectedRows = statement.executeUpdate(); |
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 as mentioned in the checklist to avoid code duplication.
try (Connection connection = ConnectionUtil.getConnection()) { | ||
PreparedStatement statement = | ||
connection.prepareStatement(query, 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.
Use PreparedStatement
over Statement
for static queries with no parameters in findAll()
method. Although prepareStatement
is used here, it's a good practice to be consistent across all methods.
ResultSet resultSet = statement.getGeneratedKeys(); | ||
if (resultSet.next()) { | ||
Long id = resultSet.getLong(1); | ||
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.
Instead of using resultSet.getLong(1)
, it's better to use resultSet.getObject(1, Long.class)
to handle possible null
values correctly.
try (Connection connection = ConnectionUtil.getConnection()) { | ||
PreparedStatement statement = connection.prepareStatement(query); | ||
statement.setLong(1, id); | ||
return statement.executeUpdate() > 0; |
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.
Instead of returning true
all the time, let's return a boolean value depending on the preparedStatement.executeUpdate()
result. This makes the method's behavior more accurate to whether the deletion actually happened.
} | ||
|
||
private Book getBookFromResultSet(ResultSet resultSet) throws SQLException { | ||
Long id = resultSet.getObject("id", Long.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.
Use resultSet.getObject("id", Long.class)
to handle possible null
values correctly.
|
||
public class ConnectionUtil { | ||
private static final Properties DB_PROPERTIES; | ||
private static final String URL = "jdbc:mysql://localhost:3306/mate"; |
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 URL seems to be incomplete. It should specify the database that you are connecting to. For example, 'jdbc:mysql://localhost:3306/your_database_name'.
private static final String USER = "root"; | ||
private static final String PASSWORD = "password"; |
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 database username and password as plain text in the source code is not secure. Consider using environment variables or a configuration file to store sensitive information securely.
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(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.
It is good practice to handle the SQLException
that might be thrown from getConnection
method and convert it to a more informative custom exception, similar to what you have done in the static block with ClassNotFoundException
.
No description provided.