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 라이브러리 구현하기 - 1단계] 제이미(임정수) 미션 제출합니다! #335

Merged
merged 8 commits into from
Sep 30, 2023

Conversation

JJ503
Copy link
Member

@JJ503 JJ503 commented Sep 29, 2023

안녕하세요 그레이!
jdbc 미션을 통해 만나게 되어 반갑습니다!

이번 미션은 레벨 2 때 사용했던 JdbcTemplate에 대한 기억을 되살려가며 구현해 보았습니다.
리뷰 잘 부탁드립니다!

커밋 범위 링크

남은 연휴 잘 보내세요!

@JJ503 JJ503 self-assigned this Sep 29, 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.

안녕하세요 제이미, 그레이입니다 ㅎㅎ

이번 미션 함께하게 되어서 기대가 되네요 😀
1단계 너무 잘 구현해주셨네요 ㅎㅎ 몇 가지 궁금한 부분은 커멘트로 남겼어요.
다음 단계 진행하면서 적용해봐도 될 것 같아 이번 단계는 바로 approve해도 될 것 같아요.

리뷰 요청은 언제든지 환영입니다 :)
남은 연휴 잘 보내세요 ~~ 🙇🏻

Comment on lines +13 to +17
rs.getLong(1),
rs.getString(2),
rs.getString(3),
rs.getString(4)
);
Copy link

Choose a reason for hiding this comment

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

컬럼의 index와 컬럼의 name으로 추출하는 두 가지의 방법이 존재하는데 인덱스로 추출해주신 이유가 궁금합니다 ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

기존에 코드가 그렇게 주어졌기에 크게 생각하지 않고 바로 인덱스로 추출하는 방식을 사용하게 되었습니다..!
그레이가 말씀해 주셔서 고민해 본 결과 저는 컴럼명을 통해 추출하는 방식이 보다 명확해 보여서 좋을 것 같네요.
다만, 컬럼명이 수정된다면 계속 변경해야겠지만, 그럴 일은 적을 것 같아 가독성을 선택하도록 하겠습니다!
해당 방법으로 수정해 두었습니다.

그레이는 어떤 방식으로 진행하셨고 이유가 무엇인지 궁금하네요!

Copy link

Choose a reason for hiding this comment

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

저는 컬럼 순서보다 컬럼 명이 읽기가 명확하고, 잘못된 값을 가져오는 상황을 방지할 수 있기 때문에 컬럼 명으로 가져오는 방법을 선택했씁니다 ㅎㅎ

@FunctionalInterface
public interface RowMapper<T> {

T mapRow(final ResultSet rs, final int rowNum) throws SQLException;
Copy link

@Kim0914 Kim0914 Sep 30, 2023

Choose a reason for hiding this comment

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

제가 1단계를 진행하면서 느낀 궁금점인데요 !

RowMapper에서 rowNum은 어떤 역할을 하는 파라미터일까요?!

Copy link
Member Author

Choose a reason for hiding this comment

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

해당 파라미터는 그랬던 것 같아서 넣은 거였는데 제가 전혀 사용하지 않는군요..!
찾아보니 반복되는 결과 집합에서 행 번호를 나타낸다고 하네요.
그런데, 해당 파라미터를 현재 로직에서 어떻게 사용해야 할지 감이 오지 않기도 하고, 현재는 사용하지 않기에 제거했습니다.

}
}

private static <T> T calculateResult(final RowMapper<T> rowMapper, final ResultSet rs) throws SQLException {
Copy link

Choose a reason for hiding this comment

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

static은 삭제해도 될 것 같은데 명시해주신 이유가 궁금하네요 😄

Comment on lines +63 to +65
if (rs.next()) {
return rowMapper.mapRow(rs, rs.getRow());
}
Copy link

Choose a reason for hiding this comment

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

queryForObject에서 호출되는 메서드인데 한 건의 결과가 아닌 경우 적절히 예외 처리를 해줘도 좋을 것 같아요 !! ㅎㅎ

@Kim0914 Kim0914 merged commit 0679fae into woowacourse:jj503 Sep 30, 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.

3 participants