-
Notifications
You must be signed in to change notification settings - Fork 300
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단계] 스플릿-박상현 미션 제출합니다. #332
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 스플릿! 미션에서 만나니 반갑네요 영광입니다 ㅎㅎ
코드를 너무 깔끔하게 짜주셔서 리뷰하는데 너무 편안했어요. 중복코드를 없애기 위해 노력한 흔적이 보여서 너무 좋았어요👍
이미 1단계 요구사항은 충분히 만족한 것 같아서 바로 머지하겠습니다! 제가 남긴 코멘트는 다음 PR 에서 답변 주시면 감사하겠습니다 ㅎㅎ
}); | ||
} | ||
|
||
public void update(final String sql) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
가변인자 유무에 따라 update() 메서드가 나뉜 것 같은데요.
setParameters() 를 보면 가변인자 길이만큼 for 문을 돌면서 parameter 설정을 해주기 때문에 파라미터가 없는 sql 도 아래의 update(sql, params) 메서드로 해결할 수 있을 것 같은데 어떻게 생각하시나요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에는 필요없는 메서드를 아예 호출하지 않게 하기위해서 분리했던 것도 있었습니다!!😊
하지만 저도 레오랑 같은 생각을 해서 update(String, Obejct ...) 메서드를 사용하려고 해보았는데 가변인자에 아무것도 적지 않으면 오버로딩된 메서드가 아닌 현재 자신을 재귀로 부르더라구요.. 😭
이것때문에 가변인자에 null 을 넣는 경우에는 가변인자의 길이를 찾는 과정에서 NPE 가 발생합니다!! 😖
두메서드의 이름을 다르게 해볼까도 생각했는데 저는 현재가 더 사용하기 편한 것 같습니다!! 해당 부분의 중복 코드도 한번 제거해보록 시도해보겠습니다!! 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그런 이슈가 있었군요! 그럼 현재 구조가 이해가네요!!
|
||
protected <T> T executionBaseWithReturn(final String sql, final JdbcTemplateExecution<T> execution) { | ||
|
||
try (final Connection connection = dataSource.getConnection(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try with resources 깔끔하네요 ㅎㅎ
if (resultSet.next()) { | ||
return mapper.map(resultSet); | ||
} | ||
return null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
결과값이 없을 때 Null 을 반환하신 이유가 있을까요?? 저희가 사용하던 JDBC 는 단건 조회 시 결과가 없으면 예외를 던져서 질문드립니다!!
물론 기존 JDBC 와 똑같이 만들 필요는 없기 때문에 잘못된 것 같아 질문 드리는건 아닙니다 ㅎㅎ 그냥 스플릿의 생각이 궁금해서 질문 드려요😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 부분에 대해서 Optional 반환, null 반환, 예외 발생에 대해서 고민해봤는데 null 을 택했습니다!!😊
- Optional 반환 : 메서드를 호출하는 측에서 해당 값이 nullable 할 수 있음을 인지하고 처리 로직을 강요한다.
- null 반환 : repository 는 db와의 통신한 결과를 전달할 뿐이다.
- 예외 발생 : 해당 메서드를 통해서 처리하는 측에서 NPE 가 발생하지 않도록 강제할 수 있다.
제가 생각하는 3방식일때의 주된 특징들입니다. 🧐
우선 1번을 택하지 않은 것은 현재 Service 코드가 Optional 을 고려하지 않은 상태로 작성되어서 입니다!! 이번 미션에서 JdbcTemplate 의 변경으로 인해 Service 레이어가 바뀌는 상황은 그렇게 내키지 않아보였습니다...😖
추가로 Repository 에서 null 일 경우 예외를 발생할까도 했지만 해당 부분은 Repository 가 알아야하는 비즈니스 로직은 아니라고 판단했습니다!!
3번을 택하지 않은 이유는 DataSource 에서 connection 을 통해서 처리가 되는 부분에서의 예외라면 DB에서 발생하는 예외여야한다고 생각하는 해당 부분은 DB 에서는 정상 동작이 가능한 상황이라서 JdbcTemplate 에서 예외를 처리하지는 않았습니다!! 😊
위에 이유들을 통해 1,3 번을 택하지 않았습니다!!
2번또한 현재 NPE 가 발생할 위험이 있습니다 하지만 이번 미션은 Library 를 만들어보는 미션이라 생각했고 이부분은 null 에 대한 고려없이 작성된 코드의 잘못이라고 생각합니다!!
만약 Service 레이어의 코드의 변경 가능하다면 저는 Optional 을 택해서 조회 데이터가 없을 때에 대한 처리가 필요하다는 것을 라이브러리를 사용하는 개발자에게 알리는 것이 좋다고 생각합니다!! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
많은 고민을 하셨군요!
현재 저희가 JDBC 를 만들어 보는 것이기 때문에 이를 사용하는 사람이 단건 조회 시 데이터가 없으면 null 이 반환되는 것을 알거고(저희가 JDBC 를 사용하면서 데이터가 없으면 DataAccessException 발생을 알듯이) 이에 맞춰 사용자가 코드를 짜면 될 것 같아요!
Library 를 만들어보는 미션이라 생각하시고 짜셨다고 해주셔서 위와 같은 생각이 들었어요.
의견 공유 감사합니다!! 👍
public <T> List<T> executeQueryForObjects(final String sql, final ResultSetObjectMapper<T> mapper) { | ||
return executionBaseWithReturn(sql, preparedStatement -> { | ||
final ResultSet resultSet = preparedStatement.executeQuery(); | ||
final List<T> objects = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
결과 리스트도 제네릭 사용하는거 좋네요!
import java.sql.PreparedStatement; | ||
import java.sql.SQLException; | ||
|
||
@FunctionalInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
함수형 인터페이스 굿~
this.dataSource = dataSource; | ||
} | ||
|
||
protected void executionBaseWithNonReturn(final String sql, final JdbcTemplateVoidExecution execution) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
템플릿 콜백 패턴인가요 멋있네요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
템플릿 콜백 패턴이라는 것이 있군요!!!😊
좋은 지식알아갑니다!!! 👍
|
||
private static final Logger log = LoggerFactory.getLogger(JdbcTemplate.class); | ||
public <T> T executeQueryForObject(final String sql, final ResultSetObjectMapper<T> mapper) { | ||
return executionBaseWithReturn(sql, preparedStatement -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드명을 보니 단건조회 같아요. 그런데 아래 메서드와 다르게 결과값이 없을 때 null 을 반환해주는 작업이 없는 이유가 있을까요??
그리고, 사용하지 않는 메서드인데 어떤 상황을 대비한 메서드인지 궁금합니다! 파라미터 없이 단건조회 하는 경우가 잘 생각이 안나네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이것은 완전 실수입니다!!ㅎㅎ..
지금 해당 메서드를 사용하면 예외가 발생할꺼에요 resultSet에 next 를 한번도 호출하지 않은 경우 현재 columnNumber 가 -1 여서 예외가 발생하더라구요....
바로 고치겠습니다!!😊
final Object... params) { | ||
return executionBaseWithReturn(sql, preparedStatement -> { | ||
setParameters(params, preparedStatement); | ||
final ResultSet resultSet = preparedStatement.executeQuery(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ResultSet 관련 리뷰가 누락됐네요.. 저장을 안했나봐요.. 죄송합니다,,
스플릿이 ResultSet 만 try-with-resources 를 활용해 리소스 반납을 하지 않은 이유가 궁금합니다!!
현재 ResultSet 을 close 해주는 부분이 없는데요. 저도 ResultSet 구현 클래스를 들어가서 주석을 읽어보니 ResultSet 을 생성한 statement 가 닫힐 때 자동으로 닫힌다고 나와있더라구요.
혹시 이 부분을 고려해서 별도의 close 로직이 없는걸까요??
구글링을 해보면 명시적으로 close 를 호출해야 한다는 내용도 있더라구요! 아래는 oracle 문서 링크입니다. 그래서 이 부분에 대한 스플릿의 의견이 궁금하네요 ㅎㅎ
oracle 관련 내용입니다! 문서
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이전에 preparedStatement 가 close 된 상태에서 resultset 을 사용하다 닫힌 것 때문에 문제가 있던 적이 있어서 해당 부분을 깊게 보지 않고 되고 있구나 정도로 넘어간 것 같네요..ㅎㅎ 좋은 리뷰 감사합니다!!👍
저희가 사용하는 JdbcPreparesStatement 의 close() 메서드 입니다.
내부적으로 부모의 close() 를 사용합니다.
부모인 JdbcStatment 의 close() 입니다.
JdbcResultSet resultSet 의 closeInternal 메서드입니다.
결과적으로 resultSet 의 close 의 호출에는 문제가 없는 것으로 파악되요!!
만약 미션에서 JdbcTemplate 만이 아닌 사용하는 PreparedStatement, ResultSet 모두 구현하고 내부적으로 PreparedStatement 의 close가 호출될때 ResultSet 의 close 가 호출되도록 구현하지 않는다면 외부에서 반드시 선언해주는 것이 맞다고 생각합니다!!
현재 저희는 내부적으로 Jdbc가 구현한 PreparedStatement, ResultSet 을 사용하기에 해당 부분이 잘 처리되고 있다고 생각해서 따로 작성은 안하는 방향으로 진행할 것 같아요😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
깊게 알아보셨군요! 이를 고려하고 짜신건지 굼금해서 여쭤봤어요 ㅎㅎ
고생하셨습니다!!
안녕하세요 레오🦁
이번에 리뷰받게 된 스플릿입니다!!
이번에는 미션을 진행하면서 최대한 중복코드를 없애보고자 여러가지 시도를 많이해봤습니다!!😁
리뷰하시면서 궁금한점이나 이해가 안되는 부분있으시면 언제든지 DM 으로 물어보시면 답변드리겠습니다!!🫡
좋은 코멘트로 재밌는 개발 대화 나눠보면 좋을 것 같아요!! 감사합니다!!🙇