-
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단계] 모디(전제희) 미션 제출합니다. #322
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.
안녕하세요 모디! 콩하나입니다!!
레벨 2 때 함께 장바구니를 만들다가 이번에는 리뷰를 하게 됐네요 ㅎㅎ
역시 모디... 잘하시는군요.
1단계에서 요구하는 내용을 아주 충분히 잘 만족하셨다고 느껴집니다~~
우선 모디에게 묻고 싶은 부분이 한 가지 있습니다!
모디는 이번 미션을 통해서 어떤 부분을 배우고 싶으신가요?
저는 레벨 4 시작할 때 구구가 왜 라이브러리가 이러한 형태로 될 수 밖에 없는지 고민해보면서 미션을 진행해보라
는 제안이 크게 와닿아서 여태 그렇게 구현을 하고, 이를 몸소 느끼는 것을 목표로 하고 있어요!
기존 라이브러리를 최대한 참고하지 않고 뇌피셜로 이것저것해보다보니까 코드 퀄리티는 조금 안좋게도 느껴지지만...(시간도 오래 걸리고요 ㅠㅠ.)
그래도 많이 배우게 되는 것 같아요!
결국 나중에는 왜 그렇게 프레임워크가 만들어질 수 밖에 없는지 느껴지기도 하고요!!
모디는 어떤 목표를 가지고 미션에 임하시는지 궁금하네요! ㅎㅎ
코멘트를 추가로 달아주시면 저도 그 부분에 집중할 수 있도록 최대한 코드 리뷰를 해보도록 하겠습니다~!!
너무 미션을 잘 수행해주셔서 1단계는 이만 머지하도록 하겠습니다~!!
고생하셨고 추석 잘 보내세요 👍
public class UserDao { | ||
|
||
private static final Logger log = LoggerFactory.getLogger(UserDao.class); | ||
|
||
private final DataSource dataSource; | ||
private final JdbcTemplate jdbcTemplate; | ||
|
||
public UserDao(final DataSource dataSource) { | ||
this.dataSource = dataSource; | ||
this.jdbcTemplate = new JdbcTemplate(dataSource); |
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.
전반적으로 dao 코드가 많이 간단해졌군요!
이번 미션에서 요구한 사항들을 잘 만족하셨네요 👍
@AfterEach | ||
void tearDown() { | ||
jdbcTemplate.update("delete from users"); | ||
jdbcTemplate.update("alter table users alter column id restart with 1"); | ||
} | ||
|
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.
제가 queryForObject
에서 아래처럼 resultSet
의 커서를 맨 처음으로 옮기고 그게 마지막인지 확인하는데요,
기존 테스트코드에서는 @Transactional
같은게 없다보니 롤백이 되지 않아 테스트를 여러 개 돌리면 이전 테스트에서 저장된 데이터가 쌓여서 실패하는 경우가 발생하더라구요.
if (rs.first() && rs.isLast()) {
...
}
임시로 하나인지 검사를 제외해도 되긴 한데 1단계에서 요구하는 jdbcTemplate 기능에는 이것이 더 맞는 것 같아 직접 삭제 로직을 추가해주었습니다!
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 pstmt = conn.prepareStatement(sql) | ||
) { | ||
for (int i = 1; i <= args.length; i++) { | ||
pstmt.setObject(i, args[i - 1]); |
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.
오... setObject라는 게 있었군요...!
저는 하나하나 매핑객체를 만들어줬었는데 이걸 쓰면 훨씬 더 간편해지겠어요!
public static Class<?> convertToClass(int types) { | ||
switch (types) { | ||
case -5: | ||
return long.class; | ||
case 1: | ||
return String.class; | ||
case 12: | ||
return String.class; | ||
default: | ||
throw new IllegalStateException(); | ||
} | ||
} | ||
} |
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.
enum 클래스를 사용해서 각 객체에 type 숫자를 매핑해줘서 클래스를 찾는 방법도 있을 것 같은데요!
switch를 사용하신 이유가 있으실까요!?
그리고 이 정적 클래스 또한 JdbcTemplate에서 사용되고 있으니 커스텀 예외를 사용해보시면 좋을 것 같네요 👍
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.
enum을 통해서 구현하고 values()로 상수의 필드를 순회하며 찾는 방법도 있을 듯 한데요
단순히 자바에서 바이트코드 레벨의 switch문에 대한 추가적인 최적화를 진행해주어서 enum을 쓰더라도 switch 사용을 고려해봤을 것 같아요!
옛날에 꾸글쓰에 올렸던 글... 재탕해봅니다 https://jaehee329.tistory.com/47
예외는 전환해줬어요!
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.
저런게 있군요...
저걸 까볼 생각을 하다니 대단하네요 모디 ㅋㅋㅋㅋ
int columnType = metaData.getColumnType(i); | ||
columnTypes[i - 1] = ColumnTypes.convertToClass(columnType); | ||
} | ||
Constructor<?> constructor = requiredType.getDeclaredConstructor(columnTypes); |
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.
처음에 해당 생성자를 봤을 때 데이터 베이스에 저장한 모든 필드를 전달받아 객체를 생성
할 수 있는 생성자를 만드는 것이 제약사항으로 존재해서 호불호가 있을 수 있겠다는 생각을 했었는데요.
다시 생각해보니까 의미있는 데이터를 데이터베이스에 저장하기 때문에 객체 생성 시에도 당연히 이 데이터들을 사용하겠네요!
데이터베이스의 모든 필드를 불러와서 객체를 생성하는 것이 좋은 방법 같네요 :)
@@ -0,0 +1,5 @@ | |||
package org.springframework.jdbc; | |||
|
|||
public class IncorrectResultSizeDataAccessException extends RuntimeException { |
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에서 해당 예외를 반환하고 있네요.
라이브러리 내에서 여러 종류의 예외가 발생하면 이를 사용하는 측에서 어려움이 있을 것 같다는 생각도 드는데요.
마침 JdbcTemplate에서 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.
오 너무 좋은 지적이네요!👍
말씀처럼 DataAccessException의 상속으로 변경하는게 좋을 것 같아요~~!!
저는 특정 라이브러리에서 발생하는 Exception은 하나로 묶어주되 예외 자체가 세분화되어있는 편이 좋다고 생각해요
예를 들어 reflection 라이브러리를 사용할 때 굉장히 다양한 종류의 exception이 발생하는데 javadoc에 각각의 exception이 언제 발생하는지 자세히 설명되어서 쓰기 편하더라고요.
@FunctionalInterface | ||
public interface RowMapper<T> { | ||
|
||
T mapRow(ResultSet rs, int rowNum) throws SQLException; | ||
} |
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은 rowNum을 사용하고 있지 않아보이는데요!
rowNum을 넣어주신 이유가 있으실까요?
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.
추가적으로 코드를 보다보니 궁금한 부분이 또 생겼어요!
현재 RowMapper를 query
라는 메소드에서만 사용해주고 계신데,
queryForObject
메소드에서도 이를 사용할 수 있을 것으로 보이네요!!
혹시 이를 query
에서만 사용하신 이유가 있으실까요?
아니면 질문을 바꿔서, queryForObject
메소드에서 사용했던 방식처럼 query 메소드도 구현할 수 있을 것으로 보이는데요.
query
메소드에서는 RowMapper 클래스를 만들어서 관리해주시는 이유가 있으실까요??
자유롭게, 편하게 답변 주세요~~
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.
말씀드렸듯 저는 완전히 독창적으로 구현하는 것보다 실제 구현된 라이브러리를 일부 살펴보면서 왜 이런 식으로 구성되었을까 의도를 고민해보는게 이번 레벨 4 미션들에서 얻는 것이 많은 것 같아서 아는 선에서는 라이브러리와 유사한 형태를 가져가보고 싶어요
rowNum은 JdbcTemplate에서 사용하는 함수형 인터페이스를 그대로 가져오고 싶어서 넣어줬어요😂
당장 구현한 내용에선 rowNum이 필요 없기 때문에 제거해도 되는 것은 맞지만요.
주변의 크루들과도 비슷하게 원 라이브러리에 rowNum이 왜 필요할까? 에 대해 이야기해봤을 때 정확한 사용처가 어디인지는 명확히 찾진 못했지만 페이징 관련 쿼리에서 rowNum을 사용할 일이 있을 수도 있겠다... 라는 얘기 정도가 오갔네요.
혹시 콩하나는 원 라이브러리에서 rowNum이 언제 쓰이는지 아시나요? 아신다면 저에게도 알려주세요!
query
와 queryForObject
의 시그니처를 다르게 가져간 이유는... 단순하게 원 JdbcTemplate도 오버로딩을 통해 다양한 시그니처의 query
와 queryForObject
를 제공해주잖아요, 다른 시그니처를 어떤 식으로 구현하고 있을까 고민해보고싶어 지금의 아예 다른 두 형태를 선택하게 되었어요. 똑같이 rowMapper를 쓰는 코드만 구현해보면 재미 없잖아요...😅
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.
혹시 콩하나는 원 라이브러리에서 rowNum이 언제 쓰이는지 아시나요? 아신다면 저에게도 알려주세요!
저도 사실 이게 궁금해서 검색을 해봤었는데요.
지금은 잘 사용하지는 않지만 rowNum
으로 행번호를 전달하여 데이터를 가져오는 작업을 조작
할 일이 있을 수 있어 이렇게 구현했다고 하네요.
마침 스프링 프로젝트 이슈에서도 해당 내용에 대한 언급이 있어서 남기도록 하겠습니다.
spring-projects/spring-framework#7796
다른 시그니처를 어떤 식으로 구현하고 있을까 고민해보고싶어 지금의 아예 다른 두 형태를 선택하게 되었어요.
좋은 동기네요 👍
Connection conn = getConnection(); | ||
PreparedStatement pstmt = conn.prepareStatement(sql) | ||
) { | ||
try (ResultSet resultSet = pstmt.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.
이것도 위쪽 try에서 처리해줄 수 있어보이네요!
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.
Connection 및 PreparedStatement 처리는 모든 메서드에서 공통인데 resultSet처리는 메서드마다 형태가 조금씩 달라서
2단계 이후에 관심사를 분리할 때 다르게 처리해주려고 나눠놓았어요!
이후에 PR에서 코드로 보여드리겠습니다! ㅋㅋㅋㅋ
아! 모디 덕분에 라이브러리도 많이 까보고 좋았습니다 👍 |
늦은 리뷰 반영...및 질문 대답... 죄송합니다! ㅋㅋㅋ😅 말씀처럼 아무것도 없는 상태에서 구현을 하기보단 라이브러리를 참고해보면서 왜 이런식으로 구현되었을까? 를 고민하는게 좀 더 유의미하게 얻어가는 점이 많은 것 같아요. 시간도 ... 조금 절약되기도 하고요 ㅋㅋㅋㅋ😂 그래서 제 구현 코드에서 필요가 없을지라도 목표하는 라이브러리의 구조를 가져와보면서(리뷰 주신 rowNum처럼) 가능하다면 구조를 같이 가져가보려고 하고 있습니다! 이 와중에 저 또한 왜 이렇게 쓰는지 잘 모르겠는데? 하는 부분들은 콩하나와 얘기해볼 수 있도록 할게요! |
안녕하세요 콩하나~ 코드로 다시 만나게 되어 반갑습니다😄
1단계 간단히 구현해서 제출합니다!
기본적인 내용들만 넣어서 돌아가는 코드 위주로 작성했고
2단계 내용이 리팩토링이어서 2단계를 진행하며 메서드 분리 및 트랜잭션 관리 등을 할까 해요!
잘 부탁드립니다~