-
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
result #401
base: main
Are you sure you want to change the base?
result #401
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.
While the implementation of the DataProcessingException
class is correct and adheres to best practices, there are a few issues in other parts of the code that need addressing. Some of these issues, such as hardcoding passwords and incomplete DB_URL
, can lead to security risks and application malfunction. Also, improvements like avoiding code duplication, enhancing exception messages, and correctly handling resource closures can greatly increase the maintainability and reliability of your code. Remember, paying attention to these details early on can save you from headaches down the line! Keep up the good work, and with a few tweaks, your code will be in great shape! 💪👨💻🛠️
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/main/java/mate/academy/Main.java
Outdated
book1.setPrice(BigDecimal.valueOf(200)); | ||
Book book2 = new Book(); | ||
book2.setTitle("book2"); | ||
book2.setPrice(BigDecimal.valueOf(300));; |
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 an unnecessary semicolon at the end of this line. Each statement in Java should end with only one semicolon.
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create new 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 should be more informative. Include the book information to specify which book creation failed. For example: "Can't create a book: " + book
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create new book", e); | ||
} | ||
return 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.
The create method should return the created book object, not null. After setting the ID to the book, you should return the book.
} | ||
resultSet.close(); | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create 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 is not accurate. It should reflect the actual operation being performed. In this case, it should be "Can't get a book by id " + id
book.setPrice(resultSet.getObject("price", BigDecimal.class)); | ||
return Optional.of(book); | ||
} | ||
resultSet.close(); |
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 no need to explicitly close the ResultSet here since you are using try-with-resources, which will automatically close resources for you.
if (resultSet.next()) { | ||
Book book = new Book(); | ||
book.setId(id); | ||
book.setTitle(resultSet.getString("title")); | ||
book.setPrice(resultSet.getObject("price", BigDecimal.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.
Consider moving the logic for creating a Book object from a ResultSet into a separate private method to avoid code duplication. This is in line with the checklist item that advises against duplicating ResultSet processing logic.
while (resultSet.next()) { | ||
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.
As with the findById method, you should move the logic for creating a Book object from a 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://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 specify the database server's URL, port, and the database name you're connecting to. For example, jdbc:mysql://localhost:3306/myDatabase
where myDatabase
is the name of your database.
static { | ||
DB_PROPERTIES = new Properties(); | ||
DB_PROPERTIES.put("user", "root"); | ||
DB_PROPERTIES.put("password", "Dimension3306"); |
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 passwords 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.
Don't forget to close the Connection
object after usage to prevent resource leaks. This can be done using try-with-resources or by calling close()
in a finally block.
Was create method createNewBook() in BookDaoImpl simple changes DB_URL in ConnectionUtil
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.
let's slightly improve your solution :)
int affectedRows = statement.executeUpdate(); | ||
if (affectedRows < 1) { | ||
throw new RuntimeException("No affected rows"); | ||
} |
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.
I think these lines are redundant as you check generatedKeys next
No description provided.