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 라이브러리 구현하기 - 1단계] 헙크(정현승) 미션 제출합니다. #294

Merged
merged 8 commits into from
Sep 30, 2023
30 changes: 24 additions & 6 deletions app/src/test/java/com/techcourse/dao/UserDaoTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,22 @@
import com.techcourse.support.jdbc.init.DatabasePopulatorUtils;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;

import java.util.List;

import static org.assertj.core.api.Assertions.assertThat;

class UserDaoTest {

private UserDao userDao;
private JdbcTemplate template;

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

userDao = new UserDao(DataSourceConfig.getInstance());
final var user = new User("gugu", "password", "[email protected]");
Expand All @@ -23,22 +29,25 @@ void setup() {

@Test
void findAll() {
final var users = userDao.findAll();
final String sql = "SELECT id, account, password, email FROM users";
final List<User> users = template.query(sql, userRowMapper());

Choose a reason for hiding this comment

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

전체적으로 테스트가 jdbcTemplate으로 진헹되었는데여요. UserDaoTest라는 클래스와는 맞지 않는거 같아요. userDao를 사용하는 방향으로 변경해보면 어떨까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

그렇네요! userDao의 존재를 전혀 알아차리지 못했네요.. ㅎㅎㅎ 바로 dao의 로직들을 JdbcTemplate으로 이동시켜야 한다고 생각했던 것 같습니다. 현재는 JdbcTemplate에서는 라이브러리처럼 CRUD에 대한 로직을 남겨두었고, Dao는 app 패키지에 있는 만큼, 데이터에 접근하기 위한 클래스이면서 비즈니스 로직을 포함하도록 sql 문이라든지, 상황에 맞는 파라미터 값을 넘겨주도록 하였습니다. UserDaoTest는 롤백시켜두었습니다! 감사합니다 :)


assertThat(users).isNotEmpty();
}

@Test
void findById() {
final var user = userDao.findById(1L);
final String sql = "SELECT id, account, password, email FROM users WHERE id = ?";
final User user = template.queryForObject(sql, userRowMapper(), 1L).orElseThrow();

assertThat(user.getAccount()).isEqualTo("gugu");
}

@Test
void findByAccount() {
final var account = "gugu";
final var user = userDao.findByAccount(account);
final String sql = "SELECT id, account, password, email FROM users WHERE account = ?";
final User user = template.queryForObject(sql, userRowMapper(), account).orElseThrow();

assertThat(user.getAccount()).isEqualTo(account);
}
Expand All @@ -47,7 +56,8 @@ void findByAccount() {
void insert() {
final var account = "insert-gugu";
final var user = new User(account, "password", "[email protected]");
userDao.insert(user);
final String sql1 = "INSERT INTO users(account, password, email) VALUES(?, ?, ?)";
final int updatedRow = template.update(sql1, user.getAccount(), user.getPassword(), user.getEmail());

final var actual = userDao.findById(2L);

Expand All @@ -60,10 +70,18 @@ void update() {
final var user = userDao.findById(1L);
user.changePassword(newPassword);

userDao.update(user);

final String sql2 = "UPDATE users SET password = ? WHERE id = ?";
template.update(sql2, user.getPassword(), user.getId());
final var actual = userDao.findById(1L);

assertThat(actual.getPassword()).isEqualTo(newPassword);
}

private RowMapper<User> userRowMapper() {
return rs -> new User(
rs.getString("account"),
rs.getString("password"),
rs.getString("email")
);
}
Copy link

@cookienc cookienc Sep 29, 2023

Choose a reason for hiding this comment

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

이 부분도 UserDao 클래스에 들어있으면 더 좋을 것 같아요.
추가적으로 userRowMapper를 private 메서드로 사용할 수도 있고 상수로도 선언할 수 있는데요. 헙크는 어떤 이유 때문에 이렇게 선언하셨나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

상수로 만든다는 생각은 해보지 못했는데, 많이 쓰이는 메서드인 만큼 상수로 만들어두면 확실히 효율적일 것이라고 생각이 들었습니다! 대단한데요??!?!? 생각해보지 못한 부분이었는데 감사합니다 :)

}
58 changes: 53 additions & 5 deletions jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java
Original file line number Diff line number Diff line change
@@ -1,17 +1,65 @@
package org.springframework.jdbc.core;

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

import javax.sql.DataSource;
import java.sql.Connection;
import java.sql.PreparedStatement;
import java.sql.ResultSet;
import java.sql.SQLException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

public class JdbcTemplate {

private static final Logger log = LoggerFactory.getLogger(JdbcTemplate.class);

private final DataSource dataSource;

public JdbcTemplate(final DataSource dataSource) {
this.dataSource = dataSource;
}

public <T> Optional<T> queryForObject(final String sql, final RowMapper<T> rowMapper, Object... args) {
try (final Connection conn = dataSource.getConnection();
final PreparedStatement pstmt = conn.prepareStatement(sql)) {
for (int i = 0; i < args.length; i++) {
pstmt.setObject(i + 1, args[i]);
}
try (final ResultSet rs = pstmt.executeQuery()) {
if (rs.next()) {
return Optional.ofNullable(rowMapper.mapRow(rs));

Choose a reason for hiding this comment

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

nullable 좋네요👍👍

}
return Optional.empty();
}
} catch (final SQLException e) {
throw new DataAccessException(e);

Choose a reason for hiding this comment

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

DataAccessException으로 바꿔 던지는거 좋습니다👍

}
}

public <T> List<T> query(String sql, RowMapper<T> rowMapper) throws DataAccessException {
try (final Connection conn = dataSource.getConnection();
final PreparedStatement pstmt = conn.prepareStatement(sql)) {
final List<T> result = new ArrayList<>();
try (final ResultSet rs = pstmt.executeQuery()) {
if (rs.next()) {
result.add(rowMapper.mapRow(rs));
}
return result;
}

} catch (final SQLException e) {
throw new DataAccessException(e);
}
}

public int update(final String sql, final Object... args) {
try (final Connection conn = dataSource.getConnection();
final PreparedStatement pstmt = conn.prepareStatement(sql)) {
for (int i = 0; i < args.length; i++) {
pstmt.setObject(i + 1, args[i]);
}
Comment on lines +57 to +59

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.

메서드를 추출한다면 해당 메서드로 이동하여 로직을 파악하여야 하기 때문에, 전체적인 코드의 맥락을 바로 파악하기엔 추출하지 않는 것도 좋다고 생각합니다 ㅎㅎㅎ 조금 더 복잡한 로직이었더라면 메서드로 추출하고 네이밍을 주어 가독성을 높일 수 있을 것 같다는 생각을 했습니다.

Choose a reason for hiding this comment

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

좋습니다! 확실히 가독성이 떨어지긴 하죠!

return pstmt.executeUpdate();
} catch (final SQLException e) {
throw new DataAccessException(e);
}
}
}
13 changes: 13 additions & 0 deletions jdbc/src/main/java/org/springframework/jdbc/core/RowMapper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.springframework.jdbc.core;

import javax.annotation.Nullable;
import java.sql.ResultSet;
import java.sql.SQLException;

@FunctionalInterface
public interface RowMapper<T> {

@Nullable

Choose a reason for hiding this comment

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

Nullable을 붙이신 이유가 따로 있을까요?(진짜 궁금)

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 라이브러리에서도 이 부분에 @Nullable 애너테이션이 붙어있어서 따라했습니다. 일단 이 애너테이션을 붙임으로써, 해당 메서드 mapRow의 반환값이 Null일 수 있음을 다른 개발자들이 조금 더 잘 인지할 수 있도록 하기 위함이라고 합니다.

실제로 저도 JdbcTemplate에서 mapRow()를 활용한 부분을 보자면 저는 queryForObjectquery 메서드에서 사용했습니다. 전자는 하나의 객체만 반환하는 것인데, 값이 없으면 원래는 null을 반환하곤 합니다. 저는 이를 막기 위해 Optional로 감싸 놓은 상태입니다. 후자 역시 null을 반환할 수 있지만 반환 타입이 List이므로 null에 대한 핸들링을 굳이 해주지 않았습니다.

어쨌든 저도 따라 붙이긴 했는데, 다른 개발자들에게 해당 메서드가 null을 반환할 수 있다고 알려주는 역할이라고 하네요! ㅎㅎㅎㅎ

Choose a reason for hiding this comment

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

반환값이 null 이 될 수도 있다는 뜻이었군요! 배워갑니다~😄

T mapRow(ResultSet rs) throws SQLException;

}
Loading