Skip to content
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 라이브러리 구현하기 - 2단계] 조이(김성연) 미션 제출합니다. #392

Merged
merged 6 commits into from
Oct 5, 2023

Conversation

yeonkkk
Copy link
Member

@yeonkkk yeonkkk commented Oct 3, 2023

안녕하세요 하마드! 조이입니다 ~
연휴는 잘 보내고 계신가요?

지난 리뷰에 대한 답변은 작성해두었으니 편하실 때 확인해주세요 ㅎ

이번 단계는.. try-with-resources 중복 부분이나 null 처리 부분을 리팩터링을 하긴 했는데, 그 이상으로 어떤 부분을 더 변경해봐야할지 생각이 잘 안나네요..
그래서 결론적으로 보면 코드 변경이 많이 없습니다 😅

혹시 보시면서 개선할 부분이 보이면 말해주시면 너무 감사할 것 같아요!!
이번 리뷰도 잘 부탁드립니다 ~🙇🏻‍♀️

Copy link

@rawfishthelgh rawfishthelgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조이 잘 구현해주셨습니다!
요구사항은 충분히 충족해 주셨는데, 몇 가지 코드 부분에서 의견을 듣고 싶어
한 번만 Request change 하도록 할게요ㅎㅎ

import org.slf4j.LoggerFactory;
import org.springframework.dao.DataAccessException;

public class BaseJdbcTemplate {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DataSource 커넥션 획득과, Sql 호출 동작을 담당(물론 실제 동작 로직은 바깥에서 functional interface로 전달하지만 )하는 클래스로 분리했네요!

JdbcTemplate 안에서 메소드로 이 동작을 처리할 수도 있을텐데 분리한 이유가 있을까요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

처음에는 단순 JdbcTemplate에서 중복적인 코드가 발생하기 때문에 메소드로 분리했었습니다.
그런데 만약 JdbcTemplate 외에도 다른 Template 들이 생겨나면 동일한 역할을 하는 메서드들을 다시 중복적으로 작성해야할 것 같다는 생각이 들어서 클래스 분리를 고민하게 되었습니다.
그리고 이렇게 커넥션 및 sql 호출하는 역할에 대한 작업을 별도 클래스로 분리하면 관리(유지보수) 측면에서도 좋을 것 같다는 생각도 들어서 분리하게 되었습니다!

final Connection connection = requireNonNull(dataSource).getConnection();
final PreparedStatement preparedStatement = connection.prepareStatement(sql)
) {
return action.execute(preparedStatement);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조이의 코드를 보다가 뭔가 저도 놓친 부분이 있는 것 같아 의견 달아봅니다.

개인적인 생각으로는, 이 클래스(메소드)에 커넥션 획득 + sql 호출까지 한 번에 담당하다 보니,
나중에 한 커넥션 안에서 여러 비즈니스 로직을 호출하는 경우가 어려울 것 같은 생각이 들어요!

한 트랜잭션에서 조회+업데이트+저장이 다 섞이려면 커넥션 획득과 sql 호출 작업이 분리되어야 하지 않을까요?

(다만 이 부분은 3단계에서 고민해도 될 것 같긴 합니다)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

하마드의 의견을 듣고 보니 하나의 트랜잭션 내에서 여러 작업을 수행하려면 다 다른 커넥션을 이용하겠군요..
작성하면서 생각 못한 부분이네요 ㅠ 3단계 하면서 좀 더 고민해보고 코드에 녹여보겠습니다!
좋은 조언 감사드려요👍

T result = baseJdbcTemplate.execute(sql,
preparedStatement -> {
setArguments(args, preparedStatement);
try (final ResultSet resultSet = preparedStatement.executeQuery()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

찾아보니까 PreparedStatement가 해제되면, ResultSet이 자동으로 해제되더라구요 저도 바꿨습니다ㅋㅋㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

좋은데요??????
덕분에 좋은 정보 얻어갑니다 ㅎㅋㅋㅋㅋ
적용 완~!

Comment on lines +71 to +73
public void execute(final String sql) {
baseJdbcTemplate.execute(sql, PreparedStatement::execute);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

자바마스터 같네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

정확한데요.

Comment on lines +7 to +10
public interface PreparedStatementAction<T> {

T execute(PreparedStatement preparedStatement) throws SQLException;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

확실히 함수형 인터페이스로 분리하니 유연하게 메소드 적용이 가능하네요

Comment on lines +17 to +32
private static final String INIT_USER_TABLE_SQL = "DROP TABLE IF EXISTS users; "
+ " "
+ "create table if not exists users ("
+ " id bigint auto_increment,"
+ " account varchar(100) not null,"
+ " password varchar(100) not null,"
+ " email varchar(100) not null,"
+ " primary key(id)"
+ ");";

private UserDao userDao;

@BeforeEach
void setup() {
DatabasePopulatorUtils.execute(DataSourceConfig.getInstance());
JdbcTemplate jdbcTemplate = new JdbcTemplate(DataSourceConfig.getInstance());
jdbcTemplate.execute(INIT_USER_TABLE_SQL);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 클리닝 작업까지 하셨네요 대단합니다...(전 안해서ㅎㅎ)

try (final Connection connection = requireNonNull(dataSource).getConnection();
final PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
setArgument(args, preparedStatement);
public <T> Optional<T> queryForObject(final String sql, final RowMapper<T> rowMapper, final Object... args) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이건 저도 여우에게 들은 피드백인데, 찾으려는 데이터가 존재하지 않을 경우 실제 jdbctemplate에서는 null 반환이 아닌 DataAccessException을 던지더라구요!
예외처리를 Dao 단에서 하는게 좋을지, 아니면 jdbctemplate에서 하는게 좋을지 한 번 고민해 보면 좋겠습니다.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오.. 좋은 고민거리네요..!!
언제나 그렇듯 정답이 없는 문제이기에 제 생각을 정리해봅니다 ㅎ

처리 기준은 데이터를 조회하려는데 데이터가 없는 상황이 문제 인 곳이 어디인가?로 정하면 좋을 것 같다는 생각을 했어요.
JdbcTemplate 의 경우, 데이터가 있다면 그 데이터를 주고 그게 아니라면 주지 않는게 핵심 역할이라고 생각해서 예외 상황으로 봐야할지 모호한 것 같아요.

반면에 Dao 혹은 Service 에서는 비지니스 로직에 따라서 데이터가 없는 것이 문제일 수 있겠네요.
그래서 지금처럼 처리하는 게 좀 더 적합하지 않을까? 싶습니다 ㅎㅎ
(근데 지금 생각해보니 이 예외는 서비스에서 처리하는게 더 자연스러운 것 같기도 하네요 🤔)

Copy link

@rawfishthelgh rawfishthelgh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

조이 코멘트 및 리뷰 반영 확인했습니다~!
3단계 진행시켜주세요

@rawfishthelgh rawfishthelgh merged commit fcfb34a into woowacourse:yeonkkk Oct 5, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants