-
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
added necessary methods #321
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.
Looks ok, left several comments
src/main/java/mate/academy/Main.java
Outdated
BookDao bookDao = (BookDao) injector.getInstance(BookDao.class); | ||
Book bookJava = new Book(); | ||
bookJava.setTitle("Java"); | ||
bookJava.setPrice(BigDecimal.valueOf(350.00)); | ||
bookDao.create(bookJava); | ||
Book bookHistory = new Book(); | ||
bookHistory.setTitle("History"); | ||
bookHistory.setPrice(BigDecimal.valueOf(350.00)); | ||
bookDao.create(bookHistory); | ||
Optional<Book> optional = bookDao.findById(3L); | ||
Book book = optional.get(); | ||
System.out.println(book); | ||
List<Book> list = bookDao.findAll(); | ||
System.out.println(list); | ||
bookDao.deleteById(1L); |
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 is hard to read, let's try to do something about it
public class BookDaoImpl implements BookDao { | ||
@Override | ||
public Book create(Book book) { | ||
String sql = "INSERT INTO book(title,price) VALUE(?, ?)"; |
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 var could use a better naming, same for similar ones in other methods
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2,book.getPrice()); | ||
int affectedRows = statement.executeUpdate(); | ||
if (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.
magic numbers, check other methods as well
statement.setBigDecimal(2,book.getPrice()); | ||
int affectedRows = statement.executeUpdate(); | ||
if (affectedRows < 1) { | ||
throw new RuntimeException("Expected to insert at least one row," |
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 custom exception that we have created
PreparedStatement statement = connection.prepareStatement(sql)) { | ||
ResultSet resultSet = statement.executeQuery(); | ||
while (resultSet.next()) { | ||
long id = resultSet.getInt("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.
Why int
if the book has Long
field? Also, check common mistakes about that getLong()
long id = resultSet.getInt("id"); | ||
String title = resultSet.getString("title"); | ||
BigDecimal price = resultSet.getBigDecimal("price"); | ||
Book book = new Book(); | ||
book.setId(id); | ||
book.setPrice(price); | ||
book.setTitle(title); | ||
books.add(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.
code duplication of getting a Book
from ResultSet
, move it to some parseBook()
method
long id = resultSet.getInt("id"); | ||
String title = resultSet.getString("title"); | ||
BigDecimal price = 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.
use constants instead of literals
books.add(book); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't create a new 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.
SQLException reason can be anything basically, not just the connection problem, so we can not be that specific in that case, but e
will provide us wit everything we need to fix the problem in case we encounter it
throw new DataProcessingException("Can't create a new connection to the DB", e); | |
throw new DataProcessingException("Can't find all books", e); |
src/main/resources/init_db.sql
Outdated
`title` VARCHAR(255), | ||
`price` DECIMAL(10, 2), | ||
PRIMARY KEY(`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.
); | |
); | |
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.
GJ! Also left several recommendations
newBook.setTitle(resultSet.getObject(TITLE_COLUMN, String.class)); | ||
newBook.setPrice(resultSet.getObject(PRICE_COLUMN, 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.
in the case of String
and BigDecimal
, it is actually fine to use getString()
and .getBigDecimal()
because it is already an object, when with case of getLong()
it returns a primitive(which can not be null
) and thus we have to use getObject()
instead.
It's fine to leave it as it is, just wanted you to know 😉
statement.setLong(KEY_POSITION_INDEX, book.getId()); | ||
int affectedRows = statement.executeUpdate(); | ||
if (affectedRows == EXCEPTION_AFFECTED_ROWS) { | ||
throw new RuntimeException("Updating book failed, no rows affected."); |
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'd also replace this ex to DataProcessingException
No description provided.