-
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단계] 포이(김보준) 미션 제출합니다. #269
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.
안녕하세요, 포이! 추천하신 무테 안경 샀습니다 🤓
개인 사정으로 리뷰가 늦어져서 죄송합니다 🥲🥲🥲
크게 바꿔야 할 부분이 보이지 않고, 의견이 궁금한 리뷰가 대부분이라 이번 미션은 approve 하겠습니다!!
2단계 가보시죠 🏃♀️🏃♀️
|
||
private int updateCount(@Nullable final Integer result) throws DataAccessException { | ||
if (result == null) { | ||
throw new DataAccessException("no update count"); |
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.
result == 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.
템플릿 콜백패턴을 위해 사용한 execute 메서드에서 Null 을 반환하는 경우가 있어 @Nullable
어노테이션을 붙였는데, 이 때문에 그냥 사용할 경우 경고가 표시되어 이런 검사로직을 넣게 됐네요~ 그렇지만 updateCount는 executeUpdate시에만 사용되고 이 메서드는 null을 반환하지 않으니 supressedWarning을 붙여주는게 더 적절한 사용방식일 수 있겠군요
|
||
public void execute(final String sql, final Object... params) throws DataAccessException { | ||
log.debug("query : {}", sql); | ||
execute(sql, new PreparedStatementCreator(), ps -> { |
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.
PreparedStatementCreator
를 사용해주셨네요! 생성의 책임을 다른 객체로 넘긴 것 좋은 것 같아요 👍👍
다만 궁금한 점이 있는데, 해당 객체는 매번 생성되어야 하는 이유가 있을까요? 현재 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.
처음에는 preparedStatement 또한 여러 종류가 있고, creator도 이에 따라 다양해 질 수 있을 것 같아 Functional Interface 로 구현하였는데 과한 확장인 것 같아 이를 폐기했습니다.
인터페이스이다 보니까 구현체를 만들어줘야했고, 중복되는 곳에서 사용되는 SimplePreparedStatementCreator를 만들어줬었는데 폐기하면서 이를 꼼꼼하게 수정하지 않았네요! 😅
|
||
public class ResultSetExtractor<T> { | ||
|
||
private final ResultSet rs; |
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
은 어떨까요 ㅎㅎ 반영 안 하셔도 됩니다!
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.
아직은 rs만으로 명확하다고 생각하여 혼동할만한 다른 변수명이 있을 때 바꿔보겠습니다~
@@ -14,6 +14,7 @@ public class DataSourceConfig { | |||
public DataSource dataSource() { | |||
return new EmbeddedDatabaseBuilder() | |||
.setType(EmbeddedDatabaseType.H2) | |||
.setName("test;DB_CLOSE_DELAY=-1;MODE=MYSQL;") |
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.
제가 추가한 건 아니고 구구가 추가했는데, 인메모리 db를 사용하면 연결된 커넥션이 없을 때 db를 그냥 종료시켜버린다고 합니다! jvm이 종료되기 전에 db가 종료되는 불상사를 방지하기 위해 넣는 옵션 같네요~
return results.get(0); | ||
} | ||
|
||
public <T> List<T> query(final String sql, final RowMapper<T> rowMapper, final Object... params) throws DataAccessException { |
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.
현재 상황에서는 query 메서드의 기본 리턴 값을 List로 두는 것과, queryForList 라는 메서드를 추가하는 것 중 어떤게 나은 것 같으신가요? 이 부분은 정답이 없는 것 같아서 포이의 의견이 궁금하네요 😆
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.
음 이 부분에서는 사실 기존의 JDBCTemplate를 의식하지 않는다면 어떤 이름도 상관 없을 것이라는 생각이 들어요! 오히려 List가 더 바람직한 이름이라고 생각하실 분들도 계실 것 같습니다!
그렇지만 기존의 JDBCTemplate를 고려한다면, 두 메서드는 약간의 차이를 갖는데요,
JDBCTemplate의 queryForList 메서드를 살펴보면
@Override
public <T> List<T> queryForList(String sql, Class<T> elementType) throws DataAccessException {
return query(sql, getSingleColumnRowMapper(elementType));
}
@Override
public List<Map<String, Object>> queryForList(String sql) throws DataAccessException {
return query(sql, getColumnMapRowMapper());
}
위처럼 SingleColumnRowMapper 나 ColumnMapRowMapper를 사용하고 있는 것을 볼 수 있어요.
즉, RowMapper를 사용자 정의를 통해 지정해주지 못하고, 특정 컬럼에 대한 반환타입만 지정하거나, 전부 컬럼이름(key, String) : 컬럼 값(value, Obejct) 방식으로 전달해줄 수 밖에 없습니다.
제가 이번 미션에서 만들고자 하는 메서드는 RowMapper를 통해 사용자가 지정한 타입의 객체로 각각의 Row를 매핑하여 반환하는 것이기 때문에 queryForList보다는 query가 적합하다는 생각이 드네요! ♞
질문해주신 부분을 못 봤네요... 여기에 남기겠습니다! |
베로 안녕하세요!
안경은 뿔테보다는 무테가 멋집니다.
이번 미션에서는
DataAccessException
이라는 공통 예외로 던져준다.이 두 가지에 집중해보았습니다.
이 과정에서 JDBC Template을 사용할 때 필요한 여러 인터페이스, 클래스들을 만들게 되었는데 이 중 RowMapper에 대한 사소한 의문이 생겼습니다.
RowMapper의 메서드 mapRow 에서 두번째 파라미터 rownum은 언제 사용하는게 적절할까요? (저는 아직은 필요를 못느껴 삭제했습니다.)
민족의 대명절 추석이 얼마 안남았는데 건강 잘챙기시고 가족분들한테도 안부 전해주세요 감사합니다.