-
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
HM JDBC #418
base: main
Are you sure you want to change the base?
HM JDBC #418
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.
Hey there! 😊 It looks like there are a few critical issues that need addressing before we can merge your Pull Request. Specifically, the database URL needs to be complete to ensure the application can connect to the database, and the deleteById
method should reflect the actual result of the operation. Additionally, remember to use consistent naming for your tables according to the SQL style guide. Keep up the great work, and once these changes are made, your code will be in excellent shape! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
|
||
@Override | ||
public int getRowsCount() { | ||
String sql = "SELECT COUNT(*) AS total_rows FROM 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 table name should be books
instead of book
according to the SQL style guide in the checklist.
|
||
@Override | ||
public boolean clear() { | ||
String sql = "TRUNCATE TABLE 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 table name should be books
instead of book
according to the SQL style guide in the checklist.
try (Connection connection = ConnectionUtil.getConnection(); | ||
Statement statement = connection.createStatement()) { | ||
int affectedRows = statement.executeUpdate(sql); | ||
return affectedRows >= getRowsCount(); |
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 getRowsCount()
after truncating the table to check if the operation was successful is incorrect, as the row count will always be 0 after a truncate operation. You should instead check if affectedRows
is not negative to indicate success.
|
||
@Override | ||
public Book create(Book book) { | ||
String sql = "INSERT INTO book(title, price) VALUES(?, ?)"; |
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 table name should be books
instead of book
according to the SQL style guide in the checklist.
public Book create(Book book) { | ||
String sql = "INSERT INTO book(title, price) VALUES(?, ?)"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection |
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.
Please use PreparedStatement instead of Statement for this query as per the best practices mentioned in the checklist.
|
||
@Override | ||
public boolean deleteById(Long id) { | ||
String sql = "DELETE FROM book 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.
The table name should be books
instead of book
according to the SQL style guide in the checklist. Also, for the deleteById
method, you should return a boolean value depending on preparedStatement.executeUpdate()
result, not just true
all the time.
import java.util.Properties; | ||
|
||
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 and missing the actual database URL. It should be a full JDBC URL of the form jdbc:mysql://:/<database_name>, for example: "jdbc:mysql://localhost:3306/my_database".
private Book(long id, String title, int price) { | ||
this.id = id; | ||
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.
It's not consistent to use both a constructor and setters for object initialization. Since you have setters for all fields, consider using a no-argument constructor and initializing fields through setters, or remove the setters and use only the constructor for initialization.
private Book(String title, int 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.
The overloaded private constructor is not necessary if you're using factory methods. You can have a single private constructor and handle object creation entirely through the static factory methods of
.
public static Book of(long id, String title, int price) { | ||
return new Book(id, title, price); | ||
} | ||
|
||
public static Book of(String title, int price) { | ||
return new Book(title, 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.
According to the checklist, when converting a ResultSet to a Book object, it's better to create an object using setters or a constructor but not both. Since you have provided factory methods, consider removing the setters and making the fields final to ensure immutability of the Book objects once created.
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 BookDao interface with CRUD operations! 👏 Remember to consistently use PreparedStatement
for all your queries, manage resources effectively by closing ResultSet
objects, and handle the results of update and delete operations properly. Keep up the good work, and always consider security best practices, like not storing sensitive information in your code. Keep learning and coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
public boolean clear() { | ||
String sql = "TRUNCATE TABLE books"; | ||
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
over Statement
, even for a static query with no parameters in clear()
method. It's the best practice, and it's slightly faster.
try (Connection connection = ConnectionUtil.getConnection(); | ||
Statement statement = connection.createStatement()) { | ||
statement.executeUpdate(sql); | ||
return true; |
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 boolean value depending on statement.executeUpdate()
result.
|
||
@Override | ||
public Book create(Book book) { | ||
String sql = "INSERT INTO books(title, price) VALUES(?, ?)"; |
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 Statement.RETURN_GENERATED_KEYS
only in create
statement. Since this is the create method, it's correctly used here. However, ensure it's not used in other methods where it's not needed.
ResultSet generatedKeys = preparedStatement.getGeneratedKeys(); | ||
if (generatedKeys.next()) { | ||
long id = generatedKeys.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.
The ResultSet
for generated keys should be closed to avoid any potential resource leaks. You can include it in the try-with-resources statement.
String sql = "SELECT * FROM books WHERE id = ?"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement preparedStatement = connection | ||
.prepareStatement(sql)) { |
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
. Since you're already using PreparedStatement
, this is just a reminder to maintain this practice.
import java.util.Properties; | ||
|
||
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. It should contain the full JDBC URL to the database, including the host, port, and database name. For example: jdbc:mysql://localhost:3306/myDatabase
|
||
public class ConnectionUtil { | ||
private static final String DB_URL = "jdbc:mysql://localhost:3306/test"; | ||
private static final Properties 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 not secure to store database credentials in code. It's better to use environment variables or a configuration file that is not included in the version control.
private Book(long id, String title, int price) { | ||
this.id = id; | ||
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.
It's not consistent to use both ways of initialization of an object. Since you have setters for all fields, you can remove the private constructor and create objects using setters or create a public constructor that initializes all fields.
private Book(String title, int 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.
This constructor should be removed to maintain consistency as mentioned in the previous comment.
public static Book of(long id, String title, int price) { | ||
return new Book(id, title, price); | ||
} | ||
|
||
public static Book of(String title, int price) { | ||
return new Book(title, 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.
Factory methods 'of' are generally used to create instances without using the 'new' keyword directly. However, these should be used consistently. If you decide to keep the factory methods, you should also ensure that all objects are created through them for consistency.
import mate.academy.model.Book; | ||
|
||
public interface BookDao { | ||
|
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.
int getRowsCount(); | ||
|
||
boolean clear(); |
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.
int getRowsCount(); | |
boolean clear(); |
|
||
@Dao | ||
public class BookDaoImpl implements BookDao { | ||
|
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.
src/main/java/mate/academy/Main.java
Outdated
|
||
bookDao.findById(3L).ifPresent(System.out::println); | ||
|
||
boolean b = bookDao.deleteById(3L); |
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.
rename
Statement statement = connection.createStatement(); | ||
ResultSet resultSet = statement.executeQuery(sql)) { | ||
while (resultSet.next()) { | ||
long id = resultSet.getLong("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.
long id = resultSet.getLong("id"); | |
Long id = resultSet.getLong("id"); |
preparedStatement.setLong(1, id); | ||
ResultSet resultSet = preparedStatement.executeQuery(); | ||
if (resultSet.next()) { | ||
String title = resultSet.getString("title"); |
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 separate method to map resultSet to book
package mate.academy.model; | ||
|
||
public class Book { | ||
private long 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.
private long id; | |
private Long id; |
public class Book { | ||
private long id; | ||
private String title; | ||
private int 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.
private int price; | |
private BigDecimal price; |
src/main/resources/init_db.sql
Outdated
@@ -0,0 +1,6 @@ | |||
CREATE TABLE book | |||
( | |||
id INT NOT NULL PRIMARY KEY 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 INT NOT NULL PRIMARY KEY AUTO_INCREMENT, | |
id BIGINT PRIMARY KEY AUTO_INCREMENT, |
src/main/resources/init_db.sql
Outdated
( | ||
id INT NOT NULL PRIMARY KEY AUTO_INCREMENT, | ||
title VARCHAR(100) NOT NULL, | ||
price INT NOT NULL |
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.
price INT NOT NULL | |
price DECIMAL NOT NULL |
import mate.academy.model.Book; | ||
|
||
public interface BookDao { | ||
|
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.
remove 8 line
if (storeBook(preparedStatement, book, true)) { | ||
throw new RuntimeException("Book not found"); | ||
} |
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.
if (storeBook(preparedStatement, book, true)) { | |
throw new RuntimeException("Book not found"); | |
} | |
statement.setString(1, book.getTitle()); | |
statement.setBigDecimal(2, book.getPrice()); | |
statement.setLong(3, book.getId()); | |
if (statement.executeUpdate() < 1) { | |
throw new DataProcessingException("Update failed for book with id:" + book.getId()); | |
} |
return Book.of(id, title, price); | ||
} | ||
|
||
private boolean storeBook(PreparedStatement preparedStatement, |
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.
remove
if (storeBook(preparedStatement, book, false)) { | ||
throw new RuntimeException("Book could not be created"); | ||
} |
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.
if (storeBook(preparedStatement, book, false)) { | |
throw new RuntimeException("Book could not be created"); | |
} | |
statement.setString(1, book.getTitle()); | |
statement.setBigDecimal(2, book.getPrice()); | |
if (statement.executeUpdate() < 1) { |
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(sql)) { | ||
statement.setLong(1, id); | ||
int affectedRow = 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.
int affectedRow = statement.executeUpdate(); | |
return statement.executeUpdate() > 0; |
No description provided.