[JDBC 라이브러리 구현하기 - 4단계] 무민(박무현) 미션 제출합니다. #533
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mcodnjs
requested changes
Oct 9, 2023
There was a problem hiding this comment.
안녕하세요, 무민!
휴일은 잘 보내셨나요 🫠 벌써 마지막 단계까지 왔네요 ㅎㅎ
3단계에서 남긴 코멘트 반영은 커밋을 통해 모두 확인했습니다!
사라질 메서드인데도 반영해주신거 👍
TransactionSynchronizationManager의 리소스 처리 부분에 대해서만 코멘트 조금 남겼어요!
현재 구현하신 부분들에 대한 테스트 코드를 작성하신다면 조금 더 단단한 코드가 될 거 같아요!!
무민이 질문 주신 부분에서, 부끄럽지만 저는 예외 처리 관련해서 깊게 고민을 하진 않았던 것 같아요 ㅠㅠ
구현에만 급급했다랄까 .. 말씀 주신 부분은 저도 조금 더 고민해보겠습니다 ㅎㅎ
오늘 마무리되는 일정인 만큼 다음 요청에서는 바로 머지하도록 하겠습니다 🙃
4단계도 수고하셨습니다!
Comment on lines
20
to
27
| public static void bindResource(DataSource key, Connection value) { | ||
| resources.set(Map.of(key, value)); | ||
| } | ||
|
|
||
| public static Connection unbindResource(DataSource key) { | ||
| return null; | ||
| final Connection connection = resources.get().get(key); | ||
| resources.remove(); | ||
| return connection; |
There was a problem hiding this comment.
- getResource() 의 경우에는 resources.get() 으로 발생하는 NPE를 예방하고자 null 검사를 하고 있는데요,
bindResource()와unbindResource()는 null 처리가 필요없을까요?! 혹은 미리 초기화해두는 방법도 있을 거 같아요!
There was a problem hiding this comment.
- 리소스 처리 관련해서 데이터소스에 이미 바인딩된 커넥션이 있는 경우, 바인딩된 커넥션이 없는 경우 등을 고려한다면 조금 더 방어적인 코드가 될 거 같아요 !!
There was a problem hiding this comment.
bindResource()와unbindResource()의 로직이 무민이 생각하신대로 동작하는지 확인해보면 어떨까요? 현재 두 로직은 Map을 활용하지 못하고 있는거 같아요 ..! 현재 로직으로선 하나의 데이터소스에 대한 커넥션만 resources에 담기지 않을까요?
| this.dataSource = dataSource; | ||
| } | ||
|
|
||
| public <T> T execute(BusinessLogicProcessor<T> businessLogicProcessor) { |
Comment on lines
34
to
40
| private void rollbackAndThrowException(final Connection connection, final Exception e) { | ||
| try { | ||
| connection.rollback(); | ||
| } catch (SQLException ex) { | ||
| throw new DataAccessException(ex); | ||
| } | ||
| } |
mcodnjs
approved these changes
Oct 11, 2023
Comment on lines
+15
to
+21
| if (resources.get() == null) { | ||
| return null; | ||
| } | ||
| if (key == null) { | ||
| throw new IllegalArgumentException("Key must not be null"); | ||
| } | ||
| return resources.get().get(key); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요 라온 😎 드디어 마지막 단계네요!
새벽이지만 오늘 휴일이라 햅삐합니다 ㅎㅎㅎ 🚀
추상화 구조는 저번이랑 비슷하기 때문에 이미지는 딱히 첨부 안했습니다!
1단계부터 예외 처리하는 부분이 신경쓰이긴 하지만 딱히 더 개선할 방법이 생각이 안 나네요...ㅠ
혹시 라온은 다르게 처리하였다면 어떻게 처리하셨는지 궁금하네요! 혹은 미션하시면서 이 부분은 중점적으로 적용했다 하신 부분이 있을까요!?
마지막까지 화이팅하세요~!