-
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
Jv jdbc intro hw solution #319
base: main
Are you sure you want to change the base?
Jv jdbc intro hw solution #319
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.
Some modifications required
|
||
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.
src/main/resources/init_db.sql
Outdated
@@ -0,0 +1,6 @@ | |||
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 NOT NULL AUTO_INCREMENT, |
public class BookDaoImpl implements BookDao { | ||
@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.
this var could use a better naming, same in other places
|
||
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.
why raw RuntimeExcepion
?
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new RuntimeException("Cannot save data", 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.
same here and in other places
Long bookId = resultSet.getLong("id"); | ||
String title = resultSet.getString("title"); | ||
BigDecimal price = resultSet.getBigDecimal("price"); | ||
Book book = new Book(); | ||
book.setId(bookId); | ||
book.setTitle(title); | ||
book.setPrice(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.
code duplication, extract a private method parseBook()
from it
Long bookId = resultSet.getLong("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.
make those literals constants
statement.setString(1, book.getTitle()); | ||
statement.setBigDecimal(2, book.getPrice()); | ||
statement.setLong(3, book.getId()); |
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
int rowsDeleted = statement.executeUpdate(); | ||
return rowsDeleted > 0; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to delete book with id: " + id + 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.
throw new DataProcessingException("Failed to delete book with id: " + id + e); | |
throw new DataProcessingException("Failed to delete book with id: " + id, e); |
} | ||
return book; | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Failed to update book with id: " + book.getId() + 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.
same here, we should pass it as a separate parameter, because we need the whole original exception info, like stack trace and it's message to be saved, and passed ll the way to the user or dev
src/main/resources/init_db.sql
Outdated
CREATE TABLE `books` ( | ||
`id` INT NOT NULL AUTO_INCREMENT, | ||
`bookTitle` VARCHAR(50), | ||
`price` BIGINT, |
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 can be like 10.99
, does integer allow any value after point?
No description provided.