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단계] 글렌(전석진) 미션 제출합니다. #308

Merged
merged 15 commits into from
Sep 29, 2023

Conversation

seokjin8678
Copy link

@seokjin8678 seokjin8678 commented Sep 28, 2023

안녕하세요 코코닥! 👃👃🐔

1단계 미션 완료하고 제출합니다!
대부분의 변경 사항은 JdbcTemplate 클래스를 보시면 될 것 같습니다!
람다식과 함수형 인터페이스를 활용하여 최대한 깔끔하게 구현해보려고 노력했습니다..!

리뷰 기다리겠습니다!
감사합니다!

커밋 범위

@seokjin8678 seokjin8678 self-assigned this Sep 28, 2023
Copy link
Member

@kokodak kokodak left a comment

Choose a reason for hiding this comment

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

존경하는 글렌쿤 안녕하세요! 👃👃🐔 입니다

코드 잘 보고 가요. 워낙 깔끔하게 잘 작성해주셨고, 1단계 요구사항 이상으로 잘해내주셔서 바로 Approve & Merge 하도록 하겠습니다!

개인적으로 궁금한 점들은 코멘트로 남겨두었는데, 나중에 한번 확인해주시면 감사하겠습니다.

남은 추석 연휴 잘 보내시고, 나머지 미션들도 화이팅입니다!!!

Comment on lines +11 to +17
private static final ResultSetMapper<User> USER_MAPPER = rs -> {
long userId = rs.getLong(1);
String userAccount = rs.getString(2);
String password = rs.getString(3);
String email = rs.getString(4);
return new User(userId, userAccount, password, email);
};
Copy link
Member

Choose a reason for hiding this comment

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

크루들마다 이 부분을 인스턴스 변수로 둔 사람도 있고, static 변수로 둔 사람도 있는데, 글렌은 ResultSetMapper 를 static 변수로 둔 이유가 무엇인가요!!

Copy link
Member

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.

크루들마다 이 부분을 인스턴스 변수로 둔 사람도 있고, static 변수로 둔 사람도 있는데, 글렌은 ResultSetMapper 를 static 변수로 둔 이유가 무엇인가요!!

인스턴스 변수로 둔다면, 매번 UserDao 인스턴스를 생성할 때 마다 해당 객체가 생성될 것 같아 성능 측면에서 비효율적일 것 같아요! 물론 싱글턴 패턴을 적용거나 DI 프레임워크를 사용한다면 이러한 비효율은 해결되겠지만..
하지만 중요한 것은 의도라고 생각해요.
해당 객체는 상태를 가지지 않고, 말그대로 Mapper 역할을 하는 객체이기 때문에 static 영역으로 선언하였습니다!

Copy link
Author

Choose a reason for hiding this comment

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

추가적으로, 인덱스로 ResultSet 에서 값을 가져오는 방식과, 컬럼 이름으로 값을 가져오는 방식이 있을 것 같은데, 글렌은 어떤 것을 선호하시나요?

컬럼 이름과 인덱스를 사용하는 것은 상황에 따라 다를 것 같아요!
와일드 카드를 사용하여 모든 컬럼을 가져올 때는 컬럼 이름을 사용하는 것이 더 유용할 것 같아요! (select * from ...)
SQL문에 컬럼이 보이지 않으니 잘못된 인덱스를 입력할 수 있기 때문에요!

하지만 지금과 같이 모든 컬럼을 명시한 SQL문을 사용한 경우에는 인덱스를 사용하는 것이 더 좋아보여요!
이미 SQL문에 컬럼명을 명시했기 때문에 굳이 컬럼 이름으로 가져올 필요가 없다고 생각해요!

Copy link
Member

Choose a reason for hiding this comment

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

모두 근거있고 타당하다고 느껴집니다! 답변 감사합니다 글렌

// todo
return null;
final var sql = "select id, account, password, email from users where account = ?";
return jdbcTemplate.queryForObject(sql, ps -> ps.setString(1, account), USER_MAPPER);
Copy link
Member

Choose a reason for hiding this comment

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

가변인자로 값을 넘겨서 JdbcTemplate 내부적으로 처리해주는 방법도 있을 것 같아요.

어떤 점에서 글렌은 람다식으로 넘기는 방식을 선택하셨나요? (느끼시는 장단점을 말해주셔도 좋아요!!)

Copy link
Author

Choose a reason for hiding this comment

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

PreparedStatement에 대한 람다를 사용하는 것이 더 직관적이라 판단하여 사용했습니다!
지금 같이 파라미터가 1개인 상황에서는 가변 인자를 넘기는 것이 더 좋아보이기는 하네요. 😁
그리고 가변인자를 사용할 때 내부적으로 배열을 생성하고 초기화하기 때문에 성능에 영향이 있을 수 있어요!
(log.info(), List.of() 메서드의 시그니처를 확인해보시면 단순히 가변 인자만 받지 않고, 여러 개의 인자를 받을 수 있는 메서드를 다중 정의 해놓은 것을 볼 수 있어요!)

Copy link
Member

@kokodak kokodak Oct 2, 2023

Choose a reason for hiding this comment

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

전 가변인자를 사용하는 방향으로 구현했었는데, 성능 문제가 있다는 점은 처음 알았네요..

관련해서 찾아봤는데, 이펙티브 자바에도 이에 관한 내용이 있더라구요. List.of() 같은 메서드에서, 가변인자 쓰면 되는데 왜 다중 정의를 해놨지? 했던 기억이 있었는데.. 글렌 덕분에 알아갑니다!! 감사해요


import static org.assertj.core.api.Assertions.assertThat;
import org.springframework.dao.EmptyResultDataAccessException;
import org.springframework.dao.IncorrectResultSizeDataAccessException;

class UserDaoTest {
Copy link
Member

Choose a reason for hiding this comment

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

테스트 코드 좋네요!! 👍👍

Comment on lines +32 to +39
if (rs.next()) {
T result = rsMapper.apply(rs);
if (!rs.next()) {
return result;
}
throw new IncorrectResultSizeDataAccessException();
}
throw new EmptyResultDataAccessException();
Copy link
Member

Choose a reason for hiding this comment

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

역시 글렌! 정말 꼼꼼한 로직이네요 ㅎㅎ.. 배워갑니다!!

예외도 직접 커스텀하셔서 적절한 네이밍을 붙여주신 덕분에, 로직 파악이 굉장히 수월했습니다!

Copy link
Member

@kokodak kokodak Sep 29, 2023

Choose a reason for hiding this comment

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

추가적으로, queryForObject() 메서드의 반환 값으로 null 이나 Optional 을 제공해주지 않고, 값이 없다면 바로 예외를 던져주는 모습이네요.

전 개인적으로 queryForObject() 의 결과 값이 없을 때의 후속 상황을 개발자에게 알려줄 때, UncheckedException 보다는 Optional 반환을 통해 조금 더 명시적으로 알려줄 수 있다고 생각하는데요!

다만, Optional 반환을 하게 되면 이에 대한 예외 처리를 개발자 측에서 해주어야한다는 단점이 있을 것 같기도 해요.(장점일지도..?)

이에 대해 글렌은 어떻게 생각하시나요? 바로 예외를 던져주신 이유가 궁금합니다!!

Copy link
Author

Choose a reason for hiding this comment

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

null을 반환하면 제일 깔끔하겠지만, 메소드의 이름이 queryForObject이고, 해당 메소드의 이름에 대한 역할을 명확하게 하기위해 따로 null 값을 전달하지 않았습니다!
JdbcTemplate 클래스는 하위 계층에서 기능을 제공하기 위해 따로 Optional 값을 반환하지 않았습니다! (사용하는 상위 계층에 위임하기 위함)

@@ -0,0 +1,8 @@
package org.springframework.dao;

public class EmptyResultDataAccessException extends DataAccessException {
Copy link
Member

Choose a reason for hiding this comment

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

예외 커스텀까지.. 굿입니다!!

@kokodak kokodak merged commit 2173aba into woowacourse:seokjin8678 Sep 29, 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