[JDBC 라이브러리 구현하기 - 1단계] 에버(손채영) 미션 제출합니다. #645
Conversation
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import javax.sql.DataSource; | ||
|
|
||
| public class JdbcTemplate { |
There was a problem hiding this comment.
아직 리팩토링이 되지 않았더라도 테스트를 작성해보면 어떨까요?
저는 주로 테스트를 작성하면서 객체의 역할이 명확한지 한번 더 확인하게 되는 것 같아요.
이번 단계에 테스트를 작성해둔다면 다음 단계에서 리팩토링할 때, JdbcTemplate가 어떤 역할을 가진 객체이고 역할이 너무 많은 건 아닌가 파악하기 수월할 것 같습니다 ㅎㅎ
There was a problem hiding this comment.
동의합니다아 급하게 구현하여 제출하다보니 마음이 급해져 가장 중요한 테스트 작성을 잊었어요
| } catch (Exception e) { | ||
| log.error(e.getMessage(), e); | ||
| throw new RuntimeException(e); | ||
| } finally { |
There was a problem hiding this comment.
최상위 Exception을 받아 RuntimeException으로 예외를 전환하고 있네요. 어떤 상황에 Exception이 터질까요?
There was a problem hiding this comment.
정확하게는 SQLException이 터짐을 확인하였고, 아래의 경우에 예외가 발생합니다!
- 데이터베이스 접근이 불가능할 경우
- 해제된 connection에 대해 preparedStatement를 불러오려는 경우
- 해제된 preparedStatement에 대해 setObject를 수행하는 경우
- setObject를 수행할 때 파라미터 번호가 SQL문과 대응되지 않는 경우
- setObject를 수행할 때 주입할 인스턴스의 타입이 명확하지 않은 경우
- executeUpdate를 수행하였는데 ResultSet이 반환되는 경우 (즉, executeUpdate를 통해 조회 쿼리를 수행하는 경우)
Exception으로 잡던 예외의 타입은 SQLException으로 변경해주었습니다 :)
There was a problem hiding this comment.
예외 발생 관련하여 JdbcTemplateTest에 테스트를 추가해두었습니다. 하지만 모킹의 범위가 너무 무거워, 무의미한 테스트가 아닐까? 하는 생각이 많이 들어요 🥲 이에 대해 몰리는 어떻게 생각하시는지 궁금해요!
| } | ||
| } | ||
|
|
||
| private <T> List<T> createDatas(Class<T> dataType, ResultSet rs) throws Exception { |
There was a problem hiding this comment.
Data가 복수 명사라 인텔리제이가 알림을 주고 있어요.... 😅
Data라는 이름이 너무 포괄적인 것 같기도 해요. 더 적절한 이름을 고민해볼까요? 🤔
There was a problem hiding this comment.
시적 허용과 같이 코드적 허용으로 적용했던 메서드명이었는데요 ㅎㅎ 충분히 불편한 부분일 수 있다고 생각해요!
createData 대신 createInstance로 변경해보았습니다 :)
=> 하지만 reflection 로직을 제거하는 과정에서 해당 메서드는 사라졌습니다!
| Constructor<T> constructor = dataType.getDeclaredConstructor(); | ||
| constructor.setAccessible(true); | ||
| T instance = constructor.newInstance(); | ||
| constructor.setAccessible(false); | ||
|
|
||
| Field[] fields = dataType.getDeclaredFields(); | ||
| for (Field field : fields) { | ||
| field.setAccessible(true); | ||
| Object value = rs.getObject(field.getName(), field.getType()); | ||
| field.set(instance, value); | ||
| field.setAccessible(false); | ||
| } | ||
| return instance; |
There was a problem hiding this comment.
리플랙션으로 객체를 생성하고 있군요!
개발자가 예상하지 못할 상황이 발생할 수도 있을 것 같아요.
개발자가 제공한 생성자를 통해 객체를 생성할 수 있도록 해보면 어떨까요?
There was a problem hiding this comment.
안그래도 reflection으로 인스턴스를 생성한 부분에 대해 다른 크루들에게도 조금 혼(?)이 났는데요. 중복되는 데이터베이스 연결 로직 외 인스턴스 생성 로직은 라이브러리가 아닌 해당 라이브러리를 사용하는 클라이언트가 구현하도록 하는 것이 맞다는 의견에 납득되어 구조를 변경했습니다! RowMapper 함수형 인터페이스 구현체를 전달함으로써 객체를 생성하도록 하였습니다.
| for (Field field : fields) { | ||
| field.setAccessible(true); | ||
| Object value = rs.getObject(field.getName(), field.getType()); | ||
| field.set(instance, value); | ||
| field.setAccessible(false); | ||
| } |
|
몰리! 리뷰 반영 후 다시 요청 드립니다. 여러 고민 끝에 결국에는 JdbcTemplate과 비슷한 구조를 따르게 되었어요. 리플렉션 코드도 제거했구요! 하지만 JdbcTemplateTest에 대한 명쾌한 코드는 찾기 어려웠어요. 혹시 모킹 외에 더 좋은 방안이 있다면 언급 부탁드려요 🤔 |
There was a problem hiding this comment.
안녕하세요 에버 😁
리뷰가 잘 반영된 것 같아 이번 PR은 여기서 approve 할게요.
하지만 JdbcTemplateTest에 대한 명쾌한 코드는 찾기 어려웠어요. 혹시 모킹 외에 더 좋은 방안이 있다면 언급 부탁드려요 🤔
질문 주신 테스트 시 DataSource mocking에 대해서 고민을 해봤는데, 다른 방식으로는 실제 DB를 사용해볼 수도 있을 것 같아요.
실제 데이터베이스인 h2를 사용해서 현재 app 모듈 하위의 DataSourceConfig와 비슷한 설정 파일을 구성하고 테스트 시에 주입하여 테스트 DB면 mocking을 사용하지 않고 테스트를 수월하게 진행할 수 있을 것 같아요.
그런데 jdbc 모듈에서는 h2나 다른 데이터소스 의존성이 없기 때문에 의존성을 추가하지 않는 한 현재 방식처럼 mocking을 하는 게 최선이지 않을까 싶기도 합니다 🤔
둘 중에 어떤 방식을 선택할지는 고민해보시고 적절하게 적용해보면 좋을 것 같아요 ㅎㅎ
그럼 다음 단계에서 뵙겠습니다! 😁
몰리 안녕하세요 ~~~~
저의 리뷰어로 오셨군요! 반갑습니다 😊
1단계 코드 후다닥 구현해서 제출해요.
JdbcTemplate의 구현 방식을 최대한 참고하지 않고 진행해, 통상적인 코드는 아닐 수 있어요. 그러니 몰리의 생각 마음껏 남겨주세요!
그리고 저는 미션에 시간을 들일 생각이 있기 때문에 가감없이 리뷰 남겨주셔도 좋아요! :)
마지막 미션 잘 부탁드립니다 😁
+) 리팩터링 관련은 2단계 테스크로 남겨두었어요. 참고 부탁드려요!