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단계] 제이미(임정수) 미션 제출합니다. #432

Merged
merged 10 commits into from
Oct 7, 2023

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Oct 4, 2023

안녕하세요 그레이!
빠르게 리뷰해 주셨는데 프로젝트부터 하다 보니 지금이네요... 😅
늦게나마 2단계 리뷰 요청드립니다.

또한, 지난 1단계에서 좋은 리뷰들 감사합니다!
대부분 변명이긴 하지만, 해당 pr에 답변 달아두었습니다. (1단계 pr 바로가기)

2단계를 보지 않고 1단계를 진행했는데, 알고 보니 2단계였던 내용들이 많네요.
그래서 이번 2단계에서는 지난번 그레이가 남겨주신 피드백과 중복 코드를 제거하는 방식에 대해 고민해 보았습니다.

qeuryForObject()에서 결과가 한 개가 아닌 경우에 대한 예외처리를 진행했으며,
try-with-resources 중복을 제거하는 방법에 대해 많은 분들이 콜백 패턴을 적용해 보았습니다.
그런데, 해당 패턴을 처음 학습해보기도 하고 익숙하지 않아 제가 제대로 적용한 것인지 잘 모르겠네요..!

그 외에도 좀 더 고민해 볼 만한 부분 혹은 수정했으면 하는 부분들이 있다면 말씀해 주시면 감사하겠습니다.
이번 리뷰도 잘 부탁드립니다 😊

@JJ503 JJ503 self-assigned this Oct 4, 2023
Copy link

@Kim0914 Kim0914 left a comment

Choose a reason for hiding this comment

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

제이미 안녕하세요 !

2단계 미션도 잘 구현해주셨네요 ㅎㅎ

궁금한 부분 코멘트 하나 달았는데 이것만 확인해주세요 !

Comment on lines +12 to +16
private static final RowMapper<User> rowMapper = (rs) -> new User(
rs.getLong("id"),
rs.getString("account"),
rs.getString("password"),
rs.getString("email")
Copy link

Choose a reason for hiding this comment

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

👍

final List<T> results = new ArrayList<>();

while (rs.next()) {
final T result = calculateResult(rowMapper, rs);
Copy link

Choose a reason for hiding this comment

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

while(rs.next())로 resultSet의 row가 있는지 확인한 상태인데 다시 calculateResult 메서드 내에서 rs.next()를 확인하는 이유가 궁금합니다 :)

바로 rowMapper.mapRow(rs)를 실행시키면 어떤 문제가 발생하나요 ?!😁

Copy link
Member Author

Choose a reason for hiding this comment

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

헉... 아무 문제도 없습니다... 🫢
당시 위와 같이 짰던 이유가 잘 기억이 안 나네요...
덕분에 수정했습니다. 감사합니다!

Comment on lines +30 to +35
pstmt -> {
final ResultSet rs = pstmt.executeQuery();
final List<T> results = getResults(rowMapper, rs);

return getSingleResult(results);
}, sql, args
Copy link

Choose a reason for hiding this comment

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

익명 함수를 잘 사용해주셨네요 !

@JJ503
Copy link
Member Author

JJ503 commented Oct 7, 2023

그레이 리뷰 감사합니다!
말씀해 주신 부분 수정 완료했습니다.
이와 함께 테스트가 실패하고 있어 해당 부분도 함께 수정해 주었습니다.

Copy link

@Kim0914 Kim0914 left a comment

Choose a reason for hiding this comment

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

리뷰 반영해주신 부분 확인했습니다 ㅎㅎ

다음 단계 진행하시죠 !!

@Kim0914 Kim0914 merged commit cdf5d06 into woowacourse:jj503 Oct 7, 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