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단계] 홍고(홍여진) 미션 제출합니다. #374

Merged
merged 10 commits into from
Oct 7, 2023

Conversation

hgo641
Copy link

@hgo641 hgo641 commented Oct 2, 2023

안녕하세요, 깃짱! 반갑습니다!
깃짱의 리뷰이가 되어서 넘 신나네여
커밋 범위

JdbcTemplate에서 세 개의 메소드를 구현해 UserDao의 중복을 제거해보았습니다!

  • update : 데이터 업데이트
  • query : 데이터 리스트 조회
  • queryForObject : 데이터 조회

데이터 조회는 RowMapper 함수형 인터페이스를 활용하고 있습니다! JdbcTemplate에서 sql 예외가 발생하면 항상 DataAccessException을 던지게 했습니다.

리뷰 잘 부탁드립니다 👍

@hgo641 hgo641 requested a review from gitchannn October 2, 2023 08:34
@hgo641 hgo641 self-assigned this Oct 2, 2023
Copy link

@gitchannn gitchannn left a comment

Choose a reason for hiding this comment

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

홍고 안녕하세요!
추석 즐겁게 보내셨나요〰️

코드 깔끔하게 잘 구현해 주셨네요!
리뷰 몇 개 남겼는데 확인 후 다시 리뷰 요청 주세요!


* [x] 데이터 업데이트 기능 구현
* [x] 데이터 조회 기능 구현
* [x] 데이터 리스트 조회 기능 구현

Choose a reason for hiding this comment

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

기능 목록 작성 👍

}
}

public <T> Optional<T> queryForObject(final String sql, final RowMapper<T> rowMapper, final Object... objects) {

Choose a reason for hiding this comment

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

rowMapper 이외에도 requiredType으로 반환하는 메서드도 (가능하면) 만들어보면 어떨까요!

Copy link
Author

@hgo641 hgo641 Oct 6, 2023

Choose a reason for hiding this comment

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

이후 단계에서 시간이 나면 해보겠습니다 😂😂!

return null;
public Optional<User> findByAccount(final String account) {
final var sql = "select id, account, password, email from users where account = ?";
return jdbcTemplate.queryForObject(sql, ROW_MAPPER, account);
}

Choose a reason for hiding this comment

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

account는 유니크한 값이 아니라, 만약에 두 개 이상의 값이 나오게 된다면 현재는 그냥 첫 번째 결과를 반환하고 있는 것 같아요.

홍고는 하나의 값을 쿼리하는 경우에 2개 이상이 존재하는 경우에
첫 번째 값 반환, 예외 처리 등등 여러 방법이 있을 것 같은데 왜 현재 방법을 선택했는지 궁금합니다!

Choose a reason for hiding this comment

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

또 아래에서는 값이 존재하지 않는 경우에 예외 처리를 했는데, 여기서는 왜 예외 처리를 하지 않았는지도 궁금합니당

Copy link
Author

@hgo641 hgo641 Oct 6, 2023

Choose a reason for hiding this comment

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

account는 유니크한 값이 아니라, 만약에 두 개 이상의 값이 나오게 된다면 현재는 그냥 첫 번째 결과를 반환하고 있는 것 같아요.

홍고는 하나의 값을 쿼리하는 경우에 2개 이상이 존재하는 경우에
첫 번째 값 반환, 예외 처리 등등 여러 방법이 있을 것 같은데 왜 현재 방법을 선택했는지 궁금합니다!

앗 ㅎㅎ... 여기는 그냥 account가 유니크한 값이라고 가정하고 코드를 작성했습니다! 미션 요구사항만 만족하는데에 집중하고 다른 부분은 크게 신경쓰지 못했습니다ㅠㅠ 일단 sql문에 unique옵션 추가하고, InMemoryUserRepository도 unique한 account만 들어오게 변경해놓겠습니다

e2ff1e2

Copy link
Author

Choose a reason for hiding this comment

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

또 아래에서는 값이 존재하지 않는 경우에 예외 처리를 했는데, 여기서는 왜 예외 처리를 하지 않았는지도 궁금합니당

이건 UserService에서는 예외 처리를 했는데 UserDao에선 예외 처리를 하지 않은 이유를 여쭤보시는 게 맞나요? 😀
제 개인적인 선호인데, "account와 일치하는 유저가 없을 시 예외를 던진다"는 해당 서비스의 비즈니스 요구사항으로 보고 Service객체에 몰았습니다! 활용도가 높은 Dao에서는 Optional로 반환하는 것이 편해서 이렇게 구현했습니다.


assertThat(user.getAccount()).isEqualTo("gugu");
}

@Test
void findByAccount() {
final var account = "gugu";
final var user = userDao.findByAccount(account);
final var user = userDao.findByAccount(account).get();

assertThat(user.getAccount()).isEqualTo(account);

Choose a reason for hiding this comment

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

현재 데이터 격리가 이루어지고 있지 않아서, 이 테스트 코드를 실행할 때에 데이터베이스에는 gugu가 여러 개 쌓여 있을 것 같아요. 그중에 물론 account만을 확인하기 때문에 테스트는 통과하지만, 다른 테스트와 테스트를 더 격리하면 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

👍👍 깃짱 짱 꼼꼼하시네여
974945b
@BeforeEach에서 JdbcTemplate을 사용해 truncate하게 했는데 어떤가요? 더 나은 방법 생각나시면 추천부탁드립니다! 🙇‍♂️🙇‍♀️

Choose a reason for hiding this comment

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

오토 인크리먼트 때문에
일단 세팅까지 초기화하는게 최선인 거 같은데, 베스트는 insert하고 저장된 객체를 다시 받아와서 아이디에 대한 의존성을 완전히 제거하는게 아닐까 생각해요!
근데 저도 그건 좀 무리여서 truncate하고 오토인크리먼트 리셋하는 식으로 했어요

Copy link

@gitchannn gitchannn 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,3단계는 한꺼번에 진행해도 좋을 것 같아요!

주말 잘 보내세요🌟

@gitchannn gitchannn merged commit 78fcf97 into woowacourse:hgo641 Oct 7, 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