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단계] 디노(신종화) 미션 제출합니다. #350

Merged
merged 7 commits into from
Oct 5, 2023

Conversation

jjongwa
Copy link
Member

@jjongwa jjongwa commented Oct 1, 2023

안녕하세요 오잉~👨‍🎤 다이노입니다🦖

즐거운 추석 연휴 보내고 계신가요ㅎㅎ
벌써 연휴의 절반을 넘어가고 있네요..

2단계는 리팩터링이더라구요..!
jdbcTemplate의 각 메서드마다 try-catch가 붙어있는게 맘에 안들긴 하지만
어떻게 해야 더 깔끔하게 짤 수 있을지 잘 모르겠네요..
오잉의 조언 부탁드립니다..

남은 추석 명절 즐겁게 보내시구
이번 단계도 잘 부탁드려요~~
(오잉이 1단계에서 남겨주신 리뷰 이번 단계에 적용했습니다.!)

@jjongwa jjongwa requested a review from hanueleee October 1, 2023 05:00
@jjongwa jjongwa self-assigned this Oct 1, 2023
Copy link

@hanueleee hanueleee left a comment

Choose a reason for hiding this comment

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

안녕하십니까 다이노🦖

리뷰가 늦어서 정말...............정말로..............죄송합니다🥲
그대신 최선을 다해서 리뷰하고자 노력했습니다..
하지만 1단계에서 이미 충분한 리팩토링을 진행하셔서인지
이번단계에서 추가적으로 봐야할 부분이 많지 않았던 것 같아요,,

jdbcTemplate의 각 메서드마다 try-catch가 붙어있는게 맘에 안들긴 하지만
어떻게 해야 더 깔끔하게 짤 수 있을지 잘 모르겠네요..
오잉의 조언 부탁드립니다.

저도 현재 try-catch 중복이 마음에 안들어서 이것저것 시도해보고 있는데요..!
템플릿 콜백 패턴을 사용하면 중복로직을 제거할 수 있을 것 같아요!!
(물론 저도 완성은 못했고 ㅎ 그저 try~ 중 ㅎ)

스프링에서도 '트랜잭션을 시작하고, 비즈니스 로직을 실행하고, 성공하면 커밋하고, 예외가 발생해서 실패하면 롤백한다.'라는 동일한 구조가 반복되는 문제를 해결하기 위해 템플릿 콜백 패턴을 활용했다고 합니다!

현재 코드도 완벽하지만 호옥시나 디노가 try-catch 중복을 해결하고 싶으실까봐
request changes로 남겨두겠습니다.
만약 다음단계나 추후에 천천히 개선하고 싶으시다면 바로 다시 리뷰요청 주세요!!
바로 approve 하겠습니다!!!

화이팅팅팅👨‍🎤🧑‍🎤👩‍🎤

pstmt.executeUpdate();
} catch (SQLException e) {
log.error(e.getMessage(), e);
throw new RuntimeException(e);
throw new DataAccessException(e.getMessage());
Copy link

@hanueleee hanueleee Oct 4, 2023

Choose a reason for hiding this comment

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

지금 당장 크게 중요한 부분은 아니지만..!!

예외를 전환할 때에는 기존예외를 포함해주는게 좋다고 합니다!!
기존 예외를 포함해야 추후에 에러에 대한 로그를 찍을 때 기존 예외의 스택 트레이스를 확인할 수 있기 때문입니당 (Caused by 부분)

image

현재 뼈대코드에서 제공되는 DataAccessException에는 Throwable cause를 파라미터로 받는 생성자도 열려있어서 얘를 사용하면 좋을 것 같아요 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

Caused by에 저런 식으로 나올거라고 생각하지 못했네요.. e를 담아서 던지도록 변경했습니다.!

c62edf6

}
}

@Nullable
public <T> T queryForObject(final String sql, final RowMapper<T> rowMapper, final Object... objects) {
try (final Connection conn = dataSource.getConnection();
final PreparedStatement pstmt = conn.prepareStatement(sql)
final ResultSet rs = preparedStatementAndSetValue(conn, sql, objects).executeQuery()

Choose a reason for hiding this comment

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

Suggested change
final ResultSet rs = preparedStatementAndSetValue(conn, sql, objects).executeQuery()
final PreparedStatement pstmt = preparedStatementAndSetValue(conn, sql, objects)
final ResultSet rs = pstmt.executeQuery()

조그마한 제안입니다 ㅎㅎ
한 번 끊어주는 것은 어떨까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

아주 좋습니다ㅎㅎ

@jjongwa
Copy link
Member Author

jjongwa commented Oct 5, 2023

안녕하세요 오잉!👨‍🎤🧑‍🎤👩‍🎤
몸은 괜찮으신지 모르겠네요..
환절기라 그런지 이번 연휴때 아픈 사람이 많은것 같아요.. 저도 살살 느낌이 오는것 같긴 한데..허허
이제 한 달 정도 남았으니 조금 더 힘내 봅시다!!💪

오잉이 말씀하신 템플릿 콜백 패턴을 적용해 봤는데요..!(이게 맞는진 모르겠지만)
현재 상태에선 그냥 JdbcTemplate의 메서드 구조를 조금 더 깔끔하게 가져간다는 장점 말고는 딱히 필요성을 느끼지 못했어요..
그래서 우선 시도는 해봤지만 딱히 필요 없으면 롤백할 생각도 있습니다.!

이대로 진행할지 롤백할지 오잉의 의견 들어보고 싶어서 우선 남겨놨어요!
이번에도 잘 부탁드립니다ㅎㅎ🙇

@jjongwa
Copy link
Member Author

jjongwa commented Oct 5, 2023

try-catch 중복 제거를 이런식으로 할 수도 있겠군요..! 3b41fad
오잉도 이런 방식을 생각하신 건가요.!

Copy link

@hanueleee hanueleee left a comment

Choose a reason for hiding this comment

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

안녕하세요 다이노!!
리뷰 반영해주신 것 확인했습니다. 감사해요😆

디노가 바꿔주신 구조 확인했는데, 굉장히 신기한 접근법이네요..!!
템플릿 콜백 패턴보다는 템플릿 메소드 패턴같은 느낌이 들어요.
저도 이 두가지 패턴이 이름이 비슷해서인지 헷갈려서 좀 찾아봤었는데
요 링크가 뭔가 직관적으로 이해가 되길래 살짝쿵 첨부해봅니다!

현재 상태에선 그냥 JdbcTemplate의 메서드 구조를 조금 더 깔끔하게 가져간다는 장점 말고는 딱히 필요성을 느끼지 못했어요..
그래서 우선 시도는 해봤지만 딱히 필요 없으면 롤백할 생각도 있습니다.!

저도 현재 나름대로 템플릿 콜백 패턴을 적용시켜 코드를 제출했는데(코멘트로 달아주신 방식과 유사하게),
막상 만들고 보니 기존거가 나은 것 같은 느낌이 ㅜㅋㅋㅋ 좀 들더라구요..

이유는 이번 단계에서 요구하는 JdbcTemplate같은 경우에는 만들어야하는 메소드가 딱 정해져 있어서
열심히 패턴을 적용시켰더니 얼마안되는 메서드가 더 지저분해지기만 하는..? 듯한 인상이 들었기 때문입니다.
스프링에서 트랜잭션에 탬플릿 콜백 패턴을 적용시킨 상황과는 좀 다른 느낌!
(메서드 몇 개 안되고, 더 만들 가능성이 아직은 없음 <-> 트랜잭션은 사용처가 많기 때문에 트랜잭션이 필요한 모든 메서드에 해당 로직을 다 만들어줘야함)
아마 디노도 비슷한 이유에서 JdbcTemplate이 깔끔해졌을 뿐 딱히 필요성을 못 느꼈던 것 같아요.

그래서 제 결론은!
일단 이대로 두고, 다음 단계를 진행하다가 상황을 보고 롤백할지말지 결정하는건 어떨까요??
저도 그럴 예정이라 일단은 패턴 적용 시킨 코드 내비뒀거덩요 ㅎ

우선은 이번 단계 요구사항도 다 만족하셨고,
다양한 시도도 해보셨으니 머지하도록 하겠습니다!!

너무너무 수고많으셨어요.
다음단계도 화이팅팅 🦖🎸🎶 (일렉공룡이라는 뜻)

@hanueleee hanueleee merged commit 037523d into woowacourse:jjongwa Oct 5, 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