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

jdbc-intro #399

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

jdbc-intro #399

wants to merge 3 commits into from

Conversation

alwayswannajava
Copy link

No description provided.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Hey there! 👋 It looks like there are a few critical issues that need to be addressed before we can approve this Pull Request. Specifically, the handling of preparedStatement parameters starting from the wrong index and the incorrect usage of Statement.RETURN_GENERATED_KEYS in methods where it's not required could lead to bugs and unexpected behavior. Also, the table name in SQL queries should be consistent and follow best practices. Let's make these changes to ensure the code functions correctly and adheres to the guidelines. Keep up the good work, and I'm sure with a few tweaks, we'll get there! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
src/main/java/mate/academy/dao/BookDaoImpl.java Outdated Show resolved Hide resolved
@@ -0,0 +1,36 @@
package mate.academy;

Choose a reason for hiding this comment

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

Suggested change
package mate.academy;
package mate.academy.util;

replace to util package

import mate.academy.domain.Book;

public interface BookDao {

Choose a reason for hiding this comment

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

Suggested change

Statement.RETURN_GENERATED_KEYS)) {
preparedStatement.setString(2, book.getTitle());
preparedStatement.setBigDecimal(3, book.getPrice());
preparedStatement.execute();

Choose a reason for hiding this comment

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

Suggested change
preparedStatement.execute();
preparedStatement.execute();
int affectedRows = preparedStatement.executeUpdate();
if (affectedRows < 1) {
throw new DataProcessingException(
"Expected to insert at least 1 row, but 0 was inserted");
}


@Override
public Book create(Book book) {
Book createdBook;

Choose a reason for hiding this comment

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

Suggested change
Book createdBook;

Comment on lines 35 to 38
resultSet.next();
createdBook = new Book(
resultSet.getString("title"),
resultSet.getBigDecimal("price"));

Choose a reason for hiding this comment

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

Suggested change
resultSet.next();
createdBook = new Book(
resultSet.getString("title"),
resultSet.getBigDecimal("price"));
if (generatedKeys.next()) {
Long id = generatedKeys.getObject(1, Long.class);
book.setId(id);

Comment on lines 92 to 93
PreparedStatement preparedStatement = connection.prepareStatement(UPDATE_BOOK_QUERY,
Statement.RETURN_GENERATED_KEYS)) {

Choose a reason for hiding this comment

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

fix comment

Comment on lines 115 to 116
preparedStatement.setLong(1, id);
isDeleted = preparedStatement.execute();

Choose a reason for hiding this comment

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

Suggested change
preparedStatement.setLong(1, id);
isDeleted = preparedStatement.execute();
preparedStatement.setLong(1, id);
return preparedStatement.executeUpdate() > 0;

} catch (SQLException throwables) {
throw new RuntimeException(throwables);
}
return isDeleted;

Choose a reason for hiding this comment

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

Suggested change
return isDeleted;


@Override
public boolean deleteById(int id) {
boolean isDeleted;

Choose a reason for hiding this comment

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

Suggested change
boolean isDeleted;

create schema if not exists test;

create table if not exists book (
book_id serial primary key,

Choose a reason for hiding this comment

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

Suggested change
book_id serial primary key,
id bigint primary key,

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