[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. #18

Be-poz · 2021-09-22T13:56:35Z

안녕하세요 검프, 연휴는 잘 보내셨나요 ㅎㅎ 이번주도 화이팅입니다~!
너무 구현한게 적은 느낌이라 이대로 제출해도 되나 싶은데... 리뷰 일단 요청하겠습니다!!
콜백은 2개 추가했습니다! 템플릿 메서드 패턴은 JdbcTemplate의 execute 메서드로 사용했습니다.
네이밍은 실제 JdbcTemplate의 네이밍 참고를 좀 했습니다!!

리뷰 감사합니다. 검프!!

Livenow14

안녕하세요 파즈
리뷰어 검프입니다 :)

전반적으로 요구사항을 깔끔히 구현 하셨네요!! JdbcTemplate의 간결함을 보고 깜짝 놀랐어요 :)

전체적인 피드백을 먼저 드리자면,

운영 코드 반영

css 적용

운영코드를 돌려봤는데, 페이지 상 CSS가 적용이 되지 않는거 같아요!! 이부분 반영해주세요

DB 작동안됨

현재 사이트 상 DB 반영이 되어 있지 않는것 같아요!! 이부분도 피드백 반영하며 반영해주시면 될 것 같아요 😊

간단한 피드백도 남겼어요:)

피드백을 반영하다 의문점이 있다면, 고민없이 연락 주세요 :)

Livenow14 · 2021-09-24T14:21:38Z

app/src/main/java/com/techcourse/controller/LoginController.java

 @Controller
 public class LoginController {

    private static final Logger log = LoggerFactory.getLogger(LoginController.class);
+    private final UserDao userDao = new UserDao(DataSourceConfig.getInstance());


final에 객체 생성 및 초기화를 함께한다면 static을 고려해봐도 좋을거 같아요!

여러 컨트롤러에서도 돌려쓰니깐 확실히 static이나 싱글턴으로 하는게 좋아보이네요 변경하겠습니돠

Livenow14 · 2021-09-24T14:51:19Z

app/src/main/java/com/techcourse/controller/RegisterController.java

+
+    private ModelAndView redirect(String path) {
+        return new ModelAndView(new JspView(JspView.REDIRECT_PREFIX + path));
+    }


redirect가 필요한 컨트롤러에서 메소드가 중복되네요!!
클래스로 따로 빼서 관리해도 좋을거 같아요 😊

이 메서드를 누가 가지고있으면 좋을까 생각했습니다.
처음에는 ModelAndView 클래스가 스태틱 메서드로 가지고 있게끔 변경했습니다. 갖고 있어도 그럴듯 하다 라고 생각했습니다. ModelAndView 타입을 리턴해주니까요! 그런데 내부에 JspView를 알고있다는 점이 굉장히 찝찝했습니다. import를 추가할 필요는 없었지만 이것 또한 의존이라고 생각했습니다.
그래서 최종적으로는 그냥 ViewRedirectHelper 라는 클래스를 mvc 모듈의 view/util 패키지 내부에 추가해주었습니다.

검프라면 어떤식으로 처리했을지 생각들어보고 싶습니돠!

ViewRedirectHelper 좋네요!!
JspView.REDIRECT_PREFIX도 의존할 필요가 있을까? 라는 생각에서, 나온 피드백이었어요!
저도 파즈와 마찬가지로, mvc 모듈에 유틸성 클래스를 추가했을 것 같아요 :)

Livenow14 · 2021-09-24T15:15:47Z

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

+    public final RowMapper<User> userRowMapper() {
+        return (rs -> new User(
+                rs.getLong("id"),
+                rs.getString("account"),
+                rs.getString("password"),
+                rs.getString("email")
+        ));


저는 rowNumber가 필요한 순간이 있을거라 생각해서 RowMapper의 인자에 rowNumber를 넘겨줬었어요.
파즈가 만드신 JdbcTemplate은 List을 반환 할 수 있어서 rowNumber가 필요하지 않았을 거 같아요!

List말고 다른 자료구조를 반환 해야 하는 경우가 어떤 경우가 있을까요~~? 파즈의 생각이 궁금해요!

저도 처음에 추가했었다가 현재 안써서 빼게되었는데요!
사실 스프링의 JdbcTemplate을 사용할 때에도 한 번도 사용해본적이 없어서 어떤 때에 필요한지 정확히 느껴본적이 없습니다...
다른 자료구조를 사용할 때에도 어차피 while(rs.next())로 반복문을 돌리니깐, 컬렉션 사이즈를 지정하고 생성할 때 외에는 필요할 일이 있나? 라고 느꼈습니다
어떤 때에 사용하시나요 검프는?!

만약 객체가 의존하고 있는 엔티티를 불러와야 한다면 List로만으로는 불러오기 힘들 거라 생각했어요!
저는List<Map<String, Obejct>>와 같은 타입을 반환할 수 있게 했어요. 이럴 경우 의존하는 객체의 엔티티까지 불러와 줄 수 있더라고요.
하지만, 현재 로직에선 의존하고 있지 않으니, 의존관계까지 생각할 필요는 없다고 생각해요 ㅎㅎ
List외의 다른 자료구조를 사용할때 이유가 있으면 좋겠다고 해서 남긴 피드백이었습니다

코드 없이 읽으늬 이해가 잘 안가네유 ㅜㅜ 검프 코드 한 번 보러가야겠슴다

Livenow14 · 2021-09-24T15:16:11Z

app/src/test/java/com/techcourse/dao/UserDaoTest.java

        userDao = new UserDao(DataSourceConfig.getInstance());
        final User user = new User("gugu", "password", "hkkang@woowahan.com");
        userDao.insert(user);
    }

    @Test
-    void findAll() {
+    void findAll()  {


2칸이 띄워져 있네요 :)

진짜 예리하다 이걸 어케봤지

Livenow14 · 2021-09-24T15:16:57Z

jdbc/src/main/java/nextstep/jdbc/JdbcExecutionException.java

+public class JdbcExecutionException extends RuntimeException {
+    private static final String MESSAGE = "JdbcTemplate Execute Method Error Occurred!!!";
+
+    public JdbcExecutionException() {
+        super(MESSAGE);
+    }
+}


현재와 같은 상황에선 메세지 별로 예외 클래스가 생길거 같아요.
생성자의 인자로 message를 넘겨주는 것은 어떨까요 😁

Livenow14 · 2021-09-24T15:18:55Z

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

+    private <T> T execute(String sql, Object[] args, JdbcPreparedStatementExecution<T> execution) {
+        log.debug("jdbcTemplate execute method");
+
+        try (Connection connection = dataSource.getConnection();
+             PreparedStatement pstmt = connection.prepareStatement(sql)) {
+            setStatementArguments(pstmt, args);
+            return execution.execute(pstmt);
+        } catch (SQLException e) {
+            log.error(e.getMessage());
+            throw new JdbcExecutionException();
+        }
+    }


크... 모든 쿼리가 이곳에서 이루어 지는군요!! 이부분을 보며 감탄했어요 👍👍
try with resource 활용까지..! 멋집니다 :)

Livenow14 · 2021-09-24T15:19:37Z

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

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


args가 null이 되는 경우를 검증해도 좋을 거 같아요 😊

이걸 놓쳤었는데 정말 좋은 지적이네요. 그리고 지금까지 파라미터 앞에 @NonNull 붙이면 알아서 널쳌이 되는줄 알았는데 그냥 명시하는 것 뿐이었어요! 덕분에 알게되었습니다 감사합니돠

Livenow14 · 2021-09-24T15:22:53Z

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

+public interface RowMapper<T> {
+    @Nullable


다른곳은 개행이 추가 되어있는데 여기는 붙여있네요!

처리완료~! 예리해!

Be-poz · 2021-09-25T15:11:55Z

검프 역시 꼼꼼하게 봐주셔서 감사합니다!!!!!!!!
js랑 db 안되는거는 run 돌리실 때에 gradle 말고 intellij로 돌려주세요~!!
피드백 반영했고 코멘트도 남겨놨습니다 감사합니돠

Livenow14

안녕하세요 파즈!! 2번째 pr 이네요 ㅎㅎ
리뷰 반영을 깔끔하게 잘해주셔서 코드가 더 멋져진거 같아요

js랑 db 안되는거는 run 돌리실 때에 gradle 말고 intellij로 돌려주세요~!!

A. 혹시, gralde에서도 돌아가게 해주실 수 있을까요~~? 만약 이후에 gradlew로 빌드된다면 동일한 현상이 발생할거 같아서요!!

간단한 피드백 남겼어요!! 이번 리뷰 반영하신다고 고생많으셨어요 ☺️

Livenow14 · 2021-09-26T13:08:28Z

mvc/src/main/java/nextstep/mvc/view/util/ViewRedirectHelper.java

+public class ViewRedirectHelper {
+
+    public static ModelAndView redirect(String path) {
+        return new ModelAndView(new JspView(JspView.REDIRECT_PREFIX + path));
+    }
+}


static 클래스에 private 생성자를 추가해주면 좋을 거 같아요

오 이거 진짜 놓치기 좋은 부분인데 감사합니다

Livenow14 · 2021-09-26T13:09:14Z

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

 public class JdbcTemplate {

    private static final Logger log = LoggerFactory.getLogger(JdbcTemplate.class);
+    private final DataSource dataSource;
+
+    public JdbcTemplate(DataSource dataSource) {
+        this.dataSource = dataSource;
+    }
+
+    public <T> T queryForObject(String sql, RowMapper<T> rowMapper, @NonNull Object... args) {
+        log.debug("jdbcTemplate queryForObject method - query : " + sql);
+        return execute(sql, args, pstmt -> {
+            try (ResultSet rs = pstmt.executeQuery()) {
+                T result = null;
+                while (rs.next()) {
+                    result = rowMapper.mapRow(rs);
+                }
+                return result;
+            }
+        });
+    }
+
+    public <T> List<T> query(String sql, RowMapper<T> rowMapper, @NonNull Object... args) {
+        log.debug("jdbcTemplate query method - query : " + sql);
+        return execute(sql, args, pstmt -> {
+            List<T> results = new ArrayList<>();
+            try (ResultSet rs = pstmt.executeQuery()) {
+                while (rs.next()) {
+                    results.add(rowMapper.mapRow(rs));
+                }
+                return results;
+            }
+        });
+    }
+
+    public void update(String sql, @NonNull Object... args) {
+        log.debug("jdbcTemplate update method - query : " + sql);
+        execute(sql, args, PreparedStatement::executeUpdate);
+    }
+
+    private <T> T execute(String sql, Object[] args, JdbcPreparedStatementExecution<T> execution) {
+        log.debug("jdbcTemplate execute method");
+
+        try (Connection connection = dataSource.getConnection();
+             PreparedStatement pstmt = connection.prepareStatement(sql)) {
+            setStatementArguments(pstmt, args);
+            return execution.execute(pstmt);
+        } catch (SQLException e) {
+            log.error(e.getMessage());
+            throw new JdbcExecutionException(e.getMessage());
+        }
+    }
+
+    private void setStatementArguments(PreparedStatement pstmt, Object[] args) throws SQLException {
+        if (Objects.nonNull(args)) {
+            for (int i = 0; i < args.length; i++) {
+                pstmt.setObject(i + 1, args[i]);
+            }
+        }
+    }
 }


제대로 동작하는지 테스트를 한번 해보는 것은 어떨까요? 😊

Livenow14 · 2021-09-26T13:12:02Z

jdbc/src/main/java/nextstep/jdbc/JdbcExecutionException.java

+package nextstep.jdbc;
+
+public class JdbcExecutionException extends RuntimeException {
+    private static final String MESSAGE = "JdbcTemplate Execute Method Error Occurred!!! Deatiled : ";
+
+    public JdbcExecutionException(String message) {
+        super(MESSAGE + message);
+    }
+}


사실 static 메서드를 제거를 유도한 피드백이었는데, 이것도 되게 좋은 거 같아요 😊
공통의 body를 가지는 abstract 클래스로 한단계 추상화 하면, static을 abstract 클래스로 둘 수 있을 거 같아요!
파즈의 생각은 어떠신가요?

사실 이 예외는 RuntimeException 말고 커스텀한 예외 처리하기 위해서 간단히 만들어 둔 예외였습니다!
JdbcExecution에 관련된 세부 예외들을 만들게되는 경우에는 JdbcExecutionException을 abstract class로 두고 하위 커스텀 예외들에서는 인자로 String message만 넘기게끔 만들 수 있을 것 같아요! 일단 이 클래스를 하위라고 치부하고 위쪾에 abstract class를 하나 만들어 두겠습니다!

그런데 검프네는 프로젝트 진행 시에 커스텀 익셉션들 안에 상수 필드로 메세지 따로 안가지고 있나요?!

저희 팀에서는 message를 생성자의 인자로만 가지고 있어요!! 생성시에 상위 클래스인 RuntimeException에서 message가 초기화 되구요.
서비스 동작 시 공통적으로 발생 할 수 있는 예외들을 클래스 타입으로 명시했고 (NotFoundException, DuplicateException 등)
상황에 따라 메세지로 에외를 표시했어요.
NotFoundException -> CustomException -> RuntimeException 이런 구조로 되어 있습니다!
(이런 상황에선 이후 enum 필드로 메세지를 관리해야 할 거 같긴 하네요 😊 )

Livenow14 · 2021-09-26T13:12:43Z

app/src/main/java/com/techcourse/support/jdbc/init/DatabasePopulatorUtils.java

                if (statement != null) {
                    statement.close();
                }
-            } catch (SQLException ignored) {}
+            } catch (SQLException ignored) {
+            }

            try {
                if (connection != null) {
                    connection.close();
                }
-            } catch (SQLException ignored) {}
+            } catch (SQLException ignored) {
+            }
        }
    }

-    private DatabasePopulatorUtils() {}
+    private DatabasePopulatorUtils() {
+    }
 }


여기에도 try with resource를 적용해보는것은 어떨까요 ?😁

sonarqubecloud · 2021-09-26T16:42:43Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

Be-poz · 2021-09-26T16:52:42Z

검프 안녕하세용 피드백 적용해서 다시 제출합니다!!
메인 코멘트에서 A. 혹시, gralde에서도 돌아가게 해주실 수 있을까요~~? 만약 이후에 gradlew로 빌드된다면 동일한 현상이 발생할거 같아서요!! 이 부분은 반영하지 않았습니다!
미션에서 의도한 부분이라서요~!

JwpApplication 클래스의 메서드입니다. 톰캣한테 base path를 알려주는 부분입니다.

app 모듈의 build.gradle 입니다. idea 스코프를 따로 추가해줬습니다. idea로 돌릴 시에 outputDir를 "webapp/WEB-INF/classes"로 들어가게끔 해주었습니다. 서블릿 디렉토리 표준으로 맞춰줘야 하는데 인텔리제이로 빌드할 때 맞춰주도록 되어있습니다. 궁금해서 구구한테 물어보니깐 다들 ide로 보통 돌리니깐 미션의 용이함을 위해서 설정해둔거라고 하셨습니다! 그리고 어떤 path 로 바꿔야 할지 잘 모르기도 하구요. .. 어쨋든 요거는 기본 미션에서 주어지는 상황이니깐 패쓰하겠습니다! 🙄

Livenow14

안녕하세요 파즈!
처음부터 잘 짜여있던 로직이라 파즈 코드를 통해 많이 배울 수 있었어요!

피드백 반영을 하시며 질문 해주셨던 부분들이 인상 깊었습니다. 저도 피드백을 했던 부분들에 다시한번 고민을 해보는 값진 시간이었어요. 정답을 찾아가는 모습이 멋집니다!
(저는 미션을 진행하며 intellij말고 gradle로 build 해서 많이 해맸던 기억이...ㅎㅎㅎ )

더 성장하리라 믿습니다
피드백 반영하신다고 수고 정말 많으셨어요 😀😀

Be-poz added 11 commits September 21, 2021 13:26

refactor: DataSource import 시키기

81eeff7

refactor: UserDao 나머지 함수들 구현

1579809

refactor: code formatting

262ea33

feat: JdbcTemplate query, queryForObject 메서드 생성

917e76c

refactor: UserDao JdbcTemplate 이용으로 변경

b25341a

refactor: 테스트 돌아가게끔 JdbcTemplate 수정

6bb73b9

refactor: Controller Inmemory 에서 UserDao 사용으로 변경

e0f8819

refactor: jdbctemplate 템플릿 메서드 패턴 적용

d4501a6

refactor: userRowMapper 메서드 분리

ccb6659

refactor: exception 제거

3d2249b

refactor: 오류 수정 및 try-resource 적용

b535d6c

Be-poz requested a review from Livenow14 September 22, 2021 13:56

Livenow14 suggested changes Sep 24, 2021

View reviewed changes

Be-poz added 8 commits September 25, 2021 13:08

refactor: UserDao static으로 변경

91793fb

refactor: UserDao log 추가

a90dac8

refactor: ModelAndView class redirect render method included

163bdb0

feat: ViewRedirectHelper 클래스 추가

5713aaa

refactor: JdbcExecutionException 메세지 인자로 받게끔 변경

96ec372

refactor: code reformat

3f59702

refactor: 개행 추가

339770c

refactor: Object 가변인지 널체크

e58ebc7

Livenow14 suggested changes Sep 26, 2021

View reviewed changes

Be-poz changed the title ~~[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다.~~ [JDBC 라이브러리 구현하기 - 1단계] 파즈(강승윤) 미션 제출합니다. Sep 26, 2021

Be-poz changed the title ~~[JDBC 라이브러리 구현하기 - 1단계] 파즈(강승윤) 미션 제출합니다.~~ [JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. Sep 26, 2021

Be-poz added 4 commits September 27, 2021 00:05

refactor: ViewRedirectHelper private ctor added

06894e2

refactor: JdbcException abstract class 추가

4565c33

refactor: DatabasePopulatorUtils try-resource로 변경

bb4cb45

feat: JdbcTemplate Test 추가

b2fdda3

Livenow14 approved these changes Sep 26, 2021

View reviewed changes

Livenow14 merged commit 8a9cf68 into woowacourse:be-poz Sep 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. #18

[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. #18

Be-poz commented Sep 22, 2021

Livenow14 left a comment

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 26, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 26, 2021

Be-poz Sep 26, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 24, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Livenow14 Sep 24, 2021

Be-poz Sep 25, 2021

Be-poz commented Sep 25, 2021

Livenow14 left a comment

Livenow14 Sep 26, 2021

Be-poz Sep 26, 2021

Livenow14 Sep 26, 2021

Livenow14 Sep 26, 2021

Be-poz Sep 26, 2021

Livenow14 Sep 26, 2021 •

edited

Loading

Livenow14 Sep 26, 2021

sonarqubecloud bot commented Sep 26, 2021

Be-poz commented Sep 26, 2021

Livenow14 left a comment •

edited

Loading

[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. #18

[JDBC 라이브러리 구현하기 - 1, 2단계] 파즈(강승윤) 미션 제출합니다. #18

Conversation

Be-poz commented Sep 22, 2021

Livenow14 left a comment

Choose a reason for hiding this comment

운영 코드 반영

css 적용

DB 작동안됨

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

Be-poz commented Sep 25, 2021

Livenow14 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

Livenow14 Sep 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Sep 26, 2021

Be-poz commented Sep 26, 2021

Livenow14 left a comment • edited Loading

Choose a reason for hiding this comment

Livenow14 Sep 26, 2021 •

edited

Loading

Livenow14 left a comment •

edited

Loading