-
Notifications
You must be signed in to change notification settings - Fork 301
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단계 - Transaction 적용하기] 몰리(김지민) 미션 제출합니다. #773
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 몰리!
3단계 잘 구현해 주셨네요! 몇가지 코멘트 남겼으니 확인해주세요!
} | ||
|
||
public void log(final UserHistory userHistory) { | ||
public void create(final UserHistory userHistory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 이 함수는 사용되지 않는 것 같아요!
어떤 의도가 있나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오버로딩하여 트랜잭션으로 수행되게 한 create 메서드는 집중해야 할 비즈니스 로직에 다른 관심사가 섞인, 조금 아쉬운 메서드라고 �생각하는데요. 이 메서드가 본래 비즈니스 로직에 대한 부분이라 지우기가 뭔가 아쉬웠어요 ㅎ....ㅋㅋㅋㅋ
그런데 지금 사용하지 않기 때문에 지우는 게 맞을 것 같습니다 ㅎㅎ
4단계 때 롤백하고 지금은 제거하겠습니다! 😁
@@ -56,4 +56,17 @@ public LocalDateTime getCreatedAt() { | |||
public String getCreateBy() { | |||
return createBy; | |||
} | |||
|
|||
@Override |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toString 으로 로그 만들어 두는것 좋네요 👍
@@ -37,6 +42,12 @@ public void update(final User user) { | |||
jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId()); | |||
} | |||
|
|||
public void updateWithTransactional(final Connection connection, final User user) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 오버로딩을 사용하면 조금 더 깔끔해질 것 같아요!
} | ||
} catch (SQLException ignored) {} | ||
} | ||
public void createWithTransactional(final Connection connection, final UserHistory userHistory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마찬가지로 하는 일이 비슷하고, 매개변수에 따라 달라진다면 오버로딩을 고려해봐도 좋을 것 같습니다!
@@ -12,7 +12,8 @@ | |||
|
|||
public class JdbcTemplate { | |||
|
|||
private final DataSource dataSource; | |||
private DataSource dataSource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 final 을 뺀 이유가 있나요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 connection에 접근하려고 이러저러 시도를 해보다가 미처 체크하지 못했네요 ㅎㅎ
다시 불변으로 변경하겠습니다!
@@ -24,9 +34,41 @@ public void insert(final User user) { | |||
} | |||
|
|||
public void changePassword(final long id, final String newPassword, final String createBy) { | |||
final var user = findById(id); | |||
log.info("[UserService] changePassword: id={}, newPassword={}, createBy={}", id, newPassword, createBy); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
개인적인 의견이 있어서 남깁니다! 반영 안하셔도 됩니다!
현재는 메서드들을 실행하다 예외가 발생하면 그 예외를 핸들링하는 코드는 없는 것 같아요.
물론 이렇게 해도 JDBC에서 같은 커넥션을 사용하고 있기 때문에 롤백에는 문제가 없습니다!
하지만 현재 코드에서 이 롤백 과정이 명시적으로 처리되지 않아 디버깅이나 에러 추적이 어려울 수 있다는 생각이 들었습니다!
만약 userHistoryDao.createWithTransactional(connection, new UserHistory(user, createBy));
을 실행중 예외가 발생했다면
"[UserHistoryDao] createWithTransactional: {}", userHistory.toString()
"데이터베이스 연결 중 에러가 발생했습니다.", e
이렇게 예외가 발생할 것 같습니다!
조금 더 분명하게 예외를 핸들링 하는 것도 좋아보입니다만 그냥 참고하고 가셔도 좋을 것 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재는 메서드들을 실행하다 예외가 발생하면 그 예외를 핸들링하는 코드는 없는 것 같아요.
비즈니스 로직을 처리 하다가 발생한 예외에 대한 핸들링과 롤백 처리를 말씀하시는 게 맞을까요?
해당 부분이 없었네요!
TransactionManager라는 객체를 두어 처리 하였습니다.
혹시 �제가 잘못 이해했다면 말해주세요 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3단계도 고생하셨습니다 몰리!
이제 마지막 단계인데 조금만 더 화이팅 해보죠!
안녕하세요 호기 ~
이번 단계도 잘 부탁드립니다!
트랜잭션은 외부에서 autoCommit이 false인 Connection을 주입하도록 변경했습니다.
그 과정에서 JdbcTemplate의 DataSource가 불변이 아니게 되버렸네요 🥲
관련해서 다른 좋은 아이디어 있으시면 조언 부탁드려요.
이번 단계도 파이팅해봐요 🔥