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단계] 지토(김지민) 미션 제출합니다. #362

Merged
merged 13 commits into from
Oct 3, 2023

Conversation

apptie
Copy link

@apptie apptie commented Oct 1, 2023

안녕하세요 오리
추석 연휴 잘 지내고 계신가요?
저는 뭐...집에서 코딩이나 했습니다


리뷰 반영의 경우 JdbcTemplate 메서드 네이밍 변경 & userRowMapper static 필드화를 적용했습니다
메서드 네이밍...참 어려운 것 같습니다


이번 step2에서는 뭘 해야하나 좀 감이 안잡혔습니다
그래서 고민좀 하다가 힌트랑 transaction.stage1.jdbc 패키지의 있는 내용을 살짝 봤는데 함수형 인터페이스를 활용해서 코드의 중복을 없애는 느낌인 것 같더라고요..?
저는 이전 step1 정도가 코드의 중복은 있을지언정 가독성은 더 좋다고 생각해서 상상도 못하고 있었는데 좀 당황스러웠습니다
아무튼 코드의 중복(특히 try-with-resource 파트)을 없애는 것이 2단계 미션에서의 목표라고 생각해서 함수형 인터페이스와 템플릿 콜백 패턴을 사용해봤습니다

JdbcTemplate, PreparedStatementTemplate, ResultSetTemplate 이런 식으로 시리즈 느낌이 나도록 처리했습니다

JdbcTemplate은 Connection과 PreparedStatement에 대한 자원을 관리하는 PreparedStatementTemplate.execute() 메서드만 호출하도록 했습니다
PreparedStatement에서 ResultSet에서 조회 결과를 꺼내야 하는 경우는 ResultSet에 대한 자원을 관리하는 ResultSetTemplate.execute()만 호출하도록 했습니다

각각의 템플릿 클래스에서 발생하는 예외는 별도로 분리했습니다


추가적으로 결과 단일 조회 메서드에서 여러 결과가 조회된 경우 예외를 발생시키도록 예외 로직을 추가했고, 이로 인해 UserDaoTest 코드를 살짝 리펙토링을 진행했습니다


이정도입니다
남은 연휴도 잘 보내시길 바랍니다
그런 의미로 리뷰는 천천히...푹 쉬시다가 해주시면 감사하겠습니다
아무튼 리뷰 잘 부탁드립니다

Copy link

@carsago carsago left a comment

Choose a reason for hiding this comment

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

지토 고생하셨습니다. 이번 단계는 merge할게요 본인 템포에 맞게 편하게 해주세요 ㅋㅋ
그리고 코멘트도 있으니 편할때 읽어주세요.

Comment on lines +17 to +27
private User persistUser;

@BeforeEach
void setup() {
void setUp() {
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());

userDao = new UserDao(DataSourceConfig.getInstance());
final User user = new User("gugu", "password", "[email protected]");
userDao.insert(user);
persistUser = userDao.findByAccount("gugu");
}
Copy link

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.

👍

Comment on lines +22 to +26
public int executeQuery(final String sql, final Object... statements) {
return preparedStatementTemplate.execute(
connection -> bindStatements().bind(connection.prepareStatement(sql), statements),
PreparedStatement::executeUpdate
);
Copy link

Choose a reason for hiding this comment

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

bind 로직은 전부 다 중복된 형태로 들어가는데 아예 preparedStatmentTemplate의 내부로 넘겨버리는 건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

앗 맞습니다 이걸 왜 발견 못했나 싶네요
다음에는 적용하도록 하겠습니다

Comment on lines +60 to +73
return preparedStatementTemplate.execute(
connection -> bindStatements().bind(connection.prepareStatement(sql), statements),
preparedStatement -> resultSetTemplate.execute(
preparedStatement,
resultSet -> {
final List<T> result = new ArrayList<>();

while (resultSet.next()) {
result.add(rowMapper.mapRow(resultSet));
}

bindStatements(preparedStatement, statements);
return preparedStatement;
return result;
}
)
Copy link

Choose a reason for hiding this comment

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

취향차일 수도 있지만, ResultSet 매핑 로직이 이정도로 길어진다면 메소드 추출하는 것도 괜찮을 것 같아요.

Copy link
Author

Choose a reason for hiding this comment

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

저도 이번에 코드를 작성하면서 좀...불편하기는 하더라고요
이후 리펙토링 하도록 하겠습니다

Comment on lines 10 to 11
public class JdbcTemplate {

Copy link

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.

의견 공유 감사합니다 👍

@carsago carsago merged commit 2864b3b into woowacourse:apptie Oct 3, 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