[JDBC 라이브러리 구현하기 - 1단계] 후디(조동현) 미션 제출합니다. #115

devHudi · 2022-10-03T16:44:16Z

안녕하세요, 리차드 🙌
로또 미션 때 페어로 만난게 벌써 반년이 훌쩍 넘어갔네요. 시간 참 빨라요 😨

이번 미션을 진행하면서, 중복 코드가 많이 발생하더라구요.
예전 체스미션때 중복코드를 템플릿 메서드 패턴을 통해 해결해본적 있었는데, 이번엔 람다를 사용해서 한번 개선해보았습니다.

그런데, 아직 ResultSet 에서 getXXX() 를 해오거나, preparedStatement.executeUpdate() 같은것을 실행할 때 람다식 내부에서 SQLException 을 try - catch 해줘야 하는 것은 해결하지 못했어요. 아직 감이 잡히지 않네요!

아무튼 1단계 리뷰 잘 부탁드립니다!

devHudi · 2022-10-03T16:46:50Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+    public <T> T usePreparedStatement(final String sql, final Function<PreparedStatement, T> function,
+                                      final Object... args) {
+        try (Connection connection = dataSource.getConnection();
+             PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
+
+            for (int i = 0; i < args.length; i++) {
+                preparedStatement.setObject(i + 1, args[i]);
+            }
+
+            return function.apply(preparedStatement);
+        } catch (final SQLException e) {
+            throw new DataAccessException(e.getMessage(), e);
+            // TODO: 더 추상화된 예외 메시지를 사용해야함
+        }
+    }


Connection 을 얻어오고, PreparedStatement 를 얻어오고, PreparedStatement 에 값을 설정하고, Connection 과 PreparedStatement 를 해제하는 작업을 모두 이 곳에서 템플릿화 하였습니다.

훌륭합니다!
두번째 매개변수인 Function이 예외를 throw하지 못해서 내부에서 핸들링해야 하다보니
현재 usePreparedStatement를 호출하는 코드들이 다소 길어졌는데요,
이 부분이 개선되면 전체적으로 확 깔끔해질 것 같네요!!

devHudi · 2022-10-03T16:47:30Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+        return usePreparedStatement(sql, pstmt -> {
+            try {
+                T result = null;
+                ResultSet rs = pstmt.executeQuery();
+                if (rs.next()) {
+                    result = rowMapper.run(rs);
+                }
+
+                return result;
+            } catch (SQLException e) {
+                throw new DataAccessException(e.getMessage(), e);
+                // TODO: 더 추상화된 예외 메시지를 사용해야함
+            }
+        }, args);


람다 내부에서 발생하는 예외를 항상 람다에서 try - catch 해주고 있는데, 중복되는 try - catch를 없애기 좋은 방법 없을까요?

템플릿 메서드인 usePreparedStatement의 두번째 매개변수인 Function이 예외를 throws 하지 못해서
템플릿 메서드를 호출하는 쪽에서 try/catch를 해야 하는 상황이군요!

저는 관련해서 구구의 di 정답에서 예외를 감싸는 함수형 인터페이스를 직접 구현한 코드를 참고했습니다!

@FunctionalInterface public interface ThrowingFunction<T, R, E extends Exception> { R apply(T t) throws E; }

@HJ-Rich 우와 너무 좋은 아이디어인 것 같네요! 바로 적용해보았습니다!

devHudi · 2022-10-03T16:47:57Z

app/src/main/java/com/techcourse/dao/UserDao.java

+        return jdbcTemplate.query(sql, rs -> {
+            try {
+                return new User(
+                        rs.getLong(1),
+                        rs.getString(2),
+                        rs.getString(3),
+                        rs.getString(4));
+            } catch (SQLException e) {
+                throw new DataAccessException();
+            }
+        });


여기도 람다 내부에서 중복되는 try - catch 가 발생해요 😨

이 부분은 방금 UserDao 의 USER_ROW_MAPPER 라는 상수로 분리했어요. 그래도 여전히 try - catch 가 존재하는게 불편하네요 🥲 Spring의 Jdbc Template 에서는 ResultSet 의 데이터를 꺼낼때 try - catch 해주지 않았어도 됐는데 원리가 궁금하군요.

#115 (comment)
이 코멘트를 참고해주세요! ㅎ_ㅎ

HJ-Rich

후디 안녕하세요!

step1 구현 고생 많으셨습니다! 👏
아주 좋은 틀을 이미 구현해두신 것 같네요~
리팩터링으로 화룡점정만 하시면 될 것 같습니다.

그리고 이건 저도 리뷰어 아서에게 도움 받은 내용인데요~!🎶
Spring 의 JdbcTemplate 테스트를 참고해서 JdbcTemplateTest를 추가해보면 어떨까요?

모든 리뷰 내용은 자유롭게 참고하시고 반영하고 싶은 부분만 해주시면 되는데요~
후디의 생각은 꼭 코멘트로 남겨주셨으면 합니다 ㅎ

마지막으로 체크리스트 두 개 중에 JdbcTemplate에서 담당하기는 완료가 되었지만
UserDao 테스트는 현재 깨지고 있어요 ㅠ.ㅠ 확인 부탁드립니다.

그럼 화이팅!! 😃

HJ-Rich · 2022-10-04T06:38:43Z

jdbc/src/main/java/nextstep/jdbc/RowMapper.java

+@FunctionalInterface
+public interface RowMapper<T> {
+
+    T run(final ResultSet rs);


Suggested change

@FunctionalInterface

public interface RowMapper<T> {

T run(final ResultSet rs);

@FunctionalInterface

public interface RowMapper<T> {

T run(final ResultSet rs) throws SQLException;

RowMapper를 실제로 사용하는 곳은 JdbcTemplate 내부이므로
throws 선언을 통해 라이브러리 사용자가 try/catch를 하지 않아도 되게 해줄 수 있을 것 같아요!

아하 그렇군요 😲 RowMapper 를 생성하는 것은 클라이언트지만, 전달받아 직접 실행하는 측은 JdbcTemplate 라이브러리 이므로, 클라이언트가 직접 예외처리를 하지 않아도 되네요!

HJ-Rich · 2022-10-04T06:42:59Z

app/src/main/java/com/techcourse/dao/UserDao.java

    public void update(final User user) {
-        // todo
+        String sql = "update users set (account, password, email) = (?, ?, ?) where id = ?";
+        jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail(), user.getId());
    }


현재 UserDao#update 가 호출되는 곳이 프로덕션과 테스트 합하여 총 두 곳인데요,
둘 다 User객체의 비밀번호만 변경한 뒤 update를 호출하고 있습니다.

물론 app 모듈 내 사용자 정보 변경 정책이 분명히 드러나있지 않습니다.
하지만 통상적으로 account와 email 정보에 대한 변경이 자유롭지 못하다는 점,
애플리케이션 코드 내에서 비밀번호 변경 외 정보 변경 사례가 없다는 점에서
비밀번호 변경만 적용하면 어떨까 싶은데 후디 생각은 어떠세요?!

메소드 시그니쳐가 public void update(final User user) 인 것으로 미루어보아 메소드 의도 자체가 유저 데이터를 통째로 바꾸는 것이라고 이해했어요. 물론 일반적으로는 account 와 email 컬럼을 변경할 수 없는게 대부분이지만, 도메인에 따라 충분히 변경할 '수도' 있다고 사료돼요.

또한, "비밀번호만 변경할 수 있다" 는 비즈니스 적인 요구사항이라고 생각되는데요, DAO 같은 경우에는 레이어드 아키텍처로 미루어보아 그 관심사가 비즈니스 요구사항보다는 데이터베이스 로직에 대한 추상화가 더 우선적이라고 생각해요. "비밀번호만 변경할 수 있다" 라는 비즈니스 요구사항은 그 상위 계층 (이를테면 서비스 레이어) 에서 관심을 가져야 한다고 생각합니다!

리차드는 어떻게 생각하시나요?

오 후디의 생각을 들려주셔서 감사합니다!
후디의 의견에 공감합니다~!

이하는 개인적인 의견입니다! ㅎㅎ

저는 DAO가 어떤 메서드를 개방해두었느냐에 관심을 두는 편이에요.
그래서 Jpa를 사용할 때에도 JpaRepository를 사용하지 않고 Repository를 사용해서
꼭 사용되는 메서드만 직접 선언해서 개방해두는 편입니다.

이번 상황도 애플리케이션 레이어에서 UserDao를 의존하며 데이터 작업을 수행하는 상황인데요,
UserDao가 유저 정보를 모두 변경할 수 있는 메시지를 개방하고 있다는 것 자체가 하나의 의미가 될 수 있다고 보는 편입니다.

HJ-Rich · 2022-10-04T06:44:44Z

app/src/main/java/com/techcourse/dao/UserDao.java

+        String sql = "insert into users (account, password, email) values (?, ?, ?)";
+        jdbcTemplate.update(sql, user.getAccount(), user.getPassword(), user.getEmail());
    }


INSERT와 UPDATE가 모두 JdbcTemplate의 update 메서드를 호출하고 있는데요,
메서드명 관련해서 후디의 생각이 궁금합니다!

JdbcTemplate 라이브러리의 역할은 JDBC의 낮은 수준의 Exception을 고수준으로 추상화하고, placeholder(?)에 값을 매핑하는 과정을 추상화 하는 것이라고 생각해요. 사용자는 JdbcTemplate에 INSERT, UPDATE 를 포함하여 많은 DML, DDL, DCL 들을 넣어 실행할 수 있습니다. 따라서 SQL 쿼리 단위로 메소드를 분리하기 보다는 이 쿼리가 데이터를 변경시키는가, 아니면 단순히 데이터를 가져오는가 정도로만 메소드를 분리해도 충분하다고 생각했습니다. INSERT, UPDATE, DELETE 에 따라 메소드를 다르게 분리하는 작업은 DAO와 같이 JdbcTemplate 보다 더 높은 추상화 단계에서 수행되어도 충분하다고 생각합니다!

좋습니다! 실제 메서드 컨벤션도 유사하게 구성되어 있는 사례를 보았지만
후디의 의견으로 들으니 시원시원하군요! 공감합니다 ㅎ

HJ-Rich · 2022-10-04T06:46:10Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

 public class JdbcTemplate {

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


log가 사용되지 않고 있네요~

JdbcTemplate 내부는 예외가 많이 발생할 수 있는 지점이고
라이브러리 특성 상 사용자가 직접 구현하지 않은 부분에서 예외가 발생한다면
상세한 로그를 기록해주는 게 사용성 측면에서 긍정적일 것 같아요

예외가 발생했을 때 예외 정보를 로깅해보면 어떨까요?

로그를 사용해야할 것 같네요! 로깅을 추가해보겠습니다 🙂

HJ-Rich · 2022-10-04T06:48:22Z

app/src/main/java/com/techcourse/dao/UserDao.java

+        try {
+            return new User(
+                    rs.getLong(1),
+                    rs.getString(2),
+                    rs.getString(3),
+                    rs.getString(4));
+        } catch (SQLException e) {
+            throw new DataAccessException();
+        }


인덱스로만 작성하면 User 객체 필드가 늘어날 때 리팩터링이 어려울 것 같아요.
저는 컬럼명을 직접 문자열로 작성해주는 방식을 사용해봤는데요, 후디 생각은 어떠세요?

테이블 컬럼 순서만 변경되더라도 제대로 동작하지 않겠네요! 컬럼 이름을 전달해주는 방식으로 리팩토링했습니다!

HJ-Rich · 2022-10-04T06:48:53Z

app/src/main/java/com/techcourse/dao/UserDao.java

    public User findByAccount(final String account) {
-        // todo
-        return null;
+        String sql = "select id, account, password, email from users where account = ?";
+        return jdbcTemplate.queryForObject(sql, USER_ROW_MAPPER, account);
    }


훌륭하게 라이브러리를 구현해주셔서 사용자의 코드가 아주 깔끔하군요!! 💯

HJ-Rich · 2022-10-04T06:52:23Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+        return usePreparedStatement(sql, pstmt -> {
+            try {
+                T result = null;
+                ResultSet rs = pstmt.executeQuery();
+                if (rs.next()) {
+                    result = rowMapper.run(rs);
+                }
+
+                return result;
+            } catch (SQLException e) {
+                throw new DataAccessException(e.getMessage(), e);
+                // TODO: 더 추상화된 예외 메시지를 사용해야함
+            }
+        }, args);


템플릿 메서드인 usePreparedStatement의 두번째 매개변수인 Function이 예외를 throws 하지 못해서
템플릿 메서드를 호출하는 쪽에서 try/catch를 해야 하는 상황이군요!

저는 관련해서 구구의 di 정답에서 예외를 감싸는 함수형 인터페이스를 직접 구현한 코드를 참고했습니다!

@FunctionalInterface public interface ThrowingFunction<T, R, E extends Exception> { R apply(T t) throws E; }

HJ-Rich · 2022-10-04T06:53:14Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+                List<T> results = new ArrayList<>();
+                ResultSet rs = pstmt.executeQuery();
+                if (rs.next()) {
+                    T result = rowMapper.run(rs);
+                    results.add(result);


Row를 매핑하는 곳을 메서드 추출해보면 어떨까요?

추출하였습니다. 더 깔끔해졌네요 🙂

HJ-Rich · 2022-10-04T06:54:11Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+            for (int i = 0; i < args.length; i++) {
+                preparedStatement.setObject(i + 1, args[i]);
+            }


파라미터를 매핑하는 부분을 메서드 추출해보면 어떨까요?

추출하였습니다 🙂

HJ-Rich · 2022-10-04T06:55:01Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+    public <T> T usePreparedStatement(final String sql, final Function<PreparedStatement, T> function,
+                                      final Object... args) {
+        try (Connection connection = dataSource.getConnection();
+             PreparedStatement preparedStatement = connection.prepareStatement(sql)) {
+
+            for (int i = 0; i < args.length; i++) {
+                preparedStatement.setObject(i + 1, args[i]);
+            }
+
+            return function.apply(preparedStatement);
+        } catch (final SQLException e) {
+            throw new DataAccessException(e.getMessage(), e);
+            // TODO: 더 추상화된 예외 메시지를 사용해야함
+        }
+    }


훌륭합니다!
두번째 매개변수인 Function이 예외를 throw하지 못해서 내부에서 핸들링해야 하다보니
현재 usePreparedStatement를 호출하는 코드들이 다소 길어졌는데요,
이 부분이 개선되면 전체적으로 확 깔끔해질 것 같네요!!

HJ-Rich

후디~ 피드백을 모두 멋지게 반영하셨네요!👏👏👏

테스트도 정상적으로 통과하는 점 확인했습니다!💯
요구사항을 모두 충족하셔서 1단계 머지 바로 가겠습니다~!

한 가지 피드백 드릴 내용이 있는데요!
궁금하시라고 코멘트로 남겨두겠습니다 ㅎ_ㅎㅋㅋ
이건 2단계에서 반영 부탁드릴게요!

오늘도 고생 많으셨습니다!
2단계도 화이팅이에요 후디~ 🎶

HJ-Rich · 2022-10-05T09:31:22Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

-                return pstmt.executeUpdate(sql);
+                return pstmt.executeUpdate();


요녀석이 테스트를 깨뜨린 주범이었군요.. ㅋㅋ

맞아요 ㅋㅋㅋㅋ 메소드 분리하면서 실수....

HJ-Rich · 2022-10-05T09:31:46Z

jdbc/src/main/java/nextstep/jdbc/RowMapper.java

-    T run(final ResultSet rs);
+    T run(final ResultSet rs) throws SQLException;


HJ-Rich · 2022-10-05T09:39:13Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+        List<T> results = query(sql, rowMapper, args);
+        return results.get(FIRST_RESULT_INDEX);


처음부터 아키텍처가 좋아서 리팩터링이 뚝딱뚝딱 되네요!! 🔨

결과값이 1개여야 하는데 아닐 때는 예외를 발생시켜보면 어떨까요?

좋은 의견이네요! 다음번 단계에서 바로 반영하겠습니다!

HJ-Rich · 2022-10-05T09:39:54Z

jdbc/src/main/java/nextstep/jdbc/ThrowingFunction.java

+@FunctionalInterface
+public interface ThrowingFunction<T, R, E extends Exception> {
+
+    R apply(final T t) throws E;
+}


멋져요~! depth들도 얕아지고 코드도 깔끔해졌네요~! 👍

HJ-Rich · 2022-10-05T09:40:26Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+            ResultSet rs = pstmt.executeQuery();
+            if (rs.next()) {
+                T result = rowMapper.run(rs);
+                results.add(result);
            }


현재 쿼리 결과 행이 여러 행이더라도 무조건 1행만 매핑되어 응답되고 있어요!! 👀

헐 그러네요!.. while을 써야하는걸 if를 썼네요 😅

HJ-Rich · 2022-10-05T09:40:43Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

        } catch (final SQLException e) {
+            log.error("error: {}", e);


HJ-Rich · 2022-10-05T09:40:56Z

app/src/main/java/com/techcourse/dao/UserDao.java

+            rs.getLong("id"),
+            rs.getString("account"),
+            rs.getString("password"),
+            rs.getString("email")


HJ-Rich · 2022-10-05T09:41:17Z

jdbc/src/main/java/nextstep/jdbc/JdbcTemplate.java

+    private void mapParametersToPreparedStatement(final PreparedStatement preparedStatement, final Object[] args)
+            throws SQLException {
+        for (int i = 0; i < args.length; i++) {
+            preparedStatement.setObject(i + 1, args[i]);
+        }
+    }


devHudi added 4 commits October 4, 2022 00:35

feat: update 및 query 메소드 구현

d572591

feat: queryForObject 구현

4c0bb9f

refactor: UserDao의 DataSource 필드 제거

18f4001

refactor: 람다를 사용한 템플릿 메소드 패턴을 사용해 중복 코드 제거

e52077c

devHudi requested a review from HJ-Rich October 3, 2022 16:44

devHudi self-assigned this Oct 3, 2022

devHudi commented Oct 3, 2022

View reviewed changes

refactor: User에 대한 RowMapper를 상수로 등록

32cf14d

HJ-Rich suggested changes Oct 4, 2022

View reviewed changes

devHudi added 8 commits October 5, 2022 12:05

fix: executeUpdate에 SQL을 전달하는 코드를 제거

60342cc

refactor: RowMapper의 run 메소드가 SQLException 처리를 미루도록 변경

f5876a1

refactor: queryForObject가 query 메소드를 재사용하도록 하여 중복 코드 제거

1277be0

refactor: ThrowingFunction 함수형 인터페이스를 정의하여 불필요한 try - catch 제거

3ea42e7

feat: 예외 발생시 로그하도록 개선

6fbe883

refactor: ResultSet에서 값을 꺼내올 때 인덱스 대신 컬럼 이름으로 꺼내오도록 개선

536a3b0

refactor: PreparedStatement에 파라미터를 매핑하는 로직을 메소드 추출

6dee265

refactor: ResultSet을 객체 리스트로 매핑하는 로직을 메소드로 분리

90ed577

HJ-Rich approved these changes Oct 5, 2022

View reviewed changes

HJ-Rich merged commit 76e9d1a into woowacourse:devhudi Oct 5, 2022

		public class JdbcTemplate {

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

		return pstmt.executeUpdate(sql);
		return pstmt.executeUpdate();

		T run(final ResultSet rs);
		T run(final ResultSet rs) throws SQLException;

		List<T> results = query(sql, rowMapper, args);
		return results.get(FIRST_RESULT_INDEX);

[JDBC 라이브러리 구현하기 - 1단계] 후디(조동현) 미션 제출합니다. #115

[JDBC 라이브러리 구현하기 - 1단계] 후디(조동현) 미션 제출합니다. #115

Conversation

devHudi commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HJ-Rich left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HJ-Rich left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HJ-Rich left a comment •

edited

Loading