[JDBC 라이브러리 구현] 리오(오영택) 1단계 미션 제출합니다. #307

Jaeyoung22 · 2023-09-28T11:23:51Z

안녕하세요 바론! 연휴 잘 보내고 계신가요?!
JDBC 라이브러리 구현해서 보냅니다!
리뷰 잘 부탁드려요~!

somsom13

안녕하세요 리오~!~!!! 바론입니다 🙇‍♀️

구현을 엄청나게 잘 하셔서 어쩌다보니 엄지척 남긴 코멘트가 전체 코멘트의 절반이 된 것 같네요... 머쓱 👍 👍 👍

리오가 확인해주셨으면 하는 코멘트가 조금 있어서 Request Change로 남겨둡니다..!! 편하게 확인하시구 다시 리뷰 요청 주시면 감사하겠습니다 ㅎㅎ

1단계 고생하셨어요~!!!!! 👍

jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java

somsom13 · 2023-09-29T08:32:08Z

jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java

+                .isThrownBy(() -> this.template.update(sql))
+                .withCause(sqlException);
+        verify(this.preparedStatement).close();
+        verify(this.connection, atLeastOnce()).close();


다른 테스트에서는 connection을 verify(this.connection).close() 로 검증하셨는데, 예외 테스트들에서는 atLeastOnce 라는 조건을 추가하셨군요!!

예외 테스트에는 한 번 이상 호출 이라는 조건을 추가하신 이유가 궁금해요~!~! 😄

좋은 질문이네요!!

정상적인 상황에서는 명시적으로 한 번만 호출되었음을 가정하기 때문에 atLeastOnce()를 붙이지 않았어요.

그러나 예외의 상황에서는 SQLException이 몇 번 발생됐는지를 확인할 수도 없고 확인할 필요도 없을 것 같아요!
그래서 어쨌든 무조건 커넥션을 닫는다는 뜻을 추가하기 위해서 추가했어요!

혹시 설명이 부족해서 이해가 안되시면 DM으로 언제든지 질문해주세요!

오호 이해 너무 잘 됐습니다
감사합니다 ㅎㅎ 테스트 장인이시네요...

somsom13 · 2023-09-29T08:39:58Z

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

+    @FunctionalInterface
+    public interface RowMapper<T> {
+        T run(ResultSet resultSet) throws SQLException;
+    }


RowMapper 인터페이스를 jdbcTemplate 내부에 두셨군요!!
확실히 JdbcTemplate을 통해서 쿼리 작업을 수행하도록 하는 지금 미션에서는 RowMapper가 JdbcTemplate 내에 있어도 좋을 것 같아요 👍 👍

그런데 만약 ADao에서는 JdbcTemplate을 사용하고 BDao에서는 NamedParameterJdbcTemplate를 사용하는 것 처럼 여러 템플릿을 함께 사용하게 된다면, BDao에서는 JdbcTemplate이 필요하지 않음에도 JdbcTemplate.RowMapper를 사용해야 할 것 같아요.

이런 상황에 대해서 리오는 어떻게 생각하시는지 궁금해요!!

지금 미션에서는 생기지 않을 상황이라 괜한 고민인가..? 싶지만 리오의 의견이 궁금해서 코멘트 남깁니다!!

넵ㅎㅎ 저도 고민을 많이 했고 리뷰를 주실 것 같았는데 역시나 주셨군요 ㅎㅎ...
저도 외부로 분리할까 많이 고민을 했는데요, 저희가 NamedParameterJdbcTemplate을 구현해야하는 상황 처럼
확장성을 고려해야할 때 분리를 하는 것이 맞지 않는가... 싶어서 우선은 내부에 두었습니다!
그런데 계속 찜찜해서 2단계 때 분리하려구요...ㅎㅎ 커멘트 감사합니다!

저희가 NamedParameterJdbcTemplate을 구현해야하는 상황 처럼 확장성을 고려해야할 때 분리를 하는 것이 맞지 않는가...

저는 이것도 너무 좋은 생각이라고 생각합니다 ㅎㅎ 리오의 결정에 따라 분리하시면 좋을 것 같아요!! 😄

somsom13 · 2023-09-29T08:41:32Z

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

-    public UserDao(final DataSource dataSource) {
-        this.dataSource = dataSource;
+    private final JdbcTemplate jdbcTemplate;
+    private final JdbcTemplate.RowMapper<User> rowMapper = resultSet -> {


RowMapper를 static으로 선언하지 않은 이유가 궁금해요~!

UserDao에서만 쓰이도록 타입까지 명시를 해놔서요, UserDao가 생성될 때 한번만 호출되면 되지 않나 생각을 했는데 생각을 해보니 현재 Bean으로 등록이 되지 않은 클래스였네요...
static을 붙이지 않아서 UserDao가 생길 때마다 계속 생성되야하니까 메모리 누수가 있겠네요ㅠㅠㅠㅠ
커멘트 감사합니다!

somsom13 · 2023-09-29T08:46:44Z

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

-                }
-            } catch (SQLException ignored) {}
+    public void insert(User user) {
+        String sql = "insert into users (account, password, email) values (?, ?, ?)";


사소한 내용이지만 일부 메서드는 파라미터나 지역 변수에 final 키워드가 붙어있고, 그렇지 않은 메서드들도 있는 것 같아요!

혹시 final 키워드를 붙이는 리오만의 기준이 있으신가요?.?

앗 저는 final을 붙이지 않는 것을 선호합니다..! 있다가 없다가 하면 좀 헷갈리더라구요.. ㅎㅎ..
보이는 final들을 다 지운다고 지워봤는데 남아있는 것이 있었군요..! 싹 다 지우겠습니다!

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

somsom13 · 2023-09-29T08:50:22Z

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

+    public <T> T queryForObject(String sql, RowMapper<T> rowMapper, Object... values) {
+        return query(sql, rowMapper, values).get(0);
+    }


query의 결과가 비어있는 리스트라면 get(0) 을 했을 때 어떤 결과가 나오나요?.?

천재?

당장 고치겠습니다 감사합니다!

somsom13

안녕하세요 리오~~~

리뷰 반영 엄청 빠르게 해주셨네요 👍 👍 (하지만 저의 리뷰는 그렇게 빠르지 못했죠 죄송합니다..)
리뷰 반영도 완벽하게 해주셨고 1단계 요구사항도 아주 깔끔하게 만족해주신 것 같아서 approve 하겠습니다

고생 많으셨습니다!!!!! 즐거운 추석 연휴 보내시고 2단계에서 뵐게요~!~!!

somsom13 · 2023-09-29T14:53:03Z

jdbc/src/test/java/nextstep/jdbc/JdbcTemplateTest.java

+                .isThrownBy(() -> this.template.update(sql))
+                .withCause(sqlException);
+        verify(this.preparedStatement).close();
+        verify(this.connection, atLeastOnce()).close();


오호 이해 너무 잘 됐습니다
감사합니다 ㅎㅎ 테스트 장인이시네요...

somsom13 · 2023-09-29T14:55:05Z

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

+    @FunctionalInterface
+    public interface RowMapper<T> {
+        T run(ResultSet resultSet) throws SQLException;
+    }


저희가 NamedParameterJdbcTemplate을 구현해야하는 상황 처럼 확장성을 고려해야할 때 분리를 하는 것이 맞지 않는가...

저는 이것도 너무 좋은 생각이라고 생각합니다 ㅎㅎ 리오의 결정에 따라 분리하시면 좋을 것 같아요!! 😄

somsom13 · 2023-09-29T14:55:46Z

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

+        List<T> result = query(sql, rowMapper, values);
+        if (result.isEmpty()) {
+            return null;
+        }
+        return result.get(0);


somsom13 · 2023-09-29T15:04:29Z

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

@@ -57,15 +58,22 @@ public <T> List<T> query(String sql, RowMapper<T> rowMapper, Object... values) {
                result.add(rowMapper.run(resultSet));
            }

+            resultSet.close();


resultSet.close()를 추가해주셨네용! 저도 이 부분에 대해서 고민을 많이 했었는데, ResultSet의 close메서드를 보니

Note: A ResultSet object is automatically closed by the Statement object that generated it when that Statement object is closed, re-executed, or is used to retrieve the next result from a sequence of multiple results.

라고 적혀있더라구요!

Statement의 close 메서드에도 다음과 같이 적혀있었어요.

Note:When a Statement object is closed, its current ResultSet object, if one exists, is also closed.

혹시 몰라 UserDaoTest를 통해 디버깅도 한 번 해봤는데, JdbcResultSet에서 closeInternal이 호출되는 걸 확인할 수 있었어요.

그래서 저는 resultSet.close를 별도로 호출하지 않기로 결정했는데, 리오의 의견도 나중에 알려주시면 좋을 것 같아요 ㅎㅎ

혹시 제가 잘못된 정보를 알고 있는거라면 정정해주시면 너무 감사하겠습니다 🙇‍♀️

Jaeyoung22 added 4 commits September 28, 2023 19:43

test: 학습 테스트 작성

9440a38

refactor: 중복 코드 제거

6f0ec14

fix: 에러 수정

c57ca8b

test: JdbcTemplate 테스트 작성

f8bca32

somsom13 requested changes Sep 29, 2023

View reviewed changes

Jaeyoung22 added 3 commits September 29, 2023 18:45

refactor: RowMapper static으로 설정

3daf86f

refactor: final 키워드 제거

23226fa

fix: queryForObject()의 결과물이 없을 경우 null을 반환하도록 수정

f5de85e

somsom13 approved these changes Sep 29, 2023

View reviewed changes

somsom13 merged commit e132f22 into woowacourse:jaeyoung22 Sep 29, 2023
1 check failed

hum02 mentioned this pull request Oct 5, 2023

[JDBC 라이브러리 구현하기 - 2단계] 두둠(최영훈) 미션 제출합니다. #406

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JDBC 라이브러리 구현] 리오(오영택) 1단계 미션 제출합니다. #307

[JDBC 라이브러리 구현] 리오(오영택) 1단계 미션 제출합니다. #307

Jaeyoung22 commented Sep 28, 2023

somsom13 left a comment

somsom13 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

somsom13 Sep 29, 2023

somsom13 Sep 29, 2023 •

edited

Loading

Jaeyoung22 Sep 29, 2023

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

somsom13 Sep 29, 2023

Jaeyoung22 Sep 29, 2023

somsom13 Sep 29, 2023 •

edited

Loading

Jaeyoung22 Sep 29, 2023

somsom13 left a comment

somsom13 Sep 29, 2023

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023

somsom13 Sep 29, 2023 •

edited

Loading

[JDBC 라이브러리 구현] 리오(오영택) 1단계 미션 제출합니다. #307

[JDBC 라이브러리 구현] 리오(오영택) 1단계 미션 제출합니다. #307

Conversation

Jaeyoung22 commented Sep 28, 2023

somsom13 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

somsom13 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Sep 29, 2023 • 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

somsom13 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023 •

edited

Loading

somsom13 Sep 29, 2023 •

edited

Loading