-
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 BookDao #416
base: main
Are you sure you want to change the base?
Implemented BookDao #416
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 interface naming conventions in your BookDao
interface and structuring your Book
class according to JavaBean conventions! Just remember to keep an eye on the consistency of object creation and exception messages for future enhancements. Keep up the good work! 👍💡
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
BookDao bookDao = (BookDao) injector.getInstance(BookDao.class); | ||
|
||
Book zapovit = new Book("zapovit", BigDecimal.valueOf(100)); | ||
Book newZapiovit = bookDao.create(zapovit); |
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 variable 'newZapiovit' is never used. If you don't need the result of 'bookDao.create(zapovit)', you can omit assigning it to a variable.
Book zapovit = new Book("zapovit", BigDecimal.valueOf(100)); | ||
Book newZapiovit = bookDao.create(zapovit); | ||
|
||
Optional<Book> bookById2 = bookDao.findById(2L); |
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 variable 'bookById2' is never used. Make sure to use it or remove it if it's not necessary for the main logic.
|
||
Optional<Book> bookById2 = bookDao.findById(2L); | ||
|
||
List<Book> allBooks = 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.
The variable 'allBooks' is never used. Make sure to use it or remove it if it's not necessary for the main logic.
|
||
List<Book> allBooks = bookDao.findAll(); | ||
|
||
Book updateZapovit = bookDao.update(zapovit); |
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 variable 'updateZapovit' is never used. If you don't need the result of 'bookDao.update(zapovit)', you can omit assigning it to a variable.
|
||
boolean isDeleted = 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.
The variable 'isDeleted' is never used. Consider logging the result or handling the deletion result in some way.
throw new DataProcessingException( | ||
"Expected to update at least one row, but updated 0 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.
Use DataProcessingException
for consistency with the rest of the DAO layer.
} | ||
return book; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can not create a connection to the database", 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 and specific to the operation being performed. For example: 'Can't update book: ' + book
|
||
@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.
PreparedStatement statement = connection.prepareStatement(sql)) { | ||
statement.setLong(1, id); | ||
int affectedRows = statement.executeUpdate(); | ||
return affectedRows >= 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.
The deleteById
method should return true only if one or more rows were affected. The current implementation is correct, so no action is needed here.
int affectedRows = statement.executeUpdate(); | ||
return affectedRows >= 1; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can not create a connection to the database", 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 and specific to the operation being performed. For example: 'Can't delete book with id: ' + id
@@ -0,0 +1,34 @@ | |||
package mate.academy; |
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.
package mate.academy; | |
package mate.academy.util; |
statement.setLong(1, id); | ||
ResultSet resultSet = statement.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.
create separate methods to map result set to book
PreparedStatement statement = connection.prepareStatement(sql)) { | ||
ResultSet resultSet = statement.executeQuery(); | ||
while (resultSet.next()) { | ||
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 separate method from comment above
src/main/resources/init_db.sql
Outdated
USE `intro`; | ||
|
||
CREATE TABLE `books` ( | ||
`id` INT 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` INT 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.
not fixed
src/main/resources/init_db.sql
Outdated
CREATE TABLE `books` ( | ||
`id` INT NOT NULL AUTO_INCREMENT, | ||
`title` VARCHAR(255), | ||
`price` INT, |
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, | |
`price` DECIMAL, |
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.
Nice, left minor comments
private Book mapToBook(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.
Private methods should be after all public
if (affectedRows < 1) { | ||
throw new DataProcessingException( | ||
"Expected to update at least one row, but updated 0 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.
Code duplication
statement.setLong(3, book.getId()); | ||
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); |
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.
statement.setLong(3, book.getId()); | |
statement.setString(1, book.getTitle()); | |
statement.setBigDecimal(2, book.getPrice()); | |
statement.setString(1, book.getTitle()); | |
statement.setBigDecimal(2, book.getPrice()); | |
statement.setLong(3, book.getId()); | |
…k in BookDaoImpl.
src/main/resources/init_db.sql
Outdated
USE `intro`; | ||
|
||
CREATE TABLE `books` ( | ||
`id` INT 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.
not fixed
No description provided.