[ JDBC 2단계] 박스터 미션 제출합니다. #282

drunkenhw · 2023-09-27T05:40:09Z

안녕하세요 베베 2단계는 리팩터링이 요구사항이라 리팩터링을 했습니다.
제가 구현한 코드보다 나은 방법이 있을 방법이 있을 것 같은데 머리가 굳었는지 고정관념 때문인지 생각이 안나네요
베베의 조언 기다리겠습니다.

wonyongChoi05

안녕하세요 빡스터~ 이번 리뷰가 좀 늦었죠 ㅠ 이번에도 역시나 코드를 깔끔하게 잘 짜주셔서 크게 궁금하거나 의문이 드는 점은 없었어요! PreparedStatement를 만드는 과정을 인터페이스로 한 번 더 분리해주신게 인상 깊었는데, 다만 해당 PreparedStetementExecutor를 사용할 때 구현체를 만들어주지 않은 점이 궁금했어요. 뭔가 제가 모르는 이유가 있어서였겠죠..? 만약 일부러 구현체를 만들어주시지 않은거라면 저도 알려주세요 ㅋㅋㅋ! 슬쩍 훑어봐주시고 코멘트 부탁드릴게요 ㅎㅎ 즐거운 추석 보내세용~

wonyongChoi05 · 2023-09-29T05:42:10Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

-                    rs.close();
+        log.debug("query : {}", sql);
+
+        return execute(sql, new PreparedStatementExecutor<T>() {


new PreparedStatementExecutor<T>

위 부분에서 제네릭 T타입을 선언하신 이유는 뭔가요?? (사용하지 않고 있어서 여쭤봅니다!)

지울게요 감ㄱ사합니다

wonyongChoi05 · 2023-09-29T05:46:39Z

jdbc/src/main/java/org/springframework/jdbc/core/PreparedStatementExecutor.java

+    T fetchData(ResultSet resultSet) throws SQLException;
+
+    ResultSet fetchResultSet(PreparedStatement preparedStatement) throws SQLException;


SQLException을 메서드 시그니처로 처리하신 이유가 있으신가요? 만약 SQLException을 받았을 때 어떠한 후처리를 해줘야 할 때는 어떻게 해야할까요?

preparedStatement의 execute 메서드를 실행하면 여러가지 이유로 예외가 발생할 수 있습니다. 예를 들어 DB와 커넥션이 안된다던지, timeout이 발생한다던지, SQL 문법이 틀렸는 경우에 발생합니다.
그래서 따로 JdbcTemplate 클래스 내에서 catch하여 무언가를 하기보다는 application 에서 예외를 로깅하는 방법이 좋지않을까요?

정정
jdbc template에서 캐치해서 다른 예외로 던지는 것이 좋을 것 같습니다

wonyongChoi05 · 2023-09-29T05:51:22Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+            @Override
+            public ResultSet fetchResultSet(PreparedStatement preparedStatement) throws SQLException {
+                return preparedStatement.executeQuery();
            }
+        }, args);
+    }

-            try {
-                if (conn != null) {
-                    conn.close();
+    public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... args) {
+        log.debug("query : {}", sql);
+        return execute(sql, new PreparedStatementExecutor<>() {
+            @Override
+            public List<T> fetchData(ResultSet resultSet) throws SQLException {
+                List<T> results = new ArrayList<>();
+                while (resultSet.next()) {
+                    results.add(rowMapper.mapRow(resultSet));
                }
-            } catch (SQLException ignored) {
+                return results;


현재 PreparedStatementExecutor 인터페이스를 사용하는 쪽에서 모두 구현을 담당해서 복잡하고 가독성이 떨어지는 느낌이 들어요! PreparedStatementExecutor의 구현체를 만들어서 관리해주면 어떨까요?

저도 구현체를 만들까하다가 이렇게 구현한 이유를 말씀드리겠습니다

해당 메서드에서만 사용하는 부분인데 전부 구현체로 만든다면 오히려 어플리케이션의 복잡도를 올린다 생각했습니다. 매번 새로운 메서드를 만들게 된다면 이 인터페이스를 구현한 클래스도 만들어야하고 그렇게 된다면 어떻게 구현했길래 동작하는지, 버그가 나는지 클래스를 두번 타고 들어가야합니다. 매우 귀찮은 일이라고 생각했습니다.

그리고 이렇게 익명 클래스로 만들게 된다면 rowMapper를 파라미터로 갖지않고도 해당 인터페이스 구현체가 사용할 수 있습니다. 조회를 하는 메서드에서는 rowMapper가 필요하겠지만, 그냥 실행만 하는 sql에서는 row mapper를 필요로 하지 않습니다. 사용하지 않는 파라미터를 추가하는 것 보다 이렇게 편하게 받는 것이 좋다고 생각했습니다.

이상입니다.

오호 빡스터가 생각하는 구현체를 만들지 않은 이유는 다음과 같다고 볼 수 있겠네요!

복잡도 : 현재의 구현은 메서드 내에서 직접 익명 클래스를 사용하여 구현하고 있는데, 간단한 작업을 수행하는 메서드에서만 사용되는 인터페이스라는 점을 고려할 때, 별도의 클래스를 만들어서 관리하는 것은 오히려 더 복잡성을 높일 수 있음. 또한 구현체를 만들게 되면 해당 클래스가 어떻게 동작하고 어떤 버그가 발생할 수 있는지를 파악하기 위해 두 단계를 거쳐야 하는데, 그 자체가 복잡도 증가로 이어질 수 있음.

파라미터의 유연함 : 이 방식이 메서드에 필요한 파라미터만 받도록 하여 불필요한 파라미터 추가를 피하기 위해서임. 조회를 수행하는 메서드에서만 rowMapper가 필요한 경우, 구현체를 만들지 않고도 필요한 파라미터만 전달할 수 있음.

그렇다면 사용하는 쪽에서의 복잡도와 가독성은 어떻게 관리해주는게 좋을까요? 제가 기존 빡스터의 메서드를 분리해보았는데, 이런 형태를 재사용 할 수 있을까요? 그리고 메서드를 분리하면 가독성이 조금이나마 좋아질 수 있을까요?

public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... args) { log.debug("query : {}", sql); return executeQuery(sql, args, preparedStatement -> { try (ResultSet resultSet = preparedStatement.executeQuery()) { List<T> results = new ArrayList<>(); while (resultSet.next()) { results.add(rowMapper.mapRow(resultSet)); } return results; } catch (SQLException e) { throw new RuntimeException(e); } }); } // Function은 함수형 인터페이스 private <T> T executeQuery(String sql, Object[] args, Function<PreparedStatement, T> queryFunction) { try (Connection connection = dataSource.getConnection(); PreparedStatement preparedStatement = getPreparedStatement(sql, connection, args)) { return queryFunction.apply(preparedStatement); } catch (SQLException e) { log.error(e.getMessage(), e); throw new RuntimeException(e); } }

기존에 존재하는 execute 메서드랑은 별도의 메서드입니다.

굳이 이렇게 분리하는 방법이 아니더라도(빡스터의 코드이기 때문에 더 좋은 방법이 빡스터가 더 좋은 코드를 만들어주시겠죠!?) 빡스터는 이런 복잡성을 관리하는 방법들에 대해서 어떤 생각을 가지고 계신지도 궁금하네요! 무조건 분리해라!라기 보다는, 기존의 사용하는 쪽의 가독성도 어떻게 챙겨볼 수 있을지 고민해보셨으면 좋을 것 같아서 장문의 코멘트 남깁니다 ㅎㅎ..

제가 작성한 코드라 제가 보기에는 가독성이 그렇게 떨어진다고 생각하지 않았는데 배배가 남긴 리뷰를 보고 가독성이 떨어진다고 생각하여 조금 다른 방법으로 코드를 리팩토링 했습니다. 그리고 구현 클래스도 만들었습니다.

wonyongChoi05 · 2023-09-29T05:54:38Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

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

    public <T> T queryForObject(String sql, RowMapper<T> rowMapper, Object... args) {


만약 조회된 결과가 null이면 null 그대로 줘야 할까요?? 아니면 어떻게 처리해줘야 할까요? 받는 곳에서는 항상 NPE 검증을 해줘야 안심하고 사용할 수 있는걸까요?

Optional을 이용하겠습니다

wonyongChoi05 · 2023-09-29T05:58:17Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

        } catch (SQLException e) {
            log.error(e.getMessage(), e);
            throw new RuntimeException(e);


SQLException이 발생하는 상황은 어떤 상황일까요? 그런 상황을 생각했을 때 RuntimeException과 같은 상위의 추상화된 예외를 던지는게 좋을까요, DataAccessException와 같은 구체화된 예외를 던지는게 좋을까요??

정답 2번

wonyongChoi05 · 2023-09-29T05:59:25Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+        log.debug("query : {}", sql);
+        execute(sql, new PreparedStatementExecutor<>() {
+            @Override
+            public Object fetchData(ResultSet resultSet) throws SQLException {


사용하지 않는 메서드 시그니처는 지워도 될 것 같아요!

오우 이건 인터페이스라 구현이 필수라 어쩔 수 없는 부분입니다..
필요없는 곳에는 다른 방법으로 구현하는 방법을 생각해보겠습니다.

이 부분 제가 클론 받아서 지워봤는데 지워지긴 하더라구요! (다른 방법으로 구현하신다면 어차피 해결되긴 하겠지만요 ㅎㅎ..)

메서드 시그니처가 뭐길래 저는 지우면 에러가 나죠? 혹시 한국어로 설명 가능할까요? 진짜 몰라서 그럽니다.. 찾아봐도 베베가 말하는 것이랑 제가 생각하는 것이랑 다른 것 같아서 정확히 어디를 지우라는 뜻인가요?...

제가 설명이 부족했군요..ㅋㅋㅋ 죄송합니다 ㅠ 말보다 사진이 더 이해하기 좋을 것 같아서 사진 첨부드립니다!

아하 이 부분을 그렇게 부르나보네요 감사합니다

drunkenhw

베베 일단 코멘트는 남겼습니다. 좀 더 보완해서 다시 리뷰요청 하겠습니다.

drunkenhw · 2023-09-29T06:24:09Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

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

    public <T> T queryForObject(String sql, RowMapper<T> rowMapper, Object... args) {


Optional을 이용하겠습니다

drunkenhw · 2023-09-29T06:24:18Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

-                    rs.close();
+        log.debug("query : {}", sql);
+
+        return execute(sql, new PreparedStatementExecutor<T>() {


지울게요 감ㄱ사합니다

drunkenhw · 2023-09-29T06:25:46Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

        } catch (SQLException e) {
            log.error(e.getMessage(), e);
            throw new RuntimeException(e);


정답 2번

drunkenhw · 2023-09-29T13:27:51Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+        log.debug("query : {}", sql);
+        execute(sql, new PreparedStatementExecutor<>() {
+            @Override
+            public Object fetchData(ResultSet resultSet) throws SQLException {


오우 이건 인터페이스라 구현이 필수라 어쩔 수 없는 부분입니다..
필요없는 곳에는 다른 방법으로 구현하는 방법을 생각해보겠습니다.

drunkenhw · 2023-09-29T13:28:19Z

jdbc/src/main/java/org/springframework/jdbc/core/PreparedStatementExecutor.java

+    T fetchData(ResultSet resultSet) throws SQLException;
+
+    ResultSet fetchResultSet(PreparedStatement preparedStatement) throws SQLException;


preparedStatement의 execute 메서드를 실행하면 여러가지 이유로 예외가 발생할 수 있습니다. 예를 들어 DB와 커넥션이 안된다던지, timeout이 발생한다던지, SQL 문법이 틀렸는 경우에 발생합니다.
그래서 따로 JdbcTemplate 클래스 내에서 catch하여 무언가를 하기보다는 application 에서 예외를 로깅하는 방법이 좋지않을까요?

drunkenhw · 2023-09-29T13:32:21Z

jdbc/src/main/java/org/springframework/jdbc/core/JdbcTemplate.java

+            @Override
+            public ResultSet fetchResultSet(PreparedStatement preparedStatement) throws SQLException {
+                return preparedStatement.executeQuery();
            }
+        }, args);
+    }

-            try {
-                if (conn != null) {
-                    conn.close();
+    public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... args) {
+        log.debug("query : {}", sql);
+        return execute(sql, new PreparedStatementExecutor<>() {
+            @Override
+            public List<T> fetchData(ResultSet resultSet) throws SQLException {
+                List<T> results = new ArrayList<>();
+                while (resultSet.next()) {
+                    results.add(rowMapper.mapRow(resultSet));
                }
-            } catch (SQLException ignored) {
+                return results;


저도 구현체를 만들까하다가 이렇게 구현한 이유를 말씀드리겠습니다

해당 메서드에서만 사용하는 부분인데 전부 구현체로 만든다면 오히려 어플리케이션의 복잡도를 올린다 생각했습니다. 매번 새로운 메서드를 만들게 된다면 이 인터페이스를 구현한 클래스도 만들어야하고 그렇게 된다면 어떻게 구현했길래 동작하는지, 버그가 나는지 클래스를 두번 타고 들어가야합니다. 매우 귀찮은 일이라고 생각했습니다.

그리고 이렇게 익명 클래스로 만들게 된다면 rowMapper를 파라미터로 갖지않고도 해당 인터페이스 구현체가 사용할 수 있습니다. 조회를 하는 메서드에서는 rowMapper가 필요하겠지만, 그냥 실행만 하는 sql에서는 row mapper를 필요로 하지 않습니다. 사용하지 않는 파라미터를 추가하는 것 보다 이렇게 편하게 받는 것이 좋다고 생각했습니다.

이상입니다.

drunkenhw · 2023-09-29T16:49:24Z

배배 안녕하세요
열심히 구현하다 문득 든 생각이 있습니다. 가독성이라는게 참 어렵네요 누굴 기준으로 맞춰야할까요? 누가봐도 쉽게 읽혀야겠죠? 그럼 추상화를 하면 가독성이 떨어지지 않을까요? 그럼 절차지향으로 작성해야할까요? 도대체 뭘까요?
진짜 자기가 짠 코드에 대해 자기 기준으로 하면 어떤 코드도 난 읽을 수 있으니 괜찮아 라고 할 것 같은데 배배는 어떻게 생각하시나요?

코드를 깔끔하게 짜기위해 열심히 인터페이스를 이용했는데, 깔끔한게 베스트일까요? 아니면 좀 덜 깔끔하더라도 잘 읽히는게 좋을까요?
아니면 변경에 유연한 코드가 좋은 것일까요? 아니면 변경에 취약하지만 변경해야할 때 누가 변경해도 쉬운 코드가 좋을까요?

참 어렵습니다 배배의 의견 들려주세요
너무 길게는 작성하지 않으셔도 됩니다. 괜히 미안하니깐요

wonyongChoi05 · 2023-09-30T04:22:07Z

배배 안녕하세요 열심히 구현하다 문득 든 생각이 있습니다. 가독성이라는게 참 어렵네요 누굴 기준으로 맞춰야할까요? 누가봐도 쉽게 읽혀야겠죠? 그럼 추상화를 하면 가독성이 떨어지지 않을까요? 그럼 절차지향으로 작성해야할까요? 도대체 뭘까요? 진짜 자기가 짠 코드에 대해 자기 기준으로 하면 어떤 코드도 난 읽을 수 있으니 괜찮아 라고 할 것 같은데 배배는 어떻게 생각하시나요?

코드를 깔끔하게 짜기위해 열심히 인터페이스를 이용했는데, 깔끔한게 베스트일까요? 아니면 좀 덜 깔끔하더라도 잘 읽히는게 좋을까요? 아니면 변경에 유연한 코드가 좋은 것일까요? 아니면 변경에 취약하지만 변경해야할 때 누가 변경해도 쉬운 코드가 좋을까요?

참 어렵습니다 배배의 의견 들려주세요 너무 길게는 작성하지 않으셔도 됩니다. 괜히 미안하니깐요

크.. 빡스터 열심히 완성된건지 안된건지는 모르겠지만, 리팩터링 하신 코드 감탄하면서 봤습니다. 저는 뭔가 코드를 잘 짰다라고 하면, 빡스터가 말씀해주신 요건들을 어느정도는 지키는 코드라고 생각해요. 예를 들어 "난 객체지향적으로 정말 코드를 잘 짰어"라고 한다면, 당연히 코드가 깔끔하기도 할거고, 깔끔하니 잘 읽히기도 할거고, 변경에 유연하기도 할거라고 생각합니다. 어느 하나만 잘 되어 있다면, 그건 진짜로 좋은 코드인지 물음표가 생길 것 같아요.

물론 모든 부분에 대해서 완벽한 코드는 있기 힘들겠지만, 각 부분에서 어느정도의 트레이드 오프와 더 부족한 부분을 캐치해서 보완하는게 개발자의 역할인 것 같습니다. (종종 듣는 얘기지만 실무에서 완벽한 코드는 어렵겠지만요 ㅋㅋ...) 저는 그래서 이번에 빡스터가 리팩터링을 하면서 코드의 부족한 부분을 느껴보고 개선하는 것을 느낄 수 있었을거라고 생각해요. (아님 말구요..)

여기까지는 제 주관적인 생각이었구요 ㅋㅋㅋㅋ 그나저나 빡스터가 리팩터링 하신 코드는 제가 보았을 때 깔끔했고, 잘 읽혔고, 확장에도 유연해보였습니다. 이번 미션은 여기서 머지하겠습니다. 좋은 고민 많이 하신 것 같아 기분이 좋네요. 다음 단계에서도 마찬가지로 좋은 코드 많이 보여주세요!

feat. 시간이 null null 하시다면 dao를 사용하는 부분에서도 어떻게 잘 사용할 수 있을지 고민해보시면 좋을 것 같아요.

wonyongChoi05 · 2023-09-30T04:22:55Z

jdbc/src/main/java/org/springframework/jdbc/core/ResultSetExecutor.java

+import java.sql.ResultSet;
+import java.sql.SQLException;
+
+public interface ResultSetExecutor<T> {


좋은 방법이 있다더니.. 이제는 자바의 신이 되어버리셨네요 ㅋㅋㅋ

wonyongChoi05 · 2023-09-30T04:23:18Z

jdbc/src/main/java/org/springframework/jdbc/core/RowMapResultSetExecutor.java

+    @Override
+    public List<T> extractData(ResultSet resultSet) throws SQLException {
+        List<T> results = new ArrayList<>();
+        while (resultSet.next()) {
+            results.add(rowMapper.mapRow(resultSet));
+        }
+        return results;
+    }


👍🏻👍🏻👍🏻👍🏻

drunkenhw added 3 commits September 27, 2023 12:06

refactor: try with resource 로 변경

4c39319

refactor: result set 중복 제거

e320796

refactor: prepare statement 추상화

f033beb

wonyongChoi05 requested changes Sep 29, 2023

View reviewed changes

drunkenhw commented Sep 29, 2023

View reviewed changes

drunkenhw added 2 commits September 29, 2023 22:42

refactor: optional을 반환하도록 변경

903f8c1

refactor: 가독성을 위해 구현체 추가

b776e9c

drunkenhw added 2 commits September 30, 2023 01:56

refactor: final 제거

68f9e4f

refactor: final 제거

0b48a3f

wonyongChoi05 approved these changes Sep 30, 2023

View reviewed changes

wonyongChoi05 merged commit 6c22591 into woowacourse:drunkenhw Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ JDBC 2단계] 박스터 미션 제출합니다. #282

[ JDBC 2단계] 박스터 미션 제출합니다. #282

drunkenhw commented Sep 27, 2023

wonyongChoi05 left a comment

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

wonyongChoi05 Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw left a comment

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw Sep 29, 2023

drunkenhw commented Sep 29, 2023

wonyongChoi05 commented Sep 30, 2023 •

edited

Loading

wonyongChoi05 Sep 30, 2023

wonyongChoi05 Sep 30, 2023

		T fetchData(ResultSet resultSet) throws SQLException;

		ResultSet fetchResultSet(PreparedStatement preparedStatement) throws SQLException;

[ JDBC 2단계] 박스터 미션 제출합니다. #282

[ JDBC 2단계] 박스터 미션 제출합니다. #282

Conversation

drunkenhw commented Sep 27, 2023

wonyongChoi05 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

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

drunkenhw 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

drunkenhw commented Sep 29, 2023

wonyongChoi05 commented Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wonyongChoi05 commented Sep 30, 2023 •

edited

Loading