[JDBC 라이브러리 구현하기 - 2단계] 망고(고재철) 미션 제출합니다. #413
Conversation
There was a problem hiding this comment.
안녕하세요 망고! 명절 잘 지내셨나요 👍👍
2단계 미션도 깔끔하게 해주셨네요! 😋
중복되는 부분도 메서드로 잘 분리해주셔서 머지하려 했는데 혹시 추가적으로 하실게 있는지 여쭤보고 진행하려고 rc 했습니다!
궁금한 점도 있어서 커멘트 남겨봤습니다!
- queryForObject에서 단 하나의 데이터가 아닌 두 개 이상이 나올 때를 어떻게 생각하시나요?
- try-catch 중복을 제거할 수 있었지만 상황에 대해 예외가 다를 수 있는게 이런 부분도 공통적으로 처리하시는거를 선호하시나요?
- 클래스를 구현해서 역할을 넘기지 않고 메서드 분리만 하신 이유가 있으시다면 궁금합니다!
| public <T> Optional<T> queryForObject(final String sql, final RowMapper<T> rowMapper, final Object... objects) { | ||
| final var results = execute(sql, preparedStatement -> mapResults(rowMapper, preparedStatement), objects); | ||
| return results.stream() | ||
| .findAny(); |
There was a problem hiding this comment.
두 개 이상 조회되어도 상관없는 것을 의도하신건지 궁금하네요!
저는 두 개 이상 조회되는 부분을 예외로 처리했습니다.
queryForObject가 단 하나 조회될 것을 예상하고 가져오는데 findAny()로 두 개 이상 있어도 찾아올 수 있는거 같아요!
개인적으로는 두 개 이상 조회 됐을 때 조회된 순서에 따라 같은 조건이라도 다른 값이 나올 수 있는게 위험할 수도 있다고 느꼈어요!
There was a problem hiding this comment.
두 개 이상 조회 되어도 이게 queryForObject 메서드에서는 의도된 상황이라고 생각하고 예외 처리를 할 필요가 없겠다 생각했는데,
애초에 이런 상황 자체가 잘못 설계된 상황이라 예외 처리해서 이를 알려주는 게 맞겠네요!
ex) account가 여러 개 Insert 되어있는데 findByAccount를 한다면..? (의도된 상황이 아닐 거니까 예외 처리하는 게 맞겠네요)
예외 처리하도록 수정했습니다 😎
| private <T> T execute(final String sql, final PreparedStatementCallback<T> preparedStatementCallback, | ||
| final Object... objects) { | ||
| try (final var connection = dataSource.getConnection(); | ||
| final var preparedStatement = prepareStatement(connection.prepareStatement(sql), objects)) { | ||
| log.debug("query : {}", sql); | ||
| return preparedStatementCallback.callback(preparedStatement); | ||
| } catch (SQLException e) { | ||
| throw new DataAccessException(e); | ||
| } | ||
| } |
There was a problem hiding this comment.
굿굿 중복이 굉장히 많이 줄었네요! 👍👍
저는 따로 분리해서 클래스를 만들어서 사용했는데 망고가 하신 방식이 오히려 더 알아보기 쉬운거 같다고 느꼈어요.
따로 클래스를 만들어서 이 역할을 다른 곳에 넘겨주지 않고 메서드로만 분리하신 이유가 궁금하네요!
There was a problem hiding this comment.
오.. 사실 저는 클래스로 분리할 생각 자체를 못했어요 🤣
당연히 해당 행위 자체가 JdbcTemplate의 책임이라고 생각해서 그런가봐요.
근데 지금 다시 보니 execute랑 prepareStatement 메서드를 클래스로 분리해서 책임을 나누는 것도 괜찮아 보여요!
책임을 어디까지로 보는지 관점에 따라 다른 것 같네요 👍
| } catch (SQLException e) { | ||
| throw new DataAccessException(e); | ||
| } |
There was a problem hiding this comment.
앗 이 부분은 Exceptions should be either logged or rethrown but not both라는 SonarLint의 말을 참고해서 예외를 던지는 부분만 남기도록 했습니다..!
근데 여기서는 예외가 발생했을 때 어떤 쿼리가 나가는지 & 어떤 인자를 전달받았는지는 로그로 찍어주면 좋을 것 같네요.
해당 부분 추가했습니다~~
| private <T> List<T> mapResults(final RowMapper<T> rowMapper, final PreparedStatement preparedStatement) | ||
| throws SQLException { | ||
| final var resultSet = preparedStatement.executeQuery(); | ||
| final var results = new ArrayList<T>(); | ||
| while (resultSet.next()) { | ||
| results.add(rowMapper.mapRow(resultSet)); | ||
| } | ||
| return results; |
There was a problem hiding this comment.
RowMapper로 매핑하는 부분을 이쁘게 빼주셨네요 🥭🥭
만약 예외가 발생하면 메서드에서 예외를 전가할 수 있게 구현하셨군요!
만약 여기서 try-catch 해서 log를 찍으면 어디서 어떻게 예외가 발생했는지 보다 정확하게 알 수 있고 더 자세한 핸들링이 가능할거 같아요.
물론 그 만큼 try-catch 하는 부분이 많아서 관리하기도 어려워지는 문제도 있다고 생각해요.
망고는 어떻게 생각하시나요?
There was a problem hiding this comment.
그렇게 하면 확실히 더 자세하게 핸들링 할 수 있을 것 같네요!
또한 말씀해주신 try-catch가 많아져서 생기는 문제점도 공감합니다.
저는 개인적으로 여기서는 exception을 전달해주는 방식의 핸들링만으로도 충분하다고 생각했습니다.
가독성도 중요하니까요 😎
안녕하세요 헤나!
추석동안 열심히 쉬고 오랜만에 다시 돌아왔습니다 😎
2단계는 리팩터링인데 다른 부분들은 이미 1단계에서 진행한 것 같아서 try - catch 중복되는 부분들만 신경써봤습니다.
try - catch 중복을 어떻게 없앨지 찾아보다가 템플릿 콜백 패턴이란 게 있는 걸 보고,
PreparedStatementCallback함수형 인터페이스를 만들어서JdbcTemplate에서 콜백 함수를 불러서 처리하도록 리팩터링 했습니다.이 외에는 크게 변경 사항은 없어요! (null 값 처리 정도..?)
이번 리뷰도 잘 부탁드려요~ 🙇♂️