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

[3단계 - JDBC 라이브러리 구현하기] 페드로(류형욱) 미션 제출합니다. #850

Merged
merged 7 commits into from
Oct 14, 2024

Conversation

hw0603
Copy link
Member

@hw0603 hw0603 commented Oct 13, 2024

우주 안녕하세요! 3단계 미션 구현 완료해서 리뷰 요청 드려요🙂

바뀐 점은 크게 없고, Connection을 템플릿 외부에서 주입 가능하도록 QueryConnectionHolder를 하나 두고, 필요할 때 PrepareStatement로 변환해서 사용하는 형태로 구현했습니다.

오버로딩을 많이 활용하게 되는데, 사용자가 오버로딩된 메서드 중 어떤 것을 사용할 때 어느 정도까지 직접 커넥션을 관리해 줘야 하는지 메서드 시그니처만 보고서는 쉽게 파악할 수 없을 것 같다는 고민이 있긴 했지만... 아직까지는 뚜렷한 해결책을 찾지 못해서 이대로 요청 드리게 되었어요🥲

이번 단계도 잘 부탁합니다!

@hw0603 hw0603 self-assigned this Oct 13, 2024
Copy link

@tsulocalize tsulocalize left a comment

Choose a reason for hiding this comment

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

안녕하세요, 페드로!
슬랙 알람 오류 때문에 늦게 리뷰 드렸네요 ㅠㅠ
이번에는 간단한 리뷰만 남겨봤고, 다음 요청 시 merge 할 예정입니다!
화이팅 하세요~

Comment on lines 40 to 48
public void updateUsingExplicitConnection(User user, Connection connection) {
String sql = "UPDATE users SET account = ?, password = ?, email = ? where id = ?";
logSql(sql);
QueryConnectionHolder queryConnectionHolder = new QueryConnectionHolder(connection, sql);
PreparedStatementArgumentsSetter argumentsSetter = new PreparedStatementArgumentsSetter(
user.getAccount(), user.getPassword(), user.getEmail(), user.getId()
);
jdbcTemplate.update(queryConnectionHolder, argumentsSetter);
}

Choose a reason for hiding this comment

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

jdbcTemplate.update(sql, args...) 와 비교할 때 사용자가 사용하기 불편하다? 어렵다?고 느낄 것 같아 보여요!
혹시 jdbcTemplate.update(connection, sql, args...)로 설정하는 건 어떻게 생각하나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그것도 하나의 방법이 될 수 있겠네요👍 update 메서드의 시그니처를 추가해 보았습니다!

Comment on lines +6 to +10
@FunctionalInterface
public interface TransactionalFunction {

void execute(Connection connection) throws SQLException;
}

Choose a reason for hiding this comment

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

Consumer 역할로 보이는데 Function이라는 클래스명은 혼동을 줄 수 있을 것 같아요! (ThrowingConsumer도 있기 때문에)

Copy link
Member Author

Choose a reason for hiding this comment

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

그럴 수도 있겠네요!

사실 ThrowingConsumer 보다는 TransactionalFunction이 더 중요한 역할을 하는데요, ThrowingConsumer<T, E> 형태로 예외를 던지면서 consume 하는 전역 인터페이스를 만들어서 사용하려는 것이 의도였는데 지금은 사실 던질 수 있는 예외가 SQLException으로 고정되어 있고 TransactionManager에서만 사용하다 보니 굳이 클래스 외부에 있을 필요가 없을 것 같네요.

ThrowingConsumer 인터페이스 선언 위치를 TransactionManager 내부로 변경하였습니다. TransactionalFunction 인터페이스는 그대로 유지할게요!

@hw0603 hw0603 requested a review from tsulocalize October 14, 2024 05:14
Copy link

@tsulocalize tsulocalize left a comment

Choose a reason for hiding this comment

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

수고 많으셨어요!
마지막까지 화이팅~

@tsulocalize tsulocalize merged commit 1cf4285 into woowacourse:hw0603 Oct 14, 2024
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.

2 participants