[JDBC 라이브러리 구현하기 - 1단계] 주노(최준호) 미션 제출합니다. #297
Conversation
There was a problem hiding this comment.
안녕하세요 주노~ 땡칠입니다! 추석은 잘 보내셨나요?
저는 너무 많이 먹어서 배가 터질 것 같습니다 ㅎㅎ
1단계 미션 잘 구현해주셨어요! 고생 많으셨습니다! 범위 지정 해주신 덕분에 편하게 리뷰할 수 있었어요 👍
미션하시면서 JdbcTemplate API 사용자에 대한 고민도 해보신 것 같아요.
주노 질문에 대한 제 생각과 리뷰 중에 궁금했던 부분은 커멘트로 남겨봤습니다.
더 자세한 이야기는 커멘트로 이어가보기로 하고, 다음 단계 진행을 위해 Merge 하겠습니다!
(알림 켜두었으니 편하게 커멘트 달아주세요!)
반영이 필요하다고 생각되는 커멘트는 2단계에서 반영해주시면 감사하겠습니다.
| for (int i = 0; i < args.length; i++) { | ||
| preparedStatement.setObject(i + 1, args[i]); | ||
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
SQL에서 1 based index 방식을 사용하기 때문으로 이해하고있습니다~!
다음 stack overflow에서 프로그래밍 언어 뿐만 아니라 산술적으로 표현하는 과정을 고려하여 1 부터 인덱싱을 주었다는 이야기가 있기도 하네요!
There was a problem hiding this comment.
DB를 다루는 코드를 작성할 때 헷갈리지 않게 하려는 의도가 담겨있었군요!
답변 감사합니다 주노~
| while (resultSet.next()) { | ||
| if (result != null) { | ||
| throw new IllegalArgumentException("1개 이상의 결과가 존재합니다."); | ||
| } | ||
| result = rowMapper.mapRow(resultSet); | ||
| } |
There was a problem hiding this comment.
저도 주노 의견에 동의해요. 이렇게 미리 예외를 던지는 방식이 좋다고 생각합니다.
예를 들어 결과가 100개 나왔는데, queryForObject 함수가 임의로 n번째 결과를 주면 어떻게 될까요?
API 사용자는 오류가 난지도 모르고 '잘 가져왔겠거니..' 생각해서 다음 로직을 수행하겠죠?
이 버그를 발견하지 못하고 서비스를 운영한다면... 정말 돌이킬 수 없는 상황이 생길수도 있겠네요 😵
이런 측면에서 아주 적절한 예외 처리를 해주셨다고 생각해요.
| ResultSet resultSet = preparedStatement.executeQuery(); | ||
| T result = null; | ||
|
|
||
| while (resultSet.next()) { | ||
| if (result != null) { | ||
| throw new IllegalArgumentException("1개 이상의 결과가 존재합니다."); | ||
| } | ||
| result = rowMapper.mapRow(resultSet); | ||
| } | ||
|
|
||
| if (result == null) { | ||
| throw new IllegalArgumentException("결과가 존재하지 않습니다."); | ||
| } | ||
| return result; | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
(개인적인 의견이니 꼭 반영하지 않으셔도 됩니다!)
while() 루프를 사용하지 않고 다음과 같이 표현하면 주노가 예외처리한 의도가 더 명확해지고, 가독성도 개선될 것 같아요!
어떻게 생각하시나요?
| ResultSet resultSet = preparedStatement.executeQuery(); | |
| T result = null; | |
| while (resultSet.next()) { | |
| if (result != null) { | |
| throw new IllegalArgumentException("1개 이상의 결과가 존재합니다."); | |
| } | |
| result = rowMapper.mapRow(resultSet); | |
| } | |
| if (result == null) { | |
| throw new IllegalArgumentException("결과가 존재하지 않습니다."); | |
| } | |
| return result; | |
| } catch (Exception e) { | |
| ResultSet resultSet = preparedStatement.executeQuery(); | |
| if (!resultSet.next()) { | |
| throw new IllegalArgumentException("결과가 존재하지 않습니다."); | |
| } | |
| if (resultSet.isFirst() && !resultSet.isLast()) { | |
| throw new IllegalArgumentException("1개 이상의 결과가 존재합니다."); | |
| } | |
| return rowMapper.mapRow(resultSet); | |
| } catch (Exception e) { |
There was a problem hiding this comment.
isFirst()와 isLast()를 이용하니까 훨씬 깔끔해지는군요!!
깔끔한 개선방안 제시 감사합니다! 🙇♂️
| public List<User> findAll() { | ||
| // todo | ||
| return null; | ||
| final var sql = "select id, account, password, email from users"; | ||
| return jdbcTemplate.query(sql, USER_ROW_MAPPER); | ||
| } |
There was a problem hiding this comment.
반복되는 코드가 JdbcTemplate에 위임되었네요!
Checked Exception 들에 대한 처리도 함께 위임된 것으로 보이는데,
Checked Exception 던지기 vs Unchecked Exception 던지기. 어떤 장단점이 있을까요?
어떤 기준으로 예외 처리를 결정하셨는지 주노 생각이 궁금합니다!
There was a problem hiding this comment.
Checked Exception을 사용하면 예외처리를 강제해야하므로 프로그램의 안정성이 높아지지만 반대로 레어어간의 의존성이 높아진다는 단점이 존재합니다.
반면에 Unchecked Exception의 경우 레이어간의 의존성을 낮출 수 있다는 장점이 있고, 개발자가 예외 핸들링을 누락할 수 있다는 단점이 존재합니다.
Checked Exception을 사용한다면 예외처리를 강제함으로써 코드의 복잡성이 증가한다는 부분 때문에 Checked Exception을 사용하는 것이 와닿지 않았습니다.
Unchecked Exception으로도 충분히 예외를 다룰 수 있으며 개발자의 예외 핸들링 누락은 시스템적으로 큰 영향을 미친다고 생각하지 않아 Unchecked Exception을 던지기를 선택했습니다~
There was a problem hiding this comment.
굳이 하위 레이어의 예외에 대한 의존성을 가질 필요가 없다는 점.
개발자가 예외 처리를 누락해도 잘못된 처리 방지에는 전혀 문제가 없다는 점.
때문에 이런 선택을 하셨군요!
주노 생각 잘 들었습니다.
Checked Exception은 한번 쓰기 시작하면 여기저기 결합되죠. 개인적으로 주노가 중간에서 잘 끊어주셨다고 생각합니다.
| @@ -14,4 +20,64 @@ public class JdbcTemplate { | |||
| public JdbcTemplate(final DataSource dataSource) { | |||
| this.dataSource = dataSource; | |||
| } | |||
|
|
|||
| public void update(String sql, Object... args) { | |||
| try (Connection connection = dataSource.getConnection(); | |||
There was a problem hiding this comment.
미처 생각하지 못한 부분이 있어서 하나 더 남깁니다..!
주노 코드에서 가져온 커넥션은 AutoCloseable 때문에 현재 try-catch 문 수행 후에 닫힐 거에요.
그런데 dataSource.getConnection()으로 가져온 커넥션을 임의로 닫아버려도 괜찮을까요?
가져온 커넥션이 커넥션 풀로 관리되고 있었다면 어떻게 될까요?
There was a problem hiding this comment.
제가 질문의 의도를 정확히 이해하지 못해 일단 이해하고있는 부분에 대해 먼저 정리드리고자 합니다
저는 위 과정이 Connection Pool에서 하나의 Connection을 가져와서 사용 후 반환하는 시나리오로 이해했는데요!
땡칠이 말씀하신 커넥션을 임의로 닫아버린다는 말이 Connection Pool에 사용을 완료한 커넥션을 반환한다 라는 말로 이해하면 될까요?
만약 try-with-resource로 관리하지 않으면 하나의 커넥션을 계속 물고있게 되어 문제가 발생하기 때문에 Connection Pool의 자원 고갈문제를 해결하기 위해 커넥션을 해제한다고 이해하고있습니다.
정확히 어떤 부분을 우려하여 위와같은 질문을 주신것인지 잘 이해를 못했습니다 😅
There was a problem hiding this comment.
커넥션 풀이 관리하는 커넥션을 임의로 닫아버리면, 커넥션을 닫지 않고 재활용하는 커넥션 풀의 존재 의의가 사라지는 것을 우려하여 남긴 커멘트였습니다.
try-with-resources 사용으로 인해Connection.close()이 호출되면 사용한 커넥션을 '반환' 하는 것이 아니라 정말로 '연결 종료' 시켜버리는 것은 아닌지 고민이 들어요.
물론 똑똑한 분들이 그런 일이 생기지 않게 잘 구현했겠지만 어떤 과정으로 동작하는지 궁금해지네요!
이 주제 관련해서 알고있는게 있나요?
안녕하세요! 땡칠 0️⃣7️⃣
1단계 JDBC 라이브러리 미션 잘부탁드립니다.
리뷰에 대한 코드 범위를 지정했습니다! 해당 범위에 대해 리뷰 부탁드립니다 🙏
코드 리뷰 (범위지정)
질문
queryForObject를 구현하는 과정에서 결과가 1개 이상이면 예외가 발생해야하지 않을까? 라는 생각에 실행된 쿼리의 결과가 1개 이상이면 안된다는 생각에 예외가 발생하도록 코드를 구성했습니다.
이에대한 땡칠의 생각이 궁금합니다!
잘부탁드립니다 🙇