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단계] 비버(전인표) 미션 제출합니다. #370

Merged
merged 10 commits into from
Oct 2, 2023

Conversation

ingpyo
Copy link

@ingpyo ingpyo commented Oct 2, 2023

안녕하세요 로지!
이번에 추석연휴때 잘보내시고있나요?
2단계가 이렇게하는건지 잘 모르겠네요..

리뷰 잘부탁드립니다!!

Copy link
Member

@kyY00n kyY00n left a comment

Choose a reason for hiding this comment

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

네, 비버 안녕하세요!
기다리고있었습니다~ 2단계까지 해주셨군요! 리팩터링 과정을 차근차근 커밋에 남겨주셔서 리뷰하기 굉장히 편했습니다. 감사해요👍🏻
너무 잘 해주셔서 자잘하게 궁금한 것들만 코멘트로 남기고 바로 어프루브하려고 합니다. 답변은 여기 댓글로 남겨주시면 확인하겠습니다!
1&2단계 수고하셨습니다. 3단계에서 봬요~

@@ -16,7 +16,8 @@ public UserService(final UserDao userDao, final UserHistoryDao userHistoryDao) {
}

public User findById(final long id) {
return userDao.findById(id);
return userDao.findById(id)
.orElseThrow(() -> new IllegalArgumentException("일치하는 사용자가 없습니다."));
Copy link
Member

Choose a reason for hiding this comment

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

존재하지 않는 경우의 테스트를 작성해보시면 좋을 것 같습니다~

final PreparedStatement preparedStatement = getPreparedStatement(sql, connection, parameters);
final ResultSet resultSet = preparedStatement.executeQuery()) {
log.debug("query : {}", sql);
if (resultSet.next()) {
Copy link
Member

Choose a reason for hiding this comment

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

메서드 명으로 보아, 하나의 객체만 쿼리의 결과로 기대하는 것 같습니다.
그런데 쿼리 결과가 여러 행이 나올 경우를 예외로 처리하지 않은 이유가 궁금합니다!

@kyY00n kyY00n merged commit d8751e9 into woowacourse:ingpyo Oct 2, 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