-
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 라이브러리 구현하기 - 2단계] 바론(이소민) 미션 제출합니다. #385
Conversation
- UserDaoTest 전체 테스트 시 데이터 초기화를 위해 JdbcTemplate에 execute 메서드 추가, AfterEach로 데이터 모두 삭제
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일만에 리뷰를 단 저를 매우 치세요...🥲
바론의 코드가 깔끔하기도 하고, 1단계에서 바론과 얘기를 나눴던것을 토대로 짠 코드라는 것이 느껴져서 더욱 쉽게 리뷰할 수 있었습니다.
바론 코드에서 중점적으로 본 내용
- JdbcTemplate에서 queryForObject 로 조회할 때 예외를 발생시키기
queryForObject가 단일 객체를 조회하지 못할 경우 예외를 발생시킴으로써 JdbcTemplate와 DAO의 책임 분리가 명확하게 일어난것 같습니다! 👍🏻👍🏻 - UserDaoTest 격리시키기
리뷰에서도 남겼지만, queryForObject를 실행시켰을 때, 예외처리를 구체화 하면서 이전에 발생하지 못했던 문제가 발생하고, 이에 따라서 테스트에 문제가 있다는 것을 발견할 수 있었던 것 같습니다.!
쿼리문 좋아요!
바론이 이야기 하고 싶은 내용에 대한 답변 및 더 얘기해보고 싶은 내용
JdbcTemplate을 보면 모든 쿼리 메서드에 try-with-resources가 활용되고 있습니다. 이 중복을 제거하기 위해 함수형 인터페이스를 도입하였는데요, 결국 SQLException을 처리하기 위해 추가적인 메서드 분리가 또 필요해지더라구요.
리뷰에도 남겼지만, 함수형 인터페이스를 사용했지만 함수형 인터페이스의 구현을 메서드에 넘김으로써, 실제로는 함수형 인터페이스 사용이라기보다는 메서드 분리와 가까운 코드가 만들어진 것 같아요!
함수형 인터페이스 메서드 자체에서 SQLException을 발생시키도록 하면 어차피 executeInternal()메서드 안에서 SQLException을 DataAccessException으로 변환시켜주기 때문입니다.
자세한 사항은 리뷰에 남겼습니다.
Connection은 언제 닫히는 걸까 에 대한 질문에 디버깅까지 해주셔서 답변 정말 감사드립니다,,,ㅜ
전 핑거 프린세스였어요...
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
jdbc/src/main/java/org/springframework/jdbc/core/QueryExecutor.java
Outdated
Show resolved
Hide resolved
바론에게 추가적으로 궁금한점함수형 인터페이스 메서드 run()은 T를 반환하는데,
executeInternal 내부의 executor.run() 메서드는 List를 반환하는데도 문제가 발생하지 않더라구요. 혹시 이유를 아시나요? |
안녕하세요 이레~~~ 바론입니다! 이레가 남겨주신 코멘트들을 보고 반영해보았습니다 ㅎㅎ 그리고 반영하지 않은 부분들에 대해서는 제 생각을 남겨두었어요! 이레가 추가적으로 남겨주신 |
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.
리뷰 한 개 추가 가지고 한 번더 request change를 보내게 되었는데용
적용된 리뷰는 Resolve를 눌러줄 수 있을까요? comment가 많이 쌓여서 코드보기가 조금 어려워서요!
jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Outdated
Show resolved
Hide resolved
안녕하세요 이레~~~ 바론입니다!!! 이레랑 코드 리뷰로 얘기 주고받는거 너무 재밌네용ㅎㅎ
이 부분은 정말 머리를 한 대 얻어맞은 느낌이었습니다.. 💫 😵 지금까지 봐왔던 모든 CheckedException, UnCheckedException 중에서 가장 와닿는 말이었던 것 같아요 ㅋㅋㅋ 마지막으로 남겨주신 "람다식 내의 중괄호도 이중 뎁스라고 볼 수 있는가?" 이 부분은 도무지 참고 자료를 못 찾겠어서 phind에 물어본 결과... 이중 뎁스는 loop, 조건문에만 적용이 되는 부분이라고 하네요!
|
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.
바론 수정한 코드 잘봤습니다!
바론과 코드 관련 이야기 주고 받으면서 제가 더 배우는 게 많은 것 같아요!
제 늦은 리뷰도 이해해 주시고... 바론은 천사...😇
2단계는 여기서 이만 머지하도록 하겠습니다. 3단계에서 또 많은 얘기 나눠용
final ResultSet resultSet = pstmt.executeQuery(); | ||
final List<T> results = new ArrayList<>(); | ||
if (resultSet.next()) { | ||
results.add(rowMapper.mapRow(resultSet, resultSet.getRow())); | ||
|
||
while (resultSet.next()) { | ||
results.add(rowMapper.mapRow(resultSet)); | ||
} | ||
|
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.
ㄹ리뷰 반영해 주셔서👍🏻👍🏻
안녕하세요 이레! 바론입니다 🙇♀️
이번 단계에서는 리팩터링 하는 과정을 거쳤는데요, 생각보다 이것저것 고민할 부분이 많아서 변경 사항이 많아진 것 같습니다.
리뷰에 참고해주셨으면 하는 내용
JdbcTemplate
에서queryForObject
로 조회할 때Optional
을 반환하기 VS 예외를 발생시키기1단계에서 이레와 이야기 할 때는
queryForObject
라는 이름이 있으니 "단일 조회"가 아닌 모든 경우는 예외를 발생키여야 할 것 같다 라고 결론을 내렸었어요. 이번 단계를 진행하면서도 "JdbcTemplate
은 자신의 역할에만 충실해야 하므로 단일 객체 조회라는 목적에 어긋나면 예외를 발생시키고, 이에 대한 처리는 개발자가 DAO에서 하는 것이 맞다" 라고 결론을 내려서 동일하게 구현을 했습니다!UserDaoTest
격리시키기기존의 테스트는
BeforeEach
에서insert
작업을 하기 때문에 전체 테스트를 실행하면 중복된insert
데이터가 들어가 있는 문제가 생겼습니다. (데이터를 삭제하는 로직이 없었더라구요)이를 해결하기 위해
JdbcTemplate
에 DML, SELECT 문 외의 SQL문을 실행시키는execute
메서드를 추가하고,UserDaoTest
에서AfterEach
로 테스트 후 데이터를 삭제하도록 하여 각 테스트를 격리시켰습니다.rownum (count)
제거이 부분에 대해서는 이레와 지난 번에 이야기 한 후, 사용되지 않는 파라미터를 가지고 있을 필요가 없다고 생각해서 이 부분을 제거했습니다.
제 생각으로는
mapRow
는 개발자가 직접 구현해서 사용하는Functional Interface
이기 때문에 구현하는 방식에 따라rownum
을 사용할 수도, 사용하지 않을 수도 있는 것 같아요! 사용하는 예시를 들어보자면, 개발자가 조회 된 데이터가 몇 번째 데이터인지에 따라 순서를 지정해주는 작업을 해야한다면 이런 상황에서rownum
을 사용할 수 있지 않을까 싶어요.현재 저의 코드에서는
rownum
이 필요가 없기 때문에 인터페이스에서도 제거하였고, 이는 추후 필요한 상황이 생겼을 때 추가하는 것이 좋지 않을까 싶습니다.이레와 이야기 해보고 싶은 내용
try-with-resources
중복 제거를 위한 함수형 인터페이스 추가JdbcTemplate
을 보면 모든 쿼리 메서드에try-with-resources
가 활용되고 있습니다. 이 중복을 제거하기 위해 함수형 인터페이스를 도입하였는데요, 결국SQLException
을 처리하기 위해 추가적인 메서드 분리가 또 필요해지더라구요.(ex. JdbcTemplate의
update
메서드에서executeUpdate
를 한 차례 더 분리한 것)중복을 제거하는 이유는 유지 보수 하기 좋고, 작성하기 편한 코드를 만들기 위해서라고 생각해요. 그리고 이를 위해서는 다른 개발자가 코드를 봤을 때 이해하기 쉬워야 한다는 조건이 선행되어야 한다고 생각하는데, 제가 보기에는 오히려 지금 코드가 불필요한 메서드 분리로 가독성이 더 떨어진다는 생각이 들어요 😂
물론 이제는 새로운 쿼리 메서드가 추가되어도, Connection과 PreparedStatement를 세팅해주는 코드를 또 작성하지 않아도 된다는 편리함이 생기긴 하겠지만요..!
혹시 이레는 어떻게 생각하시는지 의견이 궁금해요! (람다 도입 전후 코드는 bc40724 이 커밋에서 보시면 더 편하게 확인하실 수 있을 것 같아요!)
그리고 지난 번 코멘트에 남겨주셨던
Connection이 언제 닫히는 걸까?
에 대한 답변을 남겨두었습니다! #305 (comment) 이 것도 확인해주시면 좋을 것 같아요 ㅎㅎ구구절절 내용이 길어졌네요.. 이번 리뷰도 잘 부탁드립니다! 🙇 🙇