JDBC 라이브러리 구현하기 - 2단계] 채채(신채원) 미션 제출합니다. #467
Conversation
There was a problem hiding this comment.
채채 안녕하세요!
이번 단계 요구사항에 있는 대로 라이브러리와 어플리케이션의 관심사를 잘 분리해주신 것 같습니다!👍👍
말씀해주신 라이브러리가 빈 값을 반환하고 서비스에서 이를 처리하는 부분에 대해서는 라이브러리의 책임을 어디까지로 보느냐에 따라 달라질 수 있을 것 같아요.
기존의 JDBC Template을 보면 사이즈가 1이 아닌 경우 ( 0 or 2 ~ INF) 무조건 예외를 던져주긴 하지만 반대로 null을 반환해준다고 해도 저는 그렇게 어색함을 느끼지는 않았을 것 같습니다.
또 채채의 코드에서는 Optional을 통해 nullable한 응답임을 명시해주고 있기도 하니까요.
해당 메서드의 역할이 정확히 한 개의 행만 반환한다. 라고 생각하신다면 exception을 던져주는 것이 자연스럽다고 생각하고, 한 개 이하의 결과를 반환한다. 라고 생각하신다면 현재 코드를 유지하셔도 좋아보입니다.
질문 주신 부분 말고도 채채의 의견을 들어보고 싶은 곳이 있어 request change할까 했으나 그렇게하면 월요일까지 4단계를 진행하기에는 일정이 조금 촉박할 수도 있겠네요..
이 부분에 대해서 현재 pr에서 의논하기 보단 다음 단계를 진행하며 고민해보시고 pr에 함께 의견 작성해주시면 감사할 것 같습니다!
이번 단계는 이만 approve 하겠습니다. 다음 단계 달려보시죠 🫡
| @@ -42,12 +43,12 @@ public List<User> findAll() { | |||
| return jdbcTemplate.queryForList(sql, USER_MAPPER); | |||
| } | |||
|
|
|||
| public User findById(final Long id) { | |||
| public Optional<User> findById(final Long id) { | |||
| public <T> Optional<T> queryForObject(final String sql, final ResultSetMapper<T> rowMapper, final Object... parameters) { | ||
| final List<T> results = query(sql, statement -> statementExecutor.execute(statement, rowMapper), parameters); | ||
| if (results.size() > 1) { | ||
| throw new DataAccessException("2개 이상의 결과를 반환할 수 없습니다."); |
There was a problem hiding this comment.
유저에게 상세한 unchckedException으로 변환해 던져 주셨네요 👍
그런데 JDBC Template 같은 라이브러리의 코드를 보면 메서드들이 UnchckedException 인데도 불구하고 이러런식으로 메서드 시그니처에 throws 가 붙어있는 것을 확인할 수 있습니다.
저도 왜 굳이 던져주는지 궁금해서 찾아봤는데 스택 오버플로우 에서 이런 의견이 있더라고요.
The (ostensible) reason for declaring an unchecked exception in a throws clause is to document the fact that the exception may be thrown by the method. This will translate into text in the method's javadocs that the programmer can (should) read to understand the method and how to use it.
대충 명시적으로 사용자가 어떤 예외가 던져지는지 알게 하고, 처리를 어떻게 할 것인지는 자유에 맡긴다는 뜻 같아요!
채채는 혹시 이와 같이 명시적으로 unchckedException을 던져주는 것에 대해 어떻게 생각하시나요?
| private PreparedStatement getPreparedStatement(final String sql, | ||
| final Connection connection, | ||
| final Object[] parameters) throws SQLException { | ||
| private PreparedStatement statementCreate(final Connection connection, |
There was a problem hiding this comment.
저는 createStatement 처럼 동사가 앞에 오는게 조금 더 자연스러울 것 같네요
| if (results.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| return Optional.ofNullable(results.iterator().next()); |
There was a problem hiding this comment.
위에서 Empty를 검증하는데도Optional.of가 아닌 Optional.ofNullable 로 반환해주셨네요
혹시 어떤 이유일까요!?
There was a problem hiding this comment.
Optional은 항상 ofNullable를 사용하였어서 습관처럼 사용하였네요 말씀처럼 위에서 체크를 해주었기 때문에 of로 하는 것이 더 좋을 것 같습니다!
| } | ||
|
|
||
| public <T> Optional<T> queryForObject(final String sql, final ResultSetMapper<T> rowMapper, final Object... parameters) { | ||
| final List<T> results = query(sql, statement -> statementExecutor.execute(statement, rowMapper), parameters); |
There was a problem hiding this comment.
채채가 작성해주신 queryForObject 에는 결과의 수를 검증하는 부분이 있으니, 이 메서드에서 queryForList를 재사용해서 중복을 줄여도 괜찮을 것 같다는 생각이 드네요.
| import java.sql.SQLException; | ||
|
|
||
| @FunctionalInterface | ||
| public interface PreparedStatementCallback<T> { |
포이 안녕하세요~! 제출이좀 늦어졌네요..ㅎㅎ
중복 제거를 중점으로 리팩토링 해보았는데요..!
아직 고민이 되는 것은 queryForObject 안에서 인자가 존재하지 않을때 예외가 나지 않도록 if문으로 빈값을 반환하도록 처리를 하였는데 여기서 빈값으로 처리를 한 후 UserService 여기서 예외를 잡습니다..! 이 로직이 전 좀 어색해 보이는데 포이 생각은 어떠신지 궁금합니다..!
이번 리뷰도 잘부탁드립니다!