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 구현하기 - 2단계] 하디(전동혁) 미션 제출합니다. #353

Merged
merged 5 commits into from
Oct 3, 2023

Conversation

jundonghyuk
Copy link

안녕하세요 오도 저번에 리뷰 잘해주셔서 감사해요 ㅎㅎ

밑에 커멘트 남겼으니 한 번 봐주셨으면 해요 !

이전 PR

이번 2단계의 목표는 클린코드 작성하기로 파악했습니다.

나름대로 clean하게 작성하려고 노력해봤는데 오도가 느끼기엔 아닐 수도 있어서 (ㅠ) 리뷰 잘 부탁드려요 !

즐거운 연휴되세요 ~

@jundonghyuk jundonghyuk changed the title [MVC 구현하기 - 2단계] 하디(전동혁) 미션 제출합니다. [JDBC 구현하기 - 2단계] 하디(전동혁) 미션 제출합니다. Oct 1, 2023
Copy link

@odo27 odo27 left a comment

Choose a reason for hiding this comment

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

안녕하세요 하디!
커스텀 예외를 만들어서 더 좋은 코드를 만들어주셨네요 👏👏
현재 JdbcTemplate의 update 메서드와 query 메서드에 Connection을 받아오는 부분이나 PreparedStatement를 만드는 부분에 중복이 존재하는 것 같아요!
혹시 여유가 된다면 이 부분의 중복을 제거하는 방법을 고민해보면 좋을 것 같아 코멘트 남깁니다 😊
또한 resource들을 close 해주는 부분도 try-with-resources 를 사용해 개선해보면 좋을 것 같아요 👍

Comment on lines 57 to 62
if (results.isEmpty()) {
throw new DataAccessException("Data not found.");
}
if (results.size() >= 2) {
throw new DataAccessException("More than one data found");
}
Copy link

Choose a reason for hiding this comment

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

꼼꼼한 예외 처리 👍

@jundonghyuk
Copy link
Author

jundonghyuk commented Oct 3, 2023

안녕하세요 오도 추석에도 리뷰해주셔서 감사합니다 !!

오도의 리뷰를 듣고 템플릿화를 한 번 해보았습니다 !

찾아본결과 resultSet은 pstmt가 닫힐 때 닫힌다고 하여 따로 닫아주지는 않았습니다 ! 감사합니다 ㅎㅎ

새로구현한 부분

Copy link

@odo27 odo27 left a comment

Choose a reason for hiding this comment

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

안녕하세요 하디!
리뷰 반영하신다고 고생 많으셨어요 👍👍

@odo27 odo27 merged commit 3924970 into woowacourse:jundonghyuk Oct 3, 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.

2 participants