-
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
jdbc-solution - V1.0 #313
base: main
Are you sure you want to change the base?
jdbc-solution - V1.0 #313
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.
LGFM
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 use DB, not list =)
public static Connection getConnection() throws SQLException { | ||
return DriverManager.getConnection(DB_PATH, 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.
let's catch exception closer to its origin
public class BookDaoImpl implements BookDao { | ||
private List<Book> books = new ArrayList<>(); | ||
private Long idCounter = 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 implementation is wrong
we are learning to use MySQL. Please check common mistakes to see some examples
Or come to QnA to discuss it
src/main/resources/init_db.sql
Outdated
id INT NOT NULL AUTO_INCREMENT PRIMARY KEY, | ||
title VARCHAR(255), | ||
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.
); | |
); | |
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
if (affectedRows < 1) { | ||
throw new DataProcessingException( | ||
"Expected to insert at least one row, but inserted 0 rows for book: " | ||
+ 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 in the update method, extract it to a method
} | ||
|
||
private Book mapResultToBook(ResultSet resultSet) throws SQLException { | ||
Long id = resultSet.getLong("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.
as it was mentioned in the common mistakes
Long id = resultSet.getLong("id"); | |
Long id = resultSet.getObject("id", Long.class); |
Long id = resultSet.getLong("id"); | ||
String title = resultSet.getString("title"); | ||
BigDecimal price = 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.
let's make those strings constant
src/main/resources/init_db.sql
Outdated
@@ -0,0 +1,6 @@ | |||
USE jdbc; | |||
CREATE TABLE books ( | |||
id INT NOT NULL AUTO_INCREMENT PRIMARY KEY, |
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.
Primary key already contains not null constraint in it
id INT NOT NULL AUTO_INCREMENT PRIMARY KEY, | |
id INT AUTO_INCREMENT PRIMARY KEY, |
src/main/resources/init_db.sql
Outdated
CREATE TABLE books ( | ||
id INT NOT NULL AUTO_INCREMENT PRIMARY KEY, | ||
title VARCHAR(255), | ||
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.
I'd rather specify the ranges
price DECIMAL | |
price DECIMAL(10,2) |
long columnId = resultSet.getObject(ID, Long.class); | ||
BigDecimal columnPrice = resultSet.getObject(PRICE, BigDecimal.class); | ||
String columnTitle = resultSet.getString(TITLE); | ||
book.setTitle(columnTitle); | ||
book.setPrice(columnPrice); | ||
book.setId(columnId); |
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.
move it back to a separate method please =) this logic is duplicated so it should live in a separate method
book.setTitle(title); | ||
book.setPrice(price); | ||
return book; | ||
private boolean checkChanges(int affectedRows, Book 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.
please think about a more meaningful name
No description provided.