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단계] 바론(이소민) 미션 제출합니다. #385

Merged
merged 9 commits into from
Oct 6, 2023

Conversation

somsom13
Copy link

@somsom13 somsom13 commented Oct 2, 2023

안녕하세요 이레! 바론입니다 🙇‍♀️

이번 단계에서는 리팩터링 하는 과정을 거쳤는데요, 생각보다 이것저것 고민할 부분이 많아서 변경 사항이 많아진 것 같습니다.

리뷰에 참고해주셨으면 하는 내용

  1. JdbcTemplate에서 queryForObject 로 조회할 때 Optional 을 반환하기 VS 예외를 발생시키기
    1단계에서 이레와 이야기 할 때는 queryForObject 라는 이름이 있으니 "단일 조회"가 아닌 모든 경우는 예외를 발생키여야 할 것 같다 라고 결론을 내렸었어요. 이번 단계를 진행하면서도 "JdbcTemplate은 자신의 역할에만 충실해야 하므로 단일 객체 조회라는 목적에 어긋나면 예외를 발생시키고, 이에 대한 처리는 개발자가 DAO에서 하는 것이 맞다" 라고 결론을 내려서 동일하게 구현을 했습니다!
  2. UserDaoTest 격리시키기
    기존의 테스트는 BeforeEach에서 insert 작업을 하기 때문에 전체 테스트를 실행하면 중복된 insert 데이터가 들어가 있는 문제가 생겼습니다. (데이터를 삭제하는 로직이 없었더라구요)
    이를 해결하기 위해 JdbcTemplate에 DML, SELECT 문 외의 SQL문을 실행시키는 execute메서드를 추가하고, UserDaoTest에서 AfterEach로 테스트 후 데이터를 삭제하도록 하여 각 테스트를 격리시켰습니다.
  3. rownum (count) 제거
    이 부분에 대해서는 이레와 지난 번에 이야기 한 후, 사용되지 않는 파라미터를 가지고 있을 필요가 없다고 생각해서 이 부분을 제거했습니다.
    제 생각으로는 mapRow 는 개발자가 직접 구현해서 사용하는 Functional Interface 이기 때문에 구현하는 방식에 따라 rownum을 사용할 수도, 사용하지 않을 수도 있는 것 같아요! 사용하는 예시를 들어보자면, 개발자가 조회 된 데이터가 몇 번째 데이터인지에 따라 순서를 지정해주는 작업을 해야한다면 이런 상황에서 rownum을 사용할 수 있지 않을까 싶어요.
    현재 저의 코드에서는 rownum이 필요가 없기 때문에 인터페이스에서도 제거하였고, 이는 추후 필요한 상황이 생겼을 때 추가하는 것이 좋지 않을까 싶습니다.

이레와 이야기 해보고 싶은 내용

  1. try-with-resources 중복 제거를 위한 함수형 인터페이스 추가
    JdbcTemplate을 보면 모든 쿼리 메서드에 try-with-resources가 활용되고 있습니다. 이 중복을 제거하기 위해 함수형 인터페이스를 도입하였는데요, 결국 SQLException을 처리하기 위해 추가적인 메서드 분리가 또 필요해지더라구요.
    (ex. JdbcTemplate의 update 메서드에서 executeUpdate를 한 차례 더 분리한 것)
    중복을 제거하는 이유는 유지 보수 하기 좋고, 작성하기 편한 코드를 만들기 위해서라고 생각해요. 그리고 이를 위해서는 다른 개발자가 코드를 봤을 때 이해하기 쉬워야 한다는 조건이 선행되어야 한다고 생각하는데, 제가 보기에는 오히려 지금 코드가 불필요한 메서드 분리로 가독성이 더 떨어진다는 생각이 들어요 😂
    물론 이제는 새로운 쿼리 메서드가 추가되어도, Connection과 PreparedStatement를 세팅해주는 코드를 또 작성하지 않아도 된다는 편리함이 생기긴 하겠지만요..!
    혹시 이레는 어떻게 생각하시는지 의견이 궁금해요! (람다 도입 전후 코드는 bc40724 이 커밋에서 보시면 더 편하게 확인하실 수 있을 것 같아요!)

그리고 지난 번 코멘트에 남겨주셨던 Connection이 언제 닫히는 걸까? 에 대한 답변을 남겨두었습니다! #305 (comment) 이 것도 확인해주시면 좋을 것 같아요 ㅎㅎ

구구절절 내용이 길어졌네요.. 이번 리뷰도 잘 부탁드립니다! 🙇 🙇

@somsom13 somsom13 self-assigned this Oct 2, 2023
@somsom13 somsom13 requested a review from zillionme October 2, 2023 13:06
Copy link

@zillionme zillionme left a comment

Choose a reason for hiding this comment

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

안녕하세요 바론 이레입니다.

3일만에 리뷰를 단 저를 매우 치세요...🥲
바론의 코드가 깔끔하기도 하고, 1단계에서 바론과 얘기를 나눴던것을 토대로 짠 코드라는 것이 느껴져서 더욱 쉽게 리뷰할 수 있었습니다.

바론 코드에서 중점적으로 본 내용

  1. JdbcTemplate에서 queryForObject 로 조회할 때 예외를 발생시키기
    queryForObject가 단일 객체를 조회하지 못할 경우 예외를 발생시킴으로써 JdbcTemplate와 DAO의 책임 분리가 명확하게 일어난것 같습니다! 👍🏻👍🏻
  2. UserDaoTest 격리시키기
    리뷰에서도 남겼지만, queryForObject를 실행시켰을 때, 예외처리를 구체화 하면서 이전에 발생하지 못했던 문제가 발생하고, 이에 따라서 테스트에 문제가 있다는 것을 발견할 수 있었던 것 같습니다.!
    쿼리문 좋아요!

바론이 이야기 하고 싶은 내용에 대한 답변 및 더 얘기해보고 싶은 내용

JdbcTemplate을 보면 모든 쿼리 메서드에 try-with-resources가 활용되고 있습니다. 이 중복을 제거하기 위해 함수형 인터페이스를 도입하였는데요, 결국 SQLException을 처리하기 위해 추가적인 메서드 분리가 또 필요해지더라구요.

리뷰에도 남겼지만, 함수형 인터페이스를 사용했지만 함수형 인터페이스의 구현을 메서드에 넘김으로써, 실제로는 함수형 인터페이스 사용이라기보다는 메서드 분리와 가까운 코드가 만들어진 것 같아요!
함수형 인터페이스 메서드 자체에서 SQLException을 발생시키도록 하면 어차피 executeInternal()메서드 안에서 SQLException을 DataAccessException으로 변환시켜주기 때문입니다.

자세한 사항은 리뷰에 남겼습니다.

Connection은 언제 닫히는 걸까 에 대한 질문에 디버깅까지 해주셔서 답변 정말 감사드립니다,,,ㅜ
전 핑거 프린세스였어요...

@zillionme
Copy link

바론에게 추가적으로 궁금한점

함수형 인터페이스 메서드 run()은 T를 반환하는데,

public <T> List<T> query(final String sql, final RowMapper<T> rowMapper, final Object... args) {
        return executeInternal(sql, pstmt -> queryMultipleResults(pstmt, rowMapper), args);
    }

executeInternal 내부의 executor.run() 메서드는 List를 반환하는데도 문제가 발생하지 않더라구요. 혹시 이유를 아시나요?

@somsom13
Copy link
Author

somsom13 commented Oct 6, 2023

안녕하세요 이레~~~ 바론입니다!

이레가 남겨주신 코멘트들을 보고 반영해보았습니다 ㅎㅎ 그리고 반영하지 않은 부분들에 대해서는 제 생각을 남겨두었어요!
제 개인적인 생각들이라서 이레가 의문이 드는 점이 있다면 코멘트 남겨주시면 감사하겠습니다 ㅎㅎ 🙇‍♀️ 🙇‍♀️

이레가 추가적으로 남겨주신 함수형 인터페이스는 T를 반환하는데, 구현체에서는 List를 반환하는 상황 에 대해서는 저희 팀원들에게 도움을 받았는데요 ㅎㅎ List 자체가 타입(type) 이기 때문에 가능하다고 합니다!

Copy link

@zillionme zillionme left a 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가 많이 쌓여서 코드보기가 조금 어려워서요!

@somsom13
Copy link
Author

somsom13 commented Oct 6, 2023

안녕하세요 이레~~~ 바론입니다!!!

이레랑 코드 리뷰로 얘기 주고받는거 너무 재밌네용ㅎㅎ
특히

jdbcTemplate이 예외를 핸들링하거나 하지 않는 선택권을 사용자에게 주는 것이라고 생각해요.

이 부분은 정말 머리를 한 대 얻어맞은 느낌이었습니다.. 💫 😵 지금까지 봐왔던 모든 CheckedException, UnCheckedException 중에서 가장 와닿는 말이었던 것 같아요 ㅋㅋㅋ

마지막으로 남겨주신 "람다식 내의 중괄호도 이중 뎁스라고 볼 수 있는가?" 이 부분은 도무지 참고 자료를 못 찾겠어서 phind에 물어본 결과... 이중 뎁스는 loop, 조건문에만 적용이 되는 부분이라고 하네요!
이레의 코멘트대로 lambda 내의 while은 이중 뎁스 취급이 안되는 것 같습니다 👍
받은 답변이 도움이 많이 되어서 밑에 함께 첨부합니다 ㅎㅎ

The lambda expression in this case is `pstmt -> {...}`, and the while loop is inside its body. 

To answer your question, this is not considered as "double depth". The term "double depth" is not a standard term in Java or programming in general, but it might refer to the concept of nested loops or nested conditionals, where one loop or conditional is inside another. In your case, the while loop is inside a lambda expression, which is not the same as being nested inside another loop or conditional [Source 12](https://www.w3schools.com/java/java_lambda.asp).

It's important to understand that lambda expressions in Java are not the same as inner classes. They have their own scope and can't hide variables from the enclosing scope inside the lambda's body. In this case, the keyword this is a reference to an enclosing instance [Source 0](https://www.baeldung.com/java-8-lambda-expressions-tips). 

Moreover, Java lambda expressions are meant to be concise and ideally written in one line of code. While it's not a strict rule, having a large block of code inside a lambda's body might make its functionality less clear [Source 0](https://www.baeldung.com/java-8-lambda-expressions-tips).

In conclusion, while you can use loops inside lambda expressions in Java, it's important to remember that the scope rules for lambda expressions are different, and you should strive to keep your lambda expressions concise for the sake of readability.

Copy link

@zillionme zillionme left a comment

Choose a reason for hiding this comment

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

바론 수정한 코드 잘봤습니다!
바론과 코드 관련 이야기 주고 받으면서 제가 더 배우는 게 많은 것 같아요!
제 늦은 리뷰도 이해해 주시고... 바론은 천사...😇
2단계는 여기서 이만 머지하도록 하겠습니다. 3단계에서 또 많은 얘기 나눠용

Comment on lines 41 to 47
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));
}

Choose a reason for hiding this comment

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

ㄹ리뷰 반영해 주셔서👍🏻👍🏻

@zillionme zillionme merged commit ee424dc into woowacourse:somsom13 Oct 6, 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