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단계] 에단(김석호) 미션 제출합니다. #321

Merged
merged 16 commits into from
Sep 30, 2023

Conversation

cookienc
Copy link

안녕하세요 준팍! 에단입니다. 명절 잘 보내고 계신가요? 저는 잘 보내고 있습니다😄
이번 미션 함께 하게 되어서 영광입니다.
JDBC 미션 1단계 완료해서 리뷰 요청드립니다. 추석때 심심하시면 리뷰해주셔도 됩니다~
그럼 남은 연휴도 푹쉬고 다음주에 뵈요!

@cookienc cookienc self-assigned this Sep 29, 2023
@cookienc cookienc changed the base branch from main to cookienc September 29, 2023 08:07
Copy link
Member

@junpakPark junpakPark left a comment

Choose a reason for hiding this comment

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

사랑하는 에단 선생님께,

안녕하십니까?
저희가 함께 페어로 진행하였던 레벨1의 2번째 미션이
229일이 지나간 것을 깨닫게 되었습니다.

같은 온보딩조였던 에단에게 이것 저것 물으면서 진행했던 그때가
제 우테코 생활의 행복한 순간들 중 하나가 아니었나 싶습니다.

어느덧 시간은 흘러 레벨 4의 3번째 미션에서
리뷰어로서 에단을 만나게 되니 감회가 새롭습니다.

리뷰어라기보단, 여전히 에단에게 이것 저것 물어보는
잘 모르는 페어라고 생각해주세요.

본론으로 돌아가서, step1 코드 굉장히 잘 봤습니다.
제가 아둔하여 몇가지 질문들을 남겼으나,
요구 사항은 멋지게 만족하여주셔서 approve를 드렸습니다.

제 질문들은 step2에서 답해주시면 감사하겠습니다.
남은 연휴 푹 쉬시구요, 항상 환절기 건강 유의하시길 바라겠습니다.

수고하셨습니다~ 🌕🍁


setCondtion(params, pstmt);

final ResultSet rs = pstmt.executeQuery();
Copy link
Member

@junpakPark junpakPark Sep 30, 2023

Choose a reason for hiding this comment

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

ResultSet도 close() 해줘야 하는 자원이라는 사실!
알고 계셨나요...?

Copy link
Author

@cookienc cookienc Oct 2, 2023

Choose a reason for hiding this comment

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

ResultSet은 Statement가 close 될 때 자동으로 close가 된다는 사실!
알고 계셨나요...?

image

저는 당연히 몰랐습니다 ㅎㅎ 덕분에 공부했습니다 :)

Copy link
Member

Choose a reason for hiding this comment

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

https://www.phind.com/search?cache=h8csh4x43gnfdyz497l20af4

ㅎㅎ statement도 connection이 닫히면 자동적으로 닫힙니다.
그럼에도 불구하고 명시적으로 close 해주는게 좀 더 좋은 습관이라고 하네요~

Copy link
Author

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 resultSet) throws SQLException;
Copy link
Member

Choose a reason for hiding this comment

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

스프링 JDBC에선 RowMapper 인터페이스에 int rowNum도 받는데요!
이 rowNum의 역할은 무엇일까용~? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

현재 컬럼의 번호라고 알고있는데, 주로 로깅할 때 쓰인다고 해서 안만들었습니다!

return Optional.ofNullable(object);
}

return Optional.empty();
Copy link
Member

Choose a reason for hiding this comment

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

Optional 사용 굿굿~ 👍👍👍

) {
log.debug("query : {}", sql);

setCondtion(params, pstmt);
Copy link
Member

Choose a reason for hiding this comment

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

queryForObject에서도 다양한 param을 받을 수 있게 하셨군요? 👍👍👍

public UserHistoryDao(final DataSource dataSource) {
this.dataSource = dataSource;
}
private final JdbcTemplate jdbcTemplate;

public UserHistoryDao(final JdbcTemplate jdbcTemplate) {
Copy link
Member

Choose a reason for hiding this comment

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

오 UserHistoryDao도 고쳐주셨군요? 👍👍👍


public UserDao(final JdbcTemplate jdbcTemplate) {
this.dataSource = null;
this.jdbcTemplate = jdbcTemplate;
Copy link
Member

Choose a reason for hiding this comment

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

그냥 질문인데요!
DataSource를 받는 생성자는 왜 지우신건가요? ㅎㅎ

 public UserDao(final DataSource dataSource) {	   
        this.jdbcTemplate = new JdbcTemplate(dataSource);	
 }

이런 식으로 활용도 가능했을 것 같은데 지우신 이유가 궁금합니당 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

DataSource를 주입받는건 dataSource가 변경될 여지가 있다는 생각하에 dataSource를 주입한다고 생각해요. 그런데 dataSource가 바뀌면 그에 따라서 jdbcTemplate 구현이 달라질거 같아서 새로운 jdbcTemplate으로 주입받는게 낫지 않을까? 하는 생각으로 바꿔놨습니다

Copy link
Member

@junpakPark junpakPark Oct 2, 2023

Choose a reason for hiding this comment

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

Datasource가 바뀌면 jdbcTemplate의 구현이 달라질 것 같다는 말씀에 조금 헷갈리는 점이 있어서 여쭤봅니당!

Datasource는 인터페이스인데,어떤 부분이 jdbcTemplate의 구현에 영향을 미칠 수 있나요??

Copy link
Author

Choose a reason for hiding this comment

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

제가 뭔가 착각하고 있었나봐요🥲 mysql에서 oracle로 바뀌면 문법이 바뀌니까 jdbcTemplate도 바뀌지 않을까라는 생각을 했었네요. sql은 정작 parameter로 받는데 말이죠. 저의 잘못된 뇌피셜이었습니다.
추가로 다른 이유를 붙이자면 dao에서 직접 jdbctemplate을 사용하면 jdbcTemplate이 bean일 때 새로운 객체를 안만들어도 되겠네요. 내부구현이 안바뀌니깐요

Copy link
Member

Choose a reason for hiding this comment

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

그렇군요 ㅎㅎ 에단 덕분에 많이 배울 수 있었습니다. 감사합니다 👍👍👍

}
} catch (SQLException ignored) {}
}
return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, id);
Copy link
Member

Choose a reason for hiding this comment

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

해당 메서드에서 .orElseThrow() 하는 방법도 있었을 것 같은데요,
왜 Optional 타입으로 반환해주셨나요? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

dao는 DB에서 데이터를 찾아오는 역할만 하는게 좋다고 생각해요. 데이터가 찾을 때 문제가 생기면 dao에서 예외처리를 해도 되지만, 데이터의 존재 유무는 Service 영역에서 처리하게는게 책임에 맞다고 생각합니다~

@junpakPark junpakPark merged commit bef38cd into woowacourse:cookienc 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