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, 3단계] 후디(조동현) 미션 제출합니다. #199

Merged
merged 9 commits into from
Oct 13, 2022

Conversation

devHudi
Copy link

@devHudi devHudi commented Oct 10, 2022

안녕하세요, 리차드 🎶 뭔가 오랜만에 리뷰 요청 드리는 것 같네요 😅
2단계 힌트를 보니 이미 1단계에서 미션 의도대로 설계가 된 것 같아서 지난번 말씀드린 것 처럼 3단계까지 함께 리뷰 요청 드려요.
(혹시 2단계 리팩토링 부분에서 제가 놓친게 있다면 리뷰 부탁드립니다 ㅎㅎ;)

3단계는 스프링이 트랜잭션 동기화와 추상화를 어떻게 하고 있는지 이해가 필요해서 조금 오래걸렸던 것 같아요.
관련하여 공부를 좀 했는데 https://hudi.blog/spring-transaction-synchronization-and-abstraction/ 에 정리해두었으니 참고하면 좋을 것 같아요.

이번 리뷰도 잘 부탁드립니다 👋

@devHudi devHudi requested a review from HJ-Rich October 10, 2022 11:10
@devHudi devHudi self-assigned this Oct 10, 2022
Copy link

@HJ-Rich HJ-Rich left a comment

Choose a reason for hiding this comment

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

후디~ 빠르게 잘 구현해주셨네요! 💯
훌륭합니다~! 👏👏👏

몇가지 소소한 의견 남겨봤습니다.
필수 요구사항은 이미 모두 충족하셔서 선택적으로 반영해주시면 될 것 같습니다.

그리고 혹시 가능하시다면 JdbcTemplateTest를 구현해보시는 것도 도움이 될 것 같습니다.
거의 다 왔네요~! 화이팅! 🎶

Comment on lines +56 to +60
// 테스트 격리를 위한 메소드
public void truncate() {
String sql = "truncate table users restart identity"; // table을 truncate 한 후 identity도 1로 초기화
jdbcTemplate.update(sql);
}
Copy link

Choose a reason for hiding this comment

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

테스트에서만 사용되는 메서드를 프로덕션에 구현하는 방법에 대해 후디는 어떤 의견이신가요?~
그리고 현재 방식이라면 JDBC 벤더사가 달라지면 truncate 문법이 달라질 수 있어서 이슈가 될 수 있을 것 같아요~!

Comment on lines +29 to +40
@Override
public void changePassword(final long id, final String newPassword, final String createBy) {
TransactionStatus status = transactionManager.getTransaction(new DefaultTransactionDefinition());

try {
userService.changePassword(id, newPassword, createBy);
transactionManager.commit(status);
} catch (RuntimeException e) {
transactionManager.rollback(status);
throw new DataAccessException(e);
}
}
Copy link

Choose a reason for hiding this comment

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

JdbcTemplate은 DataAccessException 만 던지는데요,
RuntimeException으로 잡아주신 후디의 생각이 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

TransactionManagerTransactionException 을 함께 던지기 때문에 RuntimeException 으로 묶어 처리했습니다!

Comment on lines 62 to 66
final var actual = userService.findById(1L);

System.out.println("actual = " + actual);

assertThat(actual.getPassword()).isNotEqualTo(newPassword);
Copy link

Choose a reason for hiding this comment

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

sysout 발견~! 😆

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 +40 to 43
while (rs.next()) {
T result = rowMapper.run(rs);
results.add(result);
}
Copy link

Choose a reason for hiding this comment

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

👍

PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
Connection connection = DataSourceUtils.getConnection(dataSource);

try (PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
Copy link

Choose a reason for hiding this comment

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

64번 라인에 TODO 는 혹시 작업이 완료된 걸까요?!

Copy link
Author

Choose a reason for hiding this comment

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

이대로 예외 던지기로 해서 TODO 코멘트를 지웠습니다!

Comment on lines -51 to -52
try (Connection connection = dataSource.getConnection();
PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
Copy link

Choose a reason for hiding this comment

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

해당 라인에 남길 수 없어서 여기에 남깁니다 ! ㅎ
50번 라인을 이렇게 해보면 어떨까요?
JdbcTemplate의 실제 구현이 이렇게 되어있더라고요~

AS-IS
return results.get(FIRST_RESULT_INDEX);

TO-BE
return results.iterator().next();

Copy link
Author

Choose a reason for hiding this comment

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

오 너무 좋은 아이디어네요! 앞으로도 첫번째 값만 필요하면 이터레이터를 가져와서 사용하는 방법을 써야겠네요 👍

@HJ-Rich HJ-Rich merged commit 939e83f into woowacourse:devhudi Oct 13, 2022
@HJ-Rich
Copy link

HJ-Rich commented Oct 13, 2022

후디 고생 많으셨습니다! 👍

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