Skip to content
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 task #63

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ihorvasylenko23
Copy link

No description provided.

Comment on lines 17 to 19
bookDao.create(book);
Long bookId = 1L;
Optional<Book> foundBook = bookDao.findById(bookId);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is bad idea to find book by hardcoded id

Suggested change
bookDao.create(book);
Long bookId = 1L;
Optional<Book> foundBook = bookDao.findById(bookId);
Book createdBook = bookDao.create(book);
Optional<Book> foundBook = bookDao.findById(ceatedBook.getId());

PreparedStatement statement = connection.prepareStatement(CREATE_QUERY,
PreparedStatement.RETURN_GENERATED_KEYS)) {
setBookParameters(statement, book);
statement.executeUpdate();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there should be

Suggested change
statement.executeUpdate();
int updatedRows = statement.executeUpdate();
if(updatedRows < 1) {
throw new DataProcassException ...

PreparedStatement statement = connection.prepareStatement(UPDATE_QUERY)) {
setBookParameters(statement, book);
statement.setLong(3, book.getId());
statement.executeUpdate();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there should be

Suggested change
statement.executeUpdate();
int updatedRows = statement.executeUpdate();
if(updatedRows < 1) {
throw new DataProcassException ...

Comment on lines 8 to 12
Book create(Book book);
Optional<Book> findById(Long id);
List<Book> findAll();
Book update(Book book);
boolean deleteById(Long id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty lines between methods
also add empty line to the end of file

Comment on lines 19 to 23
private static final String CREATE_QUERY = "INSERT INTO books (title, price) VALUES (?, ?)";
private static final String FIND_BY_ID_QUERY = "SELECT * FROM books WHERE id = ?";
private static final String FIND_ALL_QUERY = "SELECT * FROM books";
private static final String UPDATE_QUERY = "UPDATE books SET title = ?, price = ? WHERE id = ?";
private static final String DELETE_BY_ID_QUERY = "DELETE FROM books WHERE id = ?";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make queries as local variables for method where you use it

Comment on lines 33 to 34
throw new DataProcessingException("Failed to create book: no rows were updated.");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its okay to affect zero rows

Comment on lines 88 to 90
if (updatedRows < 1) {
throw new DataProcessingException("Failed to create book: no rows were updated.");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

}
try (ResultSet generatedKeys = statement.getGeneratedKeys()) {
if (generatedKeys.next()) {
book.setId(generatedKeys.getLong(1));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see common mistakes how to get long values forom result set

statement.setString(1, book.getTitle());
statement.setBigDecimal(2, book.getPrice());
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty line to the end of file

public void setPrice(BigDecimal price) {
this.price = price;
}
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add empty line to the end of file

<version>8.1.0</version>
</dependency>
</dependencies>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants