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 CRUD operations #66

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

Conversation

SaiyaJ1N
Copy link

No description provided.

Copy link

@nacenik nacenik left a comment

Choose a reason for hiding this comment

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

Let's improve your solution :)

//update
book.setTitle("JDBC for wood people, Part2");
book.setPrice(BigDecimal.valueOf(500));
book.setId(3L);
Copy link

Choose a reason for hiding this comment

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

According to the query, you cannot change id for book.

Comment on lines 9 to 13
Book create(Book book);
Optional<Book> findById(Long id);
List<Book> findAll();
Book update(Book book);
boolean deleteById(Long id);
Copy link

Choose a reason for hiding this comment

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

Add spaces between methods

Comment on lines 20 to 21
statement.setString(1, book.getTitle());
statement.setBigDecimal(2, book.getPrice());
Copy link

Choose a reason for hiding this comment

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

Magic numbers. Fix it in all places. Maybe you can reuse some of them :)

Comment on lines 23 to 26
int affectedRows = statement.executeUpdate();
if (affectedRows < 1) {
throw new RuntimeException("Expected to insert at least 1 row, but inserted 0 rows.");
}
Copy link

Choose a reason for hiding this comment

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

You don't need to check affected rows here

Book bookFromDb = null;
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement statement = connection.prepareStatement(query)) {
statement.setLong(1, 3L);
Copy link

Choose a reason for hiding this comment

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

what is that? Where do you search by the id that method gets?

Comment on lines 100 to 104
int affectedRow = statement.executeUpdate();
if (affectedRow < 1) {
throw new RuntimeException("Expected to delete at least 1 row but was 0.");
}
return affectedRow > 0;
Copy link

Choose a reason for hiding this comment

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

you can delete 0 rows. Also, how are you expecting to get false in this method according to your solution?

Suggested change
int affectedRow = statement.executeUpdate();
if (affectedRow < 1) {
throw new RuntimeException("Expected to delete at least 1 row but was 0.");
}
return affectedRow > 0;
return statement.executeUpdate() > 0;

}
return affectedRow > 0;
} catch (SQLException e) {
throw new DataProcessingException("Can not delete row by the id: " + id, e);
Copy link

Choose a reason for hiding this comment

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

row or book?

}

} catch (SQLException e) {
throw new DataProcessingException("Can not update row by the book with id: " + book.getId(), e);
Copy link

Choose a reason for hiding this comment

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

row or book?

PRIMARY KEY (id)
);

SHOW TABLES;
Copy link

Choose a reason for hiding this comment

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

Suggested change
SHOW TABLES;

@@ -0,0 +1,14 @@
SHOW DATABASES;
Copy link

Choose a reason for hiding this comment

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

Suggested change
SHOW DATABASES;

@SaiyaJ1N SaiyaJ1N requested a review from nacenik August 14, 2023 17:24
pom.xml Outdated
@@ -56,4 +62,4 @@
</plugins>
</pluginManagement>
</build>
</project>
</project>

Choose a reason for hiding this comment

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

Do not forget about empty line at the end of files.

book.setId(id);
}
} catch (SQLException e) {
throw new DataProcessingException("Can`t get a new Book: " + book, e);

Choose a reason for hiding this comment

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

Not resolved.

Suggested change
throw new DataProcessingException("Can`t get a new Book: " + book, e);
throw new DataProcessingException("Can`t create a new book: " + book, e);

try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement statement = connection.prepareStatement(query)) {
statement.setLong(1, 3L);

Choose a reason for hiding this comment

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

Not resolved.

Comment on lines 81 to 86

int affectedRow = statement.executeUpdate();
if (affectedRow < 1) {
throw new RuntimeException("Expected to update at least 1 row but was 0.");
}

Choose a reason for hiding this comment

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

Not resolved.


private Book parseResultSetInBook(ResultSet resultSet) throws SQLException {
Book dbBook = new Book();
dbBook.setId(resultSet.getLong("id"));

Choose a reason for hiding this comment

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

image

try {
Class.forName("com.mysql.cj.jdbc.Driver");
} catch (ClassNotFoundException e) {
throw new RuntimeException(e);

Choose a reason for hiding this comment

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

Give some valuable message here.

CREATE TABLE books (
id BIGINT NOT NULL AUTO_INCREMENT,
title VARCHAR(255),
price INT,

Choose a reason for hiding this comment

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

Suggested change
price INT,
price DECIMAL,

Copy link

@nacenik nacenik left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

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.

3 participants