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단계] 주디(오은비) 미션 제출합니다. #596

Merged
merged 18 commits into from
Oct 13, 2023

Conversation

eunbii0213
Copy link

성하~ (성하 하이라는 뜻)
시간 버그가 걸렸나요?; 병원 갔다오고 약 먹구 잠깐 눈 붙였는데 하루가 지나가버렸어요...
일어나서 전 제가 1시간만 잔 줄 알았는데.....날짜가 바뀌어있더라구요? ㅎ.. 죄송합니다...
리뷰 잘 부탁드려오..

Copy link

@sh111-coder sh111-coder left a comment

Choose a reason for hiding this comment

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

안녕하세요 주디! 4단계도 잘 구현해주셨네요!
시간보니 오전까지 달리셨네요 ㄷㄷ 👍🏻

1가지 궁금한 부분과 개선 사항 코멘트로 남겼습니다!
마지막 피드백일 것 같아요! 화이팅!


import static org.assertj.core.api.Assertions.assertThat;

class AppUserServiceTest {

Choose a reason for hiding this comment

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

꼼꼼한 테스트 굿굿!! 👍🏻

Comment on lines -16 to -17
public void log(final Connection connection, final UserHistory userHistory) {
System.out.println("로그뜰까요아닐까용오오오");

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋ여기 삭제 기록도 남네요

Comment on lines 23 to 25
public void insert(final User user) {
userService.insert(user);
}

Choose a reason for hiding this comment

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

현재 changePassword에서만 트랜잭션이 적용되고 있는데,
findByIdinsert에서도 트랜잭션이 적용되어야 할 것 같아요!

적용하다 보면 트랜잭션 처리 코드가 중복될텐데,
이를 분리해보는 방법도 생각해보면 좋을 것 같아요!
(이 부분이 어렵다면 그냥 중복 트랜잭션 코드만 작성해주셔도 괜찮을 것 같아요!)

Copy link
Author

Choose a reason for hiding this comment

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

findById는 조회인데도 트랜잭션을 적용해야하는 이유가 있나요?!

Choose a reason for hiding this comment

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

조회 중에도 격리 수준에 따라서 데이터 정합성이 지켜지지 않는 경우가 있어서 트랜잭션을 적용하는 것이 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

넵!! 적용하겠습니다!

throw new SizeException("too many result. expected 1 but was " + results.size());
throw new DataAccessException("too many result. expected 1 but was " + results.size());

Choose a reason for hiding this comment

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

여기서 기존 SizeException이 아닌 DataAccessException을 발생시키는 이유가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

전 금붕어입니다. 뻐끔뻐끔

Copy link

@sh111-coder sh111-coder left a comment

Choose a reason for hiding this comment

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

미션 진행하느라 수고하셨습니다!
코멘트 반영해주셔서 Approve & Merge 할게요!

@sh111-coder sh111-coder merged commit 6396b9b into woowacourse:eunbii0213 Oct 13, 2023
1 check failed
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