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, 2단계] 다니(이다은) 미션 제출합니다. #19

Merged
merged 21 commits into from
Sep 30, 2021

Conversation

da-nyee
Copy link

@da-nyee da-nyee commented Sep 22, 2021

안녕하세요, 피카 ~! 🙌

이번 미션,, 처음에 감 잡는 게 너무 어려웠어요 🥲
그래도 강의 자료랑 JdbcTemplate 클래스 참고해서 열심히 구현했습니다 !

아직 테스트 코드는 작성하지 않았는데요..!
피드백 반영할 때 추가하겠습니다 ㅎㅎㅎ

코드 리뷰 무엇이든 전부 남겨주세요 ~~!
연휴 잘 보내고, 잘 부탁드립니다 !!! 🙇‍♀️

@da-nyee da-nyee requested a review from pika96 September 22, 2021 15:03
@da-nyee da-nyee self-assigned this Sep 22, 2021
Copy link

@pika96 pika96 left a comment

Choose a reason for hiding this comment

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

안녕하세요 다니!!! 피카입니다!!
너무 구현을 잘 해주셔서 눈씻고 찾아봐도 피드백 드릴 부분이 없네요.. ㅠㅠ!!
그만큼 다니가 잘한다는 뜻이겠죠?

UserDao를 만들었는데 기존 Controller에서 사용되는 InMemory를 바꾸어봐도 괜찮을 것 같아요!!
JdbcTemplateTest 테스트코드는 피드백 반영때 추가한다고 했으니 기다리고 있겠습니다!!


public class DataSourceConfig {

private static javax.sql.DataSource INSTANCE;
private static DataSource instance;
Copy link

Choose a reason for hiding this comment

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

이런 부분까지 꼼꼼하게!! 👍

Comment on lines 85 to 87
if (rs != null) {
rs.close();
}
Copy link

Choose a reason for hiding this comment

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

ResultSet 까지 잊지 않고 close 해주는 모습!! 꼼꼼하네요!

Comment on lines 27 to 32

connection = dataSource.getConnection();
statement = connection.createStatement();
statement.execute(sql);
} catch (NullPointerException | IOException | SQLException e) {
log.error(e.getMessage(), e);
} catch (Exception e) {
LOG.error(e.getMessage(), e);
Copy link

Choose a reason for hiding this comment

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

예외 리팩토링 좋네요!! 미션과는 관계는 없지만 리팩토링 하는김에 try with resource 리팩토링도 같이 하면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

오오 여기도 try-with-resources 적용할 수 있었네요! 반영했습니다 ~!

Copy link
Contributor

@kang-hyungu kang-hyungu left a comment

Choose a reason for hiding this comment

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

다니 간단한 피드백 남겼으니 참고하세요

}
}

public <T> List<T> query(String sql, RowMapper<T> rowMapper) {
Copy link
Contributor

Choose a reason for hiding this comment

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

이 메서드는 findAll 전용인가요?
쿼리에 파라미터를 전달 할 수 있도록 만들어야 라이브러리를 사용하는 입장에서 편리하지 않을까요?

Copy link
Author

@da-nyee da-nyee Sep 29, 2021

Choose a reason for hiding this comment

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

c223394 해당 메소드에 가변 인자를 추가했습니다 !

} finally {
try {
if (rs != null) {
rs.close();
Copy link
Contributor

Choose a reason for hiding this comment

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

ResultSet을 try with resources로 처리할 방법 없을까요?

Copy link
Author

@da-nyee da-nyee Sep 29, 2021

Choose a reason for hiding this comment

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

dfef639 try-with-resources를 적용했습니다 ~!

return jdbcTemplate.queryForObject(sql, this::getUser, account);
}

private User getUser(ResultSet rs) throws SQLException {
Copy link
Contributor

Choose a reason for hiding this comment

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

메서드명이 적절한가요?
객체를 생성하는 메서드이니 get이 들어가는게 알맞게 보이진 않네요

Copy link
Author

Choose a reason for hiding this comment

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

29762e4 createUser로 메소드명을 변경했습니다 ~!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

87.5% 87.5% Coverage
0.0% 0.0% Duplication

@da-nyee
Copy link
Author

da-nyee commented Sep 29, 2021

피카, 구구 리뷰 감사합니다!! 🙇‍♀️

피드백 전부 반영했고, JdbcTemplateTest도 작성했습니다 !
이번에도 잘 부탁드립니다 ㅎㅎㅎ

@pika96
Copy link

pika96 commented Sep 30, 2021

다니 깔끔하게 리팩토링 하셨군요!!
테스트 하나하나까지 꼼꼼히 작성하셨네요!

추가로 첨언을 드리자면, jdbcTemplate에서 중복되는 try catch 부분을 콜백 패턴으로 줄이는걸 고민해보셔도 될 것 같아요!
그래도 테스트랑 코드까지 깔끔하게 짜셨으니 이만 머지하겠습니다! 🙏
남은 우테코 기간 동안 화이팅하세요!!

Copy link

@pika96 pika96 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!!!

@pika96 pika96 merged commit 694b9ba into woowacourse:da-nyee Sep 30, 2021
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