-
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
Hw jv jdbc intro #39
base: main
Are you sure you want to change the base?
Hw jv jdbc intro #39
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.
Excellent job 💯 ! There are some suggestions :)
pom.xml
Outdated
<version>8.1.0</version> | ||
</dependency> | ||
</dependencies> | ||
|
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 remove this redundant line, if you don't mind :)
@Override | ||
public boolean equals(Object o) { | ||
if (this == o) { | ||
return true; | ||
} | ||
if (o == null || getClass() != o.getClass()) { | ||
return false; | ||
} | ||
Book book = (Book) o; | ||
return Objects.equals(id, book.id) && Objects.equals(title, book.title) && Objects.equals(price, book.price); | ||
} | ||
|
||
@Override | ||
public int hashCode() { | ||
return Objects.hash(id, title, 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.
I think it's off the map in this task
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.
Good, but let`s improve your solution
src/main/java/mate/academy/Main.java
Outdated
public class Main { | ||
private static final Injector injector = Injector.getInstance("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.
What about naming constants rules?
src/main/java/mate/academy/Main.java
Outdated
public static void main(String[] args) { | ||
BookDao bookDao = (BookDao) injector.getInstance(BookDao.class); | ||
|
||
Book EffectiveJava = new 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.
when we start naming variables from upper case letter?
} | ||
|
||
private void checkInsertedRowsAmount(int affectedRows) { | ||
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 number
Book book = new Book(); | ||
book.setId(resultSet.getObject("id", Long.class)); | ||
book.setTitle(resultSet.getString("title")); | ||
book.setPrice(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.
What is price is not defined in db?
String query = "DELETE FROM books WHERE id = ?"; | ||
try (Connection connection = ConnectionUtil.getConnection(); | ||
PreparedStatement statement = connection.prepareStatement(query)) { | ||
statement.setLong(1, 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.
magic number (check others as well)
book.setId(id); | ||
} | ||
} catch (SQLException e) { | ||
throw new DataProcessingException("Can't add new book: " + book, 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("Can't add new book: " + book, e); | |
throw new DataProcessingException("Can't create new book: " + book, e); |
import mate.academy.lib.Dao; | ||
import mate.academy.model.Book; | ||
import mate.academy.services.ConnectionUtil; | ||
import java.sql.*; |
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.
try to avoid using asterix in imports
|
||
ResultSet generatedKeys = statement.getGeneratedKeys(); | ||
if(generatedKeys.next()) { | ||
Long id = generatedKeys.getObject(1, 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.
whink which data should be returned the one you provide in request or the one which is actually saved?
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 not resolved. Here I wish to see that you populating book with data that actually in data base. Because you could do some manipulation with data and then save it, but in your implementation you will return not actual data
} | ||
|
||
private BigDecimal getBookPrice(ResultSet resultSet) throws SQLException { | ||
return resultSet.getBigDecimal("price") != null |
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 is not require to create new method to hadle it, just do tha same as you did for id
|
||
ResultSet generatedKeys = statement.getGeneratedKeys(); | ||
if(generatedKeys.next()) { | ||
Long id = generatedKeys.getObject(1, 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.
This is not resolved. Here I wish to see that you populating book with data that actually in data base. Because you could do some manipulation with data and then save it, but in your implementation you will return not actual 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.
Much better, but still some comments to resolve
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.
Good job!
No description provided.