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

Mydb #358

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

Mydb #358

wants to merge 7 commits into from

Conversation

MarynaBodina
Copy link

No description provided.

Copy link

@okuzan okuzan left a comment

Choose a reason for hiding this comment

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

CI checks failed, please make sure build passes locally with any maven build command like mvn clean package

@MarynaBodina MarynaBodina requested a review from okuzan May 28, 2024 10:58
pom.xml Outdated

Choose a reason for hiding this comment

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

revert this. we should keep 1 blank line at the EOF

}
return Optional.empty();
} catch (SQLException e) {
throw new RuntimeException("Failed to find a book by id", e);

Choose a reason for hiding this comment

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

specify id in error message

preparedStatement.setLong(1, id);
return preparedStatement.executeUpdate() > 0;
} catch (SQLException e) {
throw new RuntimeException("Failed to delete a book by id", e);

Choose a reason for hiding this comment

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

specify id in error message

title VARCHAR(255),
price INT,
PRIMARY KEY(id)
);

Choose a reason for hiding this comment

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

Suggested change
);
);

try {
Class.forName("com.mysql.cj.jdbc.Driver");
} catch (ClassNotFoundException e) {
throw new RuntimeException("Can not lod JDBC driver", e);

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Can not lod JDBC driver", e);
throw new RuntimeException("Can not load JDBC driver", e);

public static void main(String[] args) {
BookDao bookDao = (BookDao) INJECTOR.getInstance(BookDao.class);

Choose a reason for hiding this comment

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

Let's create list of books here

Suggested change
List<Book> books = List.of(
new Book(...

bookDao.create(BOOK_2);
bookDao.create(BOOK_3);

bookDao.update(BOOK_1_UPDATE);

Choose a reason for hiding this comment

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

return updated book and print it

public class Main {
private static final Injector INJECTOR = Injector.getInstance("mate.academy");
private static final Book BOOK_1 = new Book("OnePiece", new BigDecimal(195));

Choose a reason for hiding this comment

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

remove these constants

PreparedStatement.RETURN_GENERATED_KEYS)) {
preparedStatement.setString(1, book.getTitle());
preparedStatement.setBigDecimal(2, book.getPrice());
preparedStatement.executeUpdate();

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Better use this case

Choose a reason for hiding this comment

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

Not fixed

preparedStatement.executeUpdate();
ResultSet resultSet = preparedStatement.getGeneratedKeys();
if (resultSet.next()) {
book.setId(resultSet.getLong(1));

Choose a reason for hiding this comment

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

Suggested change
book.setId(resultSet.getLong(1));
Long id = generatedKeys.getObject(1, Long.class);
book.setId(id);

Check common mistakes file

preparedStatement.setString(1, book.getTitle());
preparedStatement.setBigDecimal(2, book.getPrice());
preparedStatement.setLong(3, book.getId());
preparedStatement.executeUpdate();

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

Not fixed

import java.util.Properties;

public class ConnectionUtil {
private static final String DB_URL = "jdbc:mysql:"

Choose a reason for hiding this comment

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

Suggested change
private static final String DB_URL = "jdbc:mysql:"
private static final String DB_URL = "jdbc:mysql://localhost:3306/mydatabase?serverTimezone=UTC";

any concatenation creating new String object.

}
return book;
} catch (SQLException e) {
throw new RuntimeException("Failed to create a book", e);

Choose a reason for hiding this comment

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

Suggested change
throw new RuntimeException("Failed to create a book", e);
throw new DataProcessingException("Failed to create a book", e);

Check all places and use DataProcessingException

PreparedStatement.RETURN_GENERATED_KEYS)) {
preparedStatement.setString(1, book.getTitle());
preparedStatement.setBigDecimal(2, book.getPrice());
preparedStatement.executeUpdate();

Choose a reason for hiding this comment

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

Better use this case

Comment on lines 20 to 22
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(query,
PreparedStatement.RETURN_GENERATED_KEYS)) {

Choose a reason for hiding this comment

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

Try with resources use only in read operations

Choose a reason for hiding this comment

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

Not fixed

Comment on lines 78 to 79
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(query)) {

Choose a reason for hiding this comment

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

The same with Try-w-resources

Choose a reason for hiding this comment

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

Not fixed

Comment on lines 8 to 13
public class ConnectionUtil {
private static final String DB_URL =
"jdbc:mysql://localhost:3306/mydatabase?serverTimezone=UTC";
private static final Properties DB_PROPERTIES;

static {

Choose a reason for hiding this comment

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

You class should has only one instance

Choose a reason for hiding this comment

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

(Singletone)

Copy link

@Elena-Bruyako Elena-Bruyako left a comment

Choose a reason for hiding this comment

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

Please fix comments

Comment on lines 20 to 22
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(query,
PreparedStatement.RETURN_GENERATED_KEYS)) {

Choose a reason for hiding this comment

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

Not fixed

PreparedStatement.RETURN_GENERATED_KEYS)) {
preparedStatement.setString(1, book.getTitle());
preparedStatement.setBigDecimal(2, book.getPrice());
preparedStatement.executeUpdate();

Choose a reason for hiding this comment

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

Not fixed

Comment on lines 78 to 79
try (Connection connection = ConnectionUtil.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(query)) {

Choose a reason for hiding this comment

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

Not fixed

preparedStatement.setString(1, book.getTitle());
preparedStatement.setBigDecimal(2, book.getPrice());
preparedStatement.setLong(3, book.getId());
preparedStatement.executeUpdate();

Choose a reason for hiding this comment

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

Not fixed

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.

5 participants