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단계] 성하(김성훈) 미션 제출합니다. #311

Merged
merged 6 commits into from
Sep 30, 2023

Conversation

sh111-coder
Copy link

안녕하세요 루쿠!! 이번 JDBC 미션 리뷰이 성하입니다!! 잘부탁드려요 ㅎㅎㅎㅎ
같은 캠퍼스였으면 인사했을텐데 아쉽네요 ㅠㅠ 보투게더 그리워요 😭

이번 1단계에서는 서비스 코드(app 모듈) Dao의 기나긴 코드를 라이브러리 모듈(jdbc 모듈)로 옮기는 작업을 했습니다!!
기본적으로 구현 내용은 다 비슷할 것 같아서 따로 언급할 것은 없을 것 같습니다!

그나마 신경썼던 점은 queryForObject() 시에 Result가 1개 이상이라면
실제 JDBC에서도 IncorrectResultSizeDateAccessException이 발생하는데요!
해당 부분을 똑같이 Result가 1개 이상이면 예외를 발생시키도록 구현했습니다!

이러한 예외를 추가하면서 서비스 모듈의 기존 테스트(UserDaoTest)의 데이터들이
전체 테스트 수행시 남아있어서 테스트의 독립성이 지켜지지 않았습니다!

따라서, truncate.sql을 추가해서 각 테스트마다 truncate을 해주는 작업을 추가했습니다~!

라이브러리 모듈의 테스트(JdbcTest)도 진행해보려고 했으나 어떻게 테스트해야할지 잘 모르겠어서
일단 제출합니다!! 😭

리뷰 잘부탁드려요 루쿠! 👍🏻

@sh111-coder sh111-coder self-assigned this Sep 28, 2023
Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 성 하 ✋~ 추석 연휴는 잘 보내고 계신가요
갑자기 다른 캠퍼스로 이주가셔서 아쉽지만 이번에 이렇게 미션에서라도 만나게 되서 반갑습니다!!
간단하게 질문과 커멘트 남겨드렸는데 한번 확인 해주시면 좋을것 같아요~

Comment on lines +7 to +18
public abstract class JdbcUtils {

public static void closeStatement(final Statement statement) {
try {
if (statement != null) {
statement.close();
}
} catch (SQLException ignored) {
throw new CannotCloseStatementException("Failed to close Statement");
}
}
}

Choose a reason for hiding this comment

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

따로 유틸클래스로 만들어 주셨군요!
해당 클래스를 만드신 이유와 추상클래스로 만드신 이유가 궁금해요~!

Copy link
Author

Choose a reason for hiding this comment

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

기존 로직에서 Connection 관련해서 작업을 DataSourceUtils에서 진행하고 있는데요!
그래서 statement를 close하는 작업도 Util 클래스로 만들어서 해당 클래스에 책임을 위임했습니다!
유틸 클래스이기 때문에 따로 인스턴스를 생성할 필요가 없어서 DataSourceUtils와 마찬가지로 추상 클래스로 생성했습니다!

Comment on lines 24 to 26
public UserDao(final JdbcTemplate jdbcTemplate) {
this.dataSource = null;
this.jdbcTemplate = new JdbcTemplate(DataSourceConfig.getInstance());
}

Choose a reason for hiding this comment

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

생성자 파라미터로 JdbcTemplete을 받고있지만 해당 파라미터를 사용하지 않고 내부에서 인스턴스를 생성하고 있어요~

Copy link
Author

Choose a reason for hiding this comment

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

앗 그렇네요!! 놓치고 있었습니다! 바로 수정했습니다 감사합니다 ㅎㅎㅎ

@@ -14,4 +22,87 @@ public class JdbcTemplate {
public JdbcTemplate(final DataSource dataSource) {
this.dataSource = dataSource;
}

public void update(final String sql, final Object... args) {
Connection connection = DataSourceUtils.getConnection(dataSource);

Choose a reason for hiding this comment

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

DataSourceUtils 해당 클래스에서 주석보면 4단계에서 사용하라고 하시는데요?!

Copy link
Author

Choose a reason for hiding this comment

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

엇.. 이 부분은 못 봤네요! 헣 처음에 UserDao에 남아있던 로직을 봤을 때 DataSourceUtils의 메소드를 사용해서
처리할 수 있길래 사용했습니다!
이 부분은 괜찮겠죠?!

Comment on lines +29 to +44
try {
pstmt = connection.prepareStatement(sql);
log.debug("query : {}", sql);

setPreparedStatementArguments(pstmt, args);
pstmt.executeUpdate();
} catch (SQLException e) {
DataSourceUtils.releaseConnection(connection, dataSource);

log.error(e.getMessage(), e);
throw new RuntimeException(e);
} finally {
JdbcUtils.closeStatement(pstmt);
DataSourceUtils.releaseConnection(connection, dataSource);
}
}

Choose a reason for hiding this comment

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

try-with-resources를 사용하시는건 어떤가요??
(2단계를 위해서 남겨두신건가요?!)

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다!! 2단계에 있길래 일단은 냅뒀어요!

Comment on lines +9 to +10
T mapRow(ResultSet rs, int rowNum) throws SQLException;
}

Choose a reason for hiding this comment

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

파라미터로 rowNum을 받게 한 이유가 궁금해요!

Copy link
Author

Choose a reason for hiding this comment

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

이 부분 구현할 때는 실제 JDBC를 참고하고, RowMapper를 사용했었을 때를 떠올려봤어요!
그때 rowNum도 인자로 받는 것을 생각하고 구현했습니다!
지금은 RowMapper 람다로 구현 시 rowNum을 사용하지 않지만, row의 인덱스별 처리가 필요한 경우에 유용할 수 있을 것 같아요!

Comment on lines 18 to 21
void setup() {
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance(), ResourceNames.SCHEMA_RESOURCE_NAME);
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance(), ResourceNames.TRUNCATE_RESOURCE_NAME);

Choose a reason for hiding this comment

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

truncate 👍 👍

Comment on lines 35 to +36
void findById() {
final var user = userDao.findById(1L);
final var user = userDao.findAll().get(0);

Choose a reason for hiding this comment

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

@beforeeach에서 유저를 insert할때 id값도 1로 지정해서 넣어주면 어떨까요??

Copy link
Author

Choose a reason for hiding this comment

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

다시 봐보니 findById 메소드 테스트인데 findById를 사용하지 않고 있었네요 ㅠㅠ

루쿠가 말해주신 @BeforeEach에서 유저의 ID를 1로 지정하고 전체 테스트를 수행했을 때
엔티티의 ID를 1로 넣는다고 해서 고정이 되지 않더라구요!

insert해서 DB에 들어가는 ID는 Auto Increment라서 엔티티의 ID 필드와 상관없이 조회 시에 DB PK를 가져오기 때문에
1씩 증가한 ID를 가져오는 것 같아요!

그래서 @BeforeEach는 그대로 놔두고, 테스트에서 findById를 사용하는 식으로 리팩토링했습니다!!

Copy link

@aiaiaiai1 aiaiaiai1 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단계로 넘어가보시죠 🚀

@aiaiaiai1 aiaiaiai1 merged commit 12fb7ce into woowacourse:sh111-coder 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.

None yet

3 participants