-
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
Home work #405
base: main
Are you sure you want to change the base?
Home work #405
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 best practices and adhering to Java conventions! Your implementation of the BookDao
interface, the DataProcessingException
class, and the Book
class is spot-on. Keep up the good work, and remember to always secure sensitive information and handle exceptions gracefully. Your attention to detail is commendable. 🌟 Keep it up!
✨ 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
Book updatedBook = bookDao.findAll().stream() | ||
.filter(b -> Objects.equals(b.getTitle(), "Effective Java")) | ||
.findFirst() | ||
.get(); |
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 code may throw a NoSuchElementException if 'findFirst()' doesn't find any element. It's better to use 'ifPresent()' after 'findFirst()' to check if the book is present before calling 'get()'.
src/main/java/mate/academy/Main.java
Outdated
private static List<Book> books = List.of( | ||
new Book("Core Java Volume I – Fundamentals", BigDecimal.valueOf(29.99)), | ||
new Book("Effective Java", BigDecimal.valueOf(19.99)), | ||
new Book("Java - The Complete Reference", BigDecimal.valueOf(35.45))); |
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.
Avoid using hard-coded values for books in the main method. It's better to move this to a separate method or class responsible for initializing data, or even better, to use a database script.
src/main/java/mate/academy/Main.java
Outdated
|
||
//delete | ||
books = bookDao.findAll(); |
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.
Calling 'bookDao.findAll()' multiple times is inefficient and can lead to performance issues. You should call it once and reuse the result.
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement | ||
= connection.prepareStatement(query, 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. In this case, you have used PreparedStatement correctly, but make sure to apply this practice consistently across all methods.
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
statement.setLong(3, book.getId()); | ||
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.
You should execute statement.executeUpdate()
in the update
method to actually perform the update operation on the database. Without this call, the update will not be applied.
|
||
@Override | ||
public Book update(Book book) { | ||
String query = "UPDATE books SET title = ?, price = ? 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 for consistency and readability. The update
query should have UPDATE
, SET
, and WHERE
in uppercase.
|
||
@Override | ||
public boolean deleteById(Long id) { | ||
String 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 for consistency and readability. The delete
query should have DELETE
and WHERE
in uppercase.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String URL = "jdbc:mysql://localhost:3306/library_service"; |
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 for the JDBC connection is incomplete. It should specify the database you're connecting to, for example, 'jdbc:mysql://localhost:3306/myDatabase'.
import java.util.Properties; | ||
|
||
public class ConnectionUtil { | ||
private static final String URL = "jdbc:mysql://localhost:3306/library_service"; |
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.
Avoid using hard-coded database credentials in your code. Consider using environment variables or configuration files to manage sensitive information securely.
} | ||
} | ||
|
||
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 a good practice to handle SQLException
inside the getConnection
method rather than throwing it, so you can provide more informative error messages and handle specific cases if necessary.
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.
Implement this part of HW
Create init_db.sql file in src/main/resources folder. In this file, put the scripts for creating required table.
src/main/java/mate/academy/Main.java
Outdated
public static void main(String[] args) { | ||
BookDao bookDao = (BookDao) INJECTOR.getInstance(BookDao.class); | ||
|
||
// Initialize data |
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 comments please
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
statement.setLong(3, book.getId()); | ||
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.
should we check if rows were updated?
.prepareStatement(query, Statement.RETURN_GENERATED_KEYS)) { | ||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
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.
should we check if rows were updated?
-added check if rows were updated
No description provided.