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 라이브러리 구현하기 - 4단계] 매튜(김재연) 미션 제출합니다. #520

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

kpeel5839
Copy link

안녕하세요 마! 규성씨

저번 피드백도 너무 꿀맛이었습니다!

역시 마! 규성씨는 다르군요.

이번 PR 은 저번 PR 과 비교 하였을 때 크게 변동사항이 없는 것 같아요.

조금 있다면 DataSourceUtils 를 구현하여 사용한 것과, UserService 를 추상화한 것?

그 정도인 것 같네요 ㅎㅎ

마지막으로 이번 리뷰도 잘 부탁드린다는 말 전하면서 물러나도록 하겠습니다.

이번에도 잘 부탁드립니다 기무사 출신 스파이 마! 규성씨~!

@kpeel5839 kpeel5839 requested a review from aak2075 October 8, 2023 11:13
@kpeel5839 kpeel5839 self-assigned this Oct 8, 2023
@kpeel5839 kpeel5839 changed the base branch from main to kpeel5839 October 8, 2023 11:13
Copy link

@aak2075 aak2075 left a comment

Choose a reason for hiding this comment

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

안녕하세요 매! 재연씨!

지난 피드백이 꿀맛이었다니 영광입니다ㅎㅎㅎ

크게 변동 사항이 없다고 하셨지만 정말 멋지게 구현해 주셨네요! 많이 배웠습니다👍

전체적인 구조는 매우 좋지만 커넥션을 닫는 부분만 해결해 주시면 될 것 같습니다.

PreparedStatementCreator preparedStatementCreator = new PreparedStatementCreator(sql);
PreparedStatementSetter preparedStatementSetter = new PreparedStatementSetter(parameters);
Connection connection = DataSourceUtils.getConnection(dataSource);
Copy link

Choose a reason for hiding this comment

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

TransactionExecutor를 사용하지 않고 직접 update나 다른 Jdbctemplate의 메서드를 호출하는 경우에는 Connection이 닫히지 않겠군요!

@@ -15,6 +15,8 @@ dependencies {
implementation "org.apache.commons:commons-lang3:3.13.0"
implementation "ch.qos.logback:logback-classic:1.2.12"

implementation "com.h2database:h2:2.2.220"
Copy link

Choose a reason for hiding this comment

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

jdbc 모듈에 h2 의존성이 추가된 이유는 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

DataSourceConfig 를 jdbc 모듈에 옮겨서용!


DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());
final var user = new User("gugu", "password", "[email protected]");
userService.insert(user);
userDao.insert(user);
Copy link

Choose a reason for hiding this comment

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

👍

private TransactionExecutor() {
}

public static void transactionCommand(Runnable runnable) {
Copy link

Choose a reason for hiding this comment

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

최고입니다👍


public abstract class TransactionSynchronizationManager {

private static final ThreadLocal<Map<DataSource, Connection>> resources = new ThreadLocal<>();
private static final ThreadLocal<Map<DataSource, Connection>> resources = ThreadLocal.withInitial(HashMap::new);
Copy link

Choose a reason for hiding this comment

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

잘 배워갑니다👍


connection.commit();
} catch (SQLException firstException) {
rollback();
Copy link

Choose a reason for hiding this comment

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

메서드로 깔끔하게 분리해 주셨군요. 멋져요.
다만, exception 변수명을 firstException, rollback 메서드 내에서는 secondException으로 사용하고 있는데요, 첫번째인지 두번째인지는 몰라도 괜찮지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

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

모르고 안바꿨어요! 하하하하하

@kpeel5839
Copy link
Author

마! 규성씨!

제가 말씀하신 사항들을 잘 이행한지는 모르곘지만, 최대한 반영해보려 노력했습니다!

이번에도 잘 부탁드립니다!

Copy link

@aak2075 aak2075 left a comment

Choose a reason for hiding this comment

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

리뷰를 잘 반영해 주셨군요!

커넥션 닫는거 얘기했던거는 TxService를 거치지 않고 JdbcTemplate의 update를 단독으로 사용하는 경우에 커넥션이 닫히지 않아서 얘기를 했었습니다.

요구사항은 다 만족하신 것 같아서 머지하겠습니다. 위 내용만 한번 고민해주세요ㅎㅎㅎ

다음 미션도 화이팅입니다~!

@aak2075 aak2075 merged commit e307b6c into woowacourse:kpeel5839 Oct 10, 2023
1 check failed
@kpeel5839
Copy link
Author

떼이이이잉... 그렇군요...

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