-
Notifications
You must be signed in to change notification settings - Fork 300
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 라이브러리 구현하기 - 3,4단계] 스플릿(박상현) 미션 제출합니다. #537
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.
안녕하세요 스플릿!
코드가 깔끔해서 리뷰하기 좋네요 ㅎㅎ
TransactionUserService 관련해서 코멘트 남겼는데 확인 부탁드립니다! 크게 개선할 부분은 없는 것 같은데, 혹시 더 수정하고 싶은 부분 있으실 수도 있을 것 같아서 Request change 하겠습니다!!
수정하고 싶으신 부분 없으면 바로 리뷰요청 주시면 merge 하겠습니다!!
고생하셨어요 ㅎㅎ
final Connection connection = DataSourceUtils.getConnection(DataSourceConfig.getInstance()); | ||
try { | ||
connection.setAutoCommit(false); | ||
appService.insert(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.
appService 호출 부분을 제외하고 중복이라 이 부분 제거하면 매우 깔끔해지겠네요!
TransactionSynchronizationManager 에서 startTransaction()
메서드를 만든다거나, 혹은 템플릿 콜백 or 메서드 패턴을 활용해봐도 좋을 것 같아요
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.
좋은 것 같습니다!! 반영했습니다!!👍
return null; | ||
} | ||
|
||
public static void bindResource(DataSource key, Connection value) { | ||
if (Objects.isNull(resources.get())) { |
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.
null 체크 good~
this.appService = appService; | ||
} | ||
|
||
protected void rollback(final Connection connection) { |
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.
rollback()
만 정의하신 이유가 있을까요??
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.
try 문안에 try 문이 있는게 개인적으로 보기 불편하더라구요..ㅎㅎ
이 메서드도 TransactionTemplate 객체로 옮겨주었습니다!!😊
import java.sql.SQLException; | ||
import org.springframework.jdbc.core.error.SqlExceptionConverter; | ||
|
||
public abstract class TransactionService<T> { |
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.
이 친구가 transaction 시작과 끝 관련 책임을 가지는 것도 좋아보이네요!! 어떻게 생각하시나요?
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,4 단계 같이해서 리뷰요청 드립니다!!👍
정말 요구사항보다 더 좋은 방향으로 설계해보고자 많은 노력을 했으나 실패해버렸습니다ㅠㅠ
우선 이번단계에 대한 리뷰 시간이 많지 않으실 것 같아서 PR 보내고 이전 리뷰에 대한 코멘트 답변에 대한 부분 완성하고 DM 혹은 오프라인으로 찾아가서 말씀드리겠습니다!! 😄
그리고 최근에 컨디션 조절 못했더니 몸상태가 잘 안올라오더라구요ㅠㅠ 오늘 밤새신다고 하셨는데 너무 무리하지 마시구 화이팅입니다!! 💪