[JDBC 라이브러리 구현하기 - 1, 2단계] 현구막(최현구) 미션 제출합니다. #34

Hyeon9mak · 2021-09-26T01:45:49Z

하이 티끌모아 로키 산맥?! 연휴 잘 보내셨나요? 저는 신나게 먹고 자고 놀았습니다!!

이번 미션 진행하면서 콜백/템플릿 패턴을 이해하는 것 때문에 죽을 뻔했어요...
'저걸? 왜? 저렇게?' 하고 반나절을 고민만 하다가
먼저 깨달음을 얻은 라이언이 귀중한 힌트를 공유해주셔서 영감을 얻고 사용 할 수 있었어요!!!!! 👍

SimpleJdbcInsert

JdbcTemplate을 사용할 적에 ID를 바로 반환해주는 SimpleJdbcInsert를 굉장히 좋아했어요!!!
그래서 미션을 시작하자마자 'SimpleJdbcInsert는 꼭 구현해야지!' 라고 생각하고 미션에 임했는데,
실제로 구현해보니 굉장히 지저분하게 구현되더라구요... 😭
최대한 깔끔하게 정리한다고 해봤는데, 여전히 잘 모르겠네요... 마구마구(야구 아님ㅋ) 피드백 주세요!!

모듈들을 이식했어요

이전 HTTP, MVC 미션을 진행하면서 구현했던 모듈들을 그대로 사용해보고 싶어서 이식했어요!
그래서 file changed가 쵸큼 많습니다 ㅎㅎㅎㅎㅎㅎ;;;;

인스턴스 싱글톤 유지를 위한 ComponentContainer

이식해 온 모듈들 중에 가장 특이하게 보이는 녀석이 ComponentContainer일거에요!
Service와 Repository과 같은 컴포넌트 인스턴스들을 싱글톤으로 관리하고 싶다는 욕심이 생겼는데,
DispatcherServlet 초기화 중 리플렉션을 통해 핸들러(Controller)의 인스턴스를 생성하는 시점에는 필드멤버(컴포넌트)를 생성하기 애매하더라구요...
그래서 ComponentContainer라는 객체를 만들어서 DispatcherServlet 초기화 동작 전에 먼저 @controller, @service, @repository 어노테이션이 붙은 객체들을 찾아내서 인스턴스화 시키고 꺼내서 사용하도록 해보았어요!!!

@Inject 어노테이션을 사용하는 방법 또한 Controller-Service-Repository 계층 구조에서는 싱글톤 유지가 불가능 할 것 같아서 현재처럼 구현해보았습니다!!

트랜잭션 관리는 어떻게...?

미션 사이트에 게시되어있는 트랜잭션 관리를 어떻게 진행해야하는 건지 감이 잡히지 않더라구요...?
크루들의 PR을 훔쳐보던 중 👀 현재 저희가 구현한 '무언가'에 의해 별도로 트랜잭션 관리를 해주지 않아도 커밋, 롤백이 기대한대로 동작한다는 이야기를 접했어요!
이게 무엇인지 로키와 이야기를 나눠보면 좋을거 같아서 README.md의 TODO 리스트에 체크는 해두지 않았습니다!!

리뷰 잘 부탁드릴게요오오옹 🙇‍♂️ 🙇‍♂️ 🙇‍♂️ 🙇‍♂️ 🗻 🗻 ⛰️

kang-hyungu

구막 간단한 피드백 남겼으니 참고하세요

kang-hyungu · 2021-09-27T07:21:34Z

jdbc/src/main/java/nextstep/jdbc/SimpleJdbcInsert.java

+import nextstep.jdbc.exception.DataAccessException;
+import nextstep.jdbc.exception.SimpleJdbcException;
+
+public class SimpleJdbcInsert {


kang-hyungu · 2021-09-27T07:22:42Z

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

+            return preparedStatementCallback.call(preparedStatement);
+        } catch (SQLException e) {
+            LOG.error(e.getMessage(), e);
+            throw new RuntimeException(e);


커스텀 예외로 바꿔주는게 좋겠네요

헉.. 커스텀 예외를 구현하기 전에 임시로 해두고 새까맣게 잊었네요.. 😢

Rok93

구막의 화이팅 받은게 엇그제 같은데 엄청 오랜기간 못뵙고있네요 🥲
83개의 Files changed에 살~짝 당황했지만... 당황하지않고... 아니 그냥 당황한채로 😆 이것 저것 코멘트 달아봤어요 😃

저도 스프링을 사용하다보니 컴포넌트들을 싱글톤으로 관리하고 싶다는 생각을 잠시 가졌었는데 구막이 인스턴스 싱글톤 유지를 위한 ComponentContainer를 만든걸 보고 매우 흥미롭게 봤어요 😃

구막도 저랑 동일한 생각을 하고계실지는 모르겠지만 ComponentContainer에 Repository Component를 주입할 때 부분을 조금 더 바꿀 수 없을까? 라는 생각이 많이 들었어요 🤔
나름 이것저것 끄적여봤는데 성공하지는 못했네요 혹시나 이 방식에 관심이 있다면 이어서 진행을 해보시면 좋을 것 같아요 😃

미션 요구사항을 충분히 충족시켜주신 것 같아서 당장 머지👋🏻해도 전~~혀 문제없지만 이것 저것 더 이야기해보고 싶은 부분들이 많으실 것 같다는 추측(?)에 의해 일단 Request Changes로 드렸습니다.
또한 저도 SimpleInsert 구현에 대해서는 고민을 전혀 안해본터라 이 부분에 대한 코멘트는 전혀 못드려서 조금 구조를 뜯어본 뒤에 다시 돌아오겠습니다 😉 그 때, 이야기하면 더 생산적인🏭 이야기들을 할 수 있지 않을까 싶어요 👀

트랜잭션은 오늘 (아니 어제...?라고 해야겠네요) 구구가 트랜잭션 부분은 구현하지 않아도 된다고 하셨으니 구현은 꼭 하실 필요는 없지만(?) 혹시나 욕심나신다면 진행해보셔도 좋을 것 같아요.
아마 레벨1 체스 미션 때, 트랜잭션을 구현해볼까? 라는 생각을 잠시 해본 적이 있어서 고민해본 적이 있는데

con.setAutoCommit(false);
// callBack...
con.commit(); // 정상적으로 수행 
con.rollback(); //예외 발생 시

위와 같이 구현해보면 트랜잭션 구현도 가능하지 않을까라는 뇌피셜 살짝 투척해봅니다! 😉
코멘트 읽어보시고 반영해주실 수 있는 혹은 코멘트에 대한 의견들 자유롭게 남겨주세요 🙏

Rok93 · 2021-09-28T13:23:36Z

mvc/src/main/java/nextstep/mvc/exception/ComponentContainerException.java

@@ -0,0 +1,5 @@
+package nextstep.mvc.exception;
+
+public class ComponentContainerException extends RuntimeException {


전체적으로 커스텀 예외들 잘 만들어주신 것 같아요~ 👍
간단하게라도 예외 메시지도 추가해보면 어떨까요? 🤔

최대한 예외 메세지에 예외의 원인(근거?)를 담도록 바꿔보겠습니다!!

Rok93 · 2021-09-28T13:36:12Z

mvc/src/main/java/nextstep/mvc/view/JsonView.java

+    private Object toSingleData(Map<String, ?> model) {
+        return model.values()
+            .stream()
+            .findFirst()
+            .orElseThrow(IllegalStateException::new);


toSingleData은 data의 크기가 1이하인 경우에 해당 메서드가 실행될 것 같아요 🤔
값이 없는 경우에는 IllegalStateException 예외가 발생하도록 처리하셨는데 반환하는 JSON 데이터가 없다면 무조건 예외인게 맞는지 잘 모르겠어요 🤔 예를들면 저희는 204와 같이 No Content와 같은 응답도 존재할 수 있다고 생각해서 반환 값이 없는 경우도 충분히 존재한다고 생각해요 😃

이 부분은 제가 잘 못 생각하는 것일 수도 있는데... 현구막은 어떻게 생각하시나요 ?? 🙄

또한 추가적으로 데이터가 1개 혹은 없는 경우라서 (멀티쓰레드, 단일쓰레드 상황에 따른 사이드 이펙트 혹은 성능상의 이슈 등의 문제가 아니더라도... ) findFirst()가 아닌 findAny()를 쓰셔도 좋을 것 같아요 😃

어... 저도 로키가 해주신 말씀을 보고 나니 "그런가?!" 하면서 많은 생각이 드네요!!!!
여기서 거의 30분을 고민한거 같아요!!!!!!!!!!!!!!!!!! 👍 👍 👍 👍 👍

고민 끝에 내린 결론으로는 API도 데이터베이스랑 비슷하다인데.... 괜찮을지 확신은 안생기지만!!

복수의 데이터를 조회할 때 조회된 데이터가 없는 경우엔 정상 반환

단수의 데이터를 조회할 때 조회된 데이터가 없으면 예외상황

JsonView.toJsonData() 내부에서 분기를 할 때 크기가 1 초과, 1 이하인 경우가 아니라
1일 때, 1이 아닐 때로 변경해봐야겠네요!!

MVC 미션 1단계에서는 열심히 공부해놓고, 이번 미션에서는 그대로 사용해버렸네요 헤헤... 😅

Rok93 · 2021-09-28T13:49:27Z

mvc/src/main/java/nextstep/mvc/view/JsonView.java

+    @Override
+    public String getViewName() {
+        throw new IllegalStateException();


View 인터페이스에 getViewName 메서드가 있었던가...? 하고 mvc 레포지토리를 다시 한번 확인하고 왔네요 👀
구막이 인터페이스에 추가하신거였군요 😃

저는 개인적인 생각으로 viewName은 공통적으로 가지는 요소가 아니라고 생각해서 View 인터페이스의 추상 메서드로 가질 필요가 있을까 생각해요 🤔 이미 render() 메서드를 통해서 View 인터페이스의 하위요소들이 충분히 추상화 됐다고 생각하는데 현구막은 어떻게 생각하시나요~? 🤗

아! 이거! PR 본문에 질문을 포함한다는 걸 깜빡했어요!!

이전에 MVC미션에서 LegacyMVC 제거를 진행하면서 Controller 테스트코드를 작성했는데요,
이 과정에서 Controller들이 반환하는 ModelAndView 를 어떻게 테스트할지 도저히 떠오르지 않아서 ModelAndView에 getViewName라는 getter 메서드를 만들어버렸어요..

프로덕션 코드에 사용되지 않고, 오로지 테스트만을 위해 존재하는 코드가 만들어진 셈인데, 이게 올바른 행동인지 여전히 모르겠어요..
개인적으로 굉장히 지양하는 형태인데, '취향을 고려하는 것보다 테스트를 우선해야하는거 아닐까?' 라는 생각도 드네요...
라이언도 지양하긴 하지만, 테스트도 프로덕션의 일부라 필요하다면 구현해야겠다 라고 답변을 주셨는데
로키는 어떻게 생각하시는지 궁금해요!!!

저는 테스트를 위해 생성자 혹은 getter를 만들어도 괜찮다고 생각하는 쪽입니다.
테스트 코드 작성을 위해서 필요하다면 크게 고민하지 않고 만들 것 같습니다. (레벨1 때, 포비도 테스트를 위한 오버로딩을 하셨었죠 😃)
하지만 테스트만을 위한 별개의 메서드(getter를 제외한)가 생성되는 것은 무조건 피할 것 같습니다. 🤔

예외적으로 getter의 경우에는 사실 테스트하고 싶은 value는 객체의 필드 값(= view)이에요.
이럴 때는 assertJ 라이브러리 기능 중에서 필드 값을 추출하는 extracting(필드 이름)의 방식으로 getter 생성을 피할 수도 있을 것 같아요. 물론 기술 집약적인 방식이기에 이 방식을 꺼려하시는 분들도 많을 것 같습니다만 테스트 코드만을 위해서 프로덕션 코드를 건드리는 것이 싫다면 이런식으로 우회가 가능할 수 있다. 라는 의견이에요.

전 테스트가 프로덕션의 일부라고 생각하지는 않지만 라이언과 비슷한 생각을 가지고 있는 것 같습니다.
하지만 리팩토링의 대상, 유지보수의 대상 이라고는 생각해요 😃

Rok93 · 2021-09-28T13:52:00Z

mvc/src/main/java/nextstep/mvc/view/JspView.java

-    private static final Logger log = LoggerFactory.getLogger(JspView.class);
-
-    public static final String REDIRECT_PREFIX = "redirect:";
+    private static final Logger LOG = LoggerFactory.getLogger(JspView.class);


ㅋㅋㅋㅋㅋㅋㅋㅋ 오늘 구구 수업을 들은 뒤였다면 안바꾸셨을지도...? 😆

후... 바꾸면서 내심 뿌듯해 했는데..

Rok93 · 2021-09-28T15:35:27Z

mvc/src/main/java/nextstep/web/ComponentContainer.java

+    public static void initializeComponents(DataSource dataSource, Object... basePackage) throws Exception {
+        Reflections reflections = new Reflections(basePackage);
+        initializeRepositories(reflections.getTypesAnnotatedWith(Repository.class), dataSource);
+        initializeServices(reflections.getTypesAnnotatedWith(Service.class));
+        initializeControllers(reflections.getTypesAnnotatedWith(Controller.class));
+    }
+
+    private static void initializeRepositories(Set<Class<?>> repositoryTypes, DataSource dataSource) throws Exception {
+        for (Class<?> repository : repositoryTypes) {
+            Object instance = repository.getConstructor(DataSource.class).newInstance(dataSource);
+            COMPONENTS.put(repository, instance);
+        }
+    }
+
+    private static void initializeServices(Set<Class<?>> serviceTypes) throws Exception {
+        for (Class<?> service : serviceTypes) {
+            Class<?>[] fieldTypes = getFieldTypes(service);
+            Object[] fieldObjects = getFieldObjects(fieldTypes);
+
+            Object serviceInstance = service.getConstructor(fieldTypes).newInstance(fieldObjects);
+            COMPONENTS.put(service, serviceInstance);
+        }
+    }
+
+    private static void initializeControllers(Set<Class<?>> controllerTypes) throws Exception {
+        for (Class<?> controller : controllerTypes) {
+            Class<?>[] fieldTypes = getFieldTypes(controller);
+            Object[] fieldObjects = getFieldObjects(fieldTypes);
+
+            Object controllerInstance = controller.getConstructor(fieldTypes).newInstance(fieldObjects);
+            COMPONENTS.put(controller, controllerInstance);
+        }
+    }


제가 잘못한 것일 수도 있는데, Service와 Controll는 위와 같이 하나의 메서드로 변경할 수 있을 것 같아요 😃 (~~메서드명이나 변수명 등은 대충지은거니 참고 정도만 해주세요...~~)

Suggested change

public static void initializeComponents(DataSource dataSource, Object... basePackage) throws Exception {

Reflections reflections = new Reflections(basePackage);

initializeRepositories(reflections.getTypesAnnotatedWith(Repository.class), dataSource);

initializeServices(reflections.getTypesAnnotatedWith(Service.class));

initializeControllers(reflections.getTypesAnnotatedWith(Controller.class));

}

private static void initializeRepositories(Set<Class<?>> repositoryTypes, DataSource dataSource) throws Exception {

for (Class<?> repository : repositoryTypes) {

Object instance = repository.getConstructor(DataSource.class).newInstance(dataSource);

COMPONENTS.put(repository, instance);

}

}

private static void initializeServices(Set<Class<?>> serviceTypes) throws Exception {

for (Class<?> service : serviceTypes) {

Class<?>[] fieldTypes = getFieldTypes(service);

Object[] fieldObjects = getFieldObjects(fieldTypes);

Object serviceInstance = service.getConstructor(fieldTypes).newInstance(fieldObjects);

COMPONENTS.put(service, serviceInstance);

}

}

private static void initializeControllers(Set<Class<?>> controllerTypes) throws Exception {

for (Class<?> controller : controllerTypes) {

Class<?>[] fieldTypes = getFieldTypes(controller);

Object[] fieldObjects = getFieldObjects(fieldTypes);

Object controllerInstance = controller.getConstructor(fieldTypes).newInstance(fieldObjects);

COMPONENTS.put(controller, controllerInstance);

}

}

public static void initializeComponents(DataSource dataSource, Object... basePackage) throws Exception {

Reflections reflections = new Reflections(basePackage);

initializeRepositories(reflections.getTypesAnnotatedWith(Repository.class), dataSource);

List<Class<? extends Annotation>> types = List.of(Service.class, Controller.class);

for (Class<? extends Annotation> type : types) {

initialize(reflections.getTypesAnnotatedWith(type));

}

}

private static void initializeRepositories(Set<Class<?>> repositoryTypes, DataSource dataSource) throws Exception {

for (Class<?> repository : repositoryTypes) {

Object instance = repository.getConstructor(DataSource.class).newInstance(dataSource);

COMPONENTS.put(repository, instance);

}

}

private static void initialize(Set<Class<?>> classTypes) throws Exception {

for (Class<?> repository : classTypes) {

Class<?>[] fieldTypes = getFieldTypes(repository);

Object[] fieldObjects = getFieldObjects(fieldTypes);

Object instance = repository.getConstructor(fieldTypes).newInstance(fieldObjects);

COMPONENTS.put(repository, instance);

}

}

개인적인 욕심으로는 Repository 또한 List에 포함시켜서 코드 두 줄이라도 더 줄여보고 싶은데... DataSource가 매우 애매하네요... 🤔

개인적인 생각으로는 annotation 패키지에 @Bean 을 추가하고 ComponentContainer#initializeComponents 메서드에 @Bean 애너테이션을 붙여서 DataSource 빈들을 미리 COMPONENTS에 추가하면 아래의 사진과 같이 변경해볼 수 있지 않을까요? 😃

이것저것 좀 찾아보면서 해봤는데 생각보다 잘 안돼서 뒤는... 현구막에게 맡기는 걸로 🙄

Controller와 Service를 생성하는 메서드가 중복이 됨에도 현상을 유지한 가장 큰 이유는 가독성 때문이었어요!
말씀해주신대로 Repositories는 DataSource의 존재로 Controller, Service와 조금 다른 로직을 수행하게 되는데
통일성을 유지 시키고 싶었어요.

사실 ComponentContainer 자체가 '모든 걸 해결하려 했지만 결국 하나만 해결한 객체'인거 같아요. 최초 기획은 컴포넌트 뿐만 아니라 생성자에 필요한 다른 파라미터를 모두 주입하도록 해주고 싶었지만, 결국 컴포넌트만 주입받는 형태로 끝나요.

"리플렉션으로 생성자 정보를 얻어서 핸들링 하는게 왜 이렇게 어렵지?" 라고 고민을 많이 했는데, JPA도 그게 어려워서 파라미터가 없는 기본 생성자에 별도로 매핑을 한다고 하더라구요. 생성자를 통해서 주입 받는 형태를 유지하다보니 그나마 이정도로 코드가 정돈 됐던거 같아요. 타협을 해버렸습니다. 🥲

DataSource를 컴포넌트로 등록하는 방법까진 생각 못했어요!!!! 👍 🥇 💯 💯

그런데 코드가 정리되는 모습에서는 Repository가 Controller, Service와 다르게 수행되는 것에서
DataSource가 Controller, Service, Repository와 다르게 수행되는 것으로 비슷한 상황에 놓이는거 같아서...!! 히히..

또, 현재 코드상에 DataSource가 파라미터로 전달되기 전에 한 차례 초기화(DatabasePopulatorUtils.execute(DataSourceConfig.getInstance())), 하는 과정도 포함되어 있어요!
컴포넌트 컨테이너 내부에서 DataSource의 존재를 인식하고 초기화를 해주는게 찜찜한거 같아
DataSource는 현재처럼 파라미터로 전달 받는 상태를 유지를 해보겠습니다!!
(고생해주셨는데 흑흑.. 🙇‍♂️ 🙏 )

~~대신 Controller, Service는 명백한 코드 중복이므로 제안해주신대로 제거해볼게요!!~~

피케이의 코드를 리뷰하면서 곰곰히 생각해봤는데, DataSource와 마찬가지로 DatabasePopulatorUtils 클래스도 컴포넌트로 등록하면? 컴포넌트 컨테이너 내부에서 DataSource와 DatabasePopulatorUtils의 존재를 인식할 필요 없이 동작이 가능할거 같아요!!!

뒤늦게 로키의 의도를 파악한 느낌?! 다시 한 번 리팩토링 해보겠습니다!!!!!!!!!!!

부푼 기대를 안고 리팩토링을 시도해보았지만, 여전히 DatabasePopulatorUtils.execute()가 발목을 붙잡네요 😭

ComponentContainer에서 초기화를 진행하며 사용될 DataSourceConfig.getInstance()를 위해선 DatabasePopulatorUtils.execute()가 선행되어야하고, DatabasePopulatorUtils.execute()는
DispatcherServlet의 생성자의 호출이 끝난 후 ContextLoaderListener.contextInitialized()를 통해 자동으로 호출되거나
직접 호출해주어야 하더라구요.

즉, ComponentContainer를 DispatcherServlet 내부로 이동시켜야하는데 이 부분이 만족스럽지 못하네요...
또, DispacherServlet 내부로 이동 시켜서 구현한 후에는 Repository 초기화가 문제를 일으키는데,
현재 필드 값으로는 JdbcTemplate을 가지고 있으면서 생성자 파라미터로는 Datasource를 지급 받기 때문에,
필드 타입과 파라미터 타입이 일치하지 않아 초기화에 문제를 일으킵니다.

이에 대해서는 별도의 생성자 판별용 어노테이션을 만들어서 해결할 수 있을 것 같은데, 처음 제가 기획했던 ComponentContainer와 거리가 멀어지는 것 같아서 고민이 되네요 😭

처음 기획했던 모습과는 다르지만, 리플렉션 사용을 연습한다 생각하고 생성자 판별용 @Autowired 어노테이션을 만들어서 문제를 해결했습니다!

DatabasePopulatorUtils.execute() 이슈 때문에 ComponentContainer의 생성 및 초기화 위치를 AnnotationHandlerMapping.initialize() 쪽으로 이동시켰습니다. 어노테이션을 이용한 컴포넌트들을 찾는 클래스이기도 하고, ContextLoaderListener.contextInitialized() 호출 후 동작하므로 DatabasePopulatorUtils.execute() 이슈를 해결할 수 있었어요!

@Repository에서 사용할 DataSource의 경우, @Configuration 어노테이션 클래스의 @Initialize 어노테이션 메서드를 수행해서 @Repository 클래스 생성자에 필요한 데이터를 주입하도록 했습니다.

저는 @Repository가 필요로하는 필드 요소들도 @Bean으로 등록시켜주면 문제가 해결되지 않을까? 라는 단순한 접근으로 시도해봤던건데 하나를 알려주면 열을 안다더니... 현구막을 두고 나온 속담이었군요? 👍

Rok93 · 2021-09-28T16:06:19Z

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

    public static void execute(DataSource dataSource) {
-        Connection connection = null;
-        Statement statement = null;
        try {


앗.... 전 미션 수행할 때, 이거 고쳐야는지 몰라서 안봤는데.... 👀
빨리 try-with-resource로 후다닥 고쳐놔야겠네요 🙄

Rok93 · 2021-09-28T16:08:43Z

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

 public class JdbcTemplate {

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


아니.... preparedStatement 풀 네이밍으로 다 쓰고 계시군요..? ㅋㅋㅋㅋㅋㅋㅋㅋㅋ
구구 수업 중에 했던 말들 다 농담이라 생각했는데... 현구막은 정말 진지했던거였군요...? 👀 👍😆

ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ

Rok93 · 2021-09-28T16:15:23Z

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

+            if (resultSet.next()) {
+                return Optional.of(rowMapper.map(resultSet));
+            }
+
+            return Optional.empty();


실제 QueryForObject의 구현을 까보면....

QueryForObject는 먼저 query() 메서드를 호출하여 얻은 값을 DataAccessUtils.nullableSingleResult(); 함수의 메서드 파라미터로 받고 해당 함수를 호출하여 반환해요! 그리고 이 함수를 코드는 아래와 같아요!

코드를 보시면 아시겠지만 size가 1을 초과하는 경우에 대해서 예외처리가 존재하는 것을 알 수 있어요!
구막도 size가 1을 초과하는 경우에 예외를 발생시키는 로직을 추가하면 어떨까요? 😃

오... 단순히 if(resultSet.next()) 만 생각했지, 그 이상의 크기에 대해서는 핸들링 할 생각을 못했네요!!! 👍 👍 👍

(조회 결과가 있는지 없는지에 따라 예외를 발생시킬지 말지를 라이브러리를 사용하는 개발자가 결정할 사항이라 생각했는데 JdbcTemplate은 에러를 던져주네용... 와우..)

곰곰히 고민해봤는데, 데이터 1개가 넘어올 것을 기대하는 상태에서 조회가 실패했을 때 어차피 라이브러리를 사용하는 개발자도 예외를 이용해서 핸들링 할거라는 생각이 들었어요! 때문에 Optional.empty()를 전달하는게 큰 의미가 없을거 같아서 DataAccessUtils.nullableSingleResult();처럼 리팩토링 해보려구요!!

Rok93 · 2021-09-28T16:35:33Z

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

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


for문이 한 싸이클 돌 때 마다 args.length() 메서드가 호출될 것 같아요 🤔
args.length()를 변수로 추출하여 추출한 변수를 사용하거나 혹은 아래와 같이 Stream을 활용해봐도 좋을 것 같아요 😃

Suggested change

for (int i = 0; i < args.length; i++) {

preparedStatement.setObject(i + 1, args[i]);

}

IntStream.range(0, args.length)

.forEach(it -> preparedStatement.setObject(it + 1, args[it]));

이 부분에 대해서 피드백이 올거같았는데!! 👍

(다른 이야기를 하시다가 예시로)
"예를 들어 for문 루프를 돌릴 때 기준 변수에 list.size()를 사용하는게 성능상 이슈가 발생하겠지만, 그 이슈가 현재 하드웨어 스펙으로 충분히 메꿀 수 있는 수준이고, 개발자끼리 코드를 읽을 때 더 도움이 된다면 충분한 트레이드 오프가 되지 않을까요?"

레벨1~2 사이에 포비께서 예시로 해주신 말씀이에요!! 그래서 변수로 뺄까... 하다가 for문에 포함시켜봤어요!!
forEach의 경우 for문과 달리 루프중에 예외가 발생할 경우에도 멈추지 않고 모든 동작을 수행한다고 알고있어서 사용하기 무섭더라구요 😨 ..

개발자끼리 코드를 읽을 때 더 도움이 된다면 충분한 트레이드 오프가 되지 않을까요?를 생각한다면 로키와 같이 읽는 코드를 생각했을 때 변수로 추출하는게 가장 이상적인 방법인거 같네요!! 바로 적용하겠습니다!! 👍

흠 변수로 빼는 정도는 사실 그렇게 어려운 부분이 아니고 코드의 가독성을 떨어뜨린다고 생각하지는 않는데 🤔
인용하신 피드백 부분의 글을 읽어보니 어느정도 공감이 가네요 😄
그래도 변수로 추출하는 정도라면 저의 의견과 현구막이 이전에 받은 피드백을 둘 다 충족할 수 있을 것 같네요 👍

forEach가 루프중에 예외가 발생할 경우에도 멈추지 않고 모든 동작을 수행한다

는 사실을 잘 모르고 있었는데 하나 배워가네요 😉

Rok93 · 2021-09-28T16:36:08Z

jdbc/src/main/java/nextstep/jdbc/PreparedStatementCallback.java

+import java.sql.PreparedStatement;
+import java.sql.SQLException;
+
+@FunctionalInterface


함수형 인터페이스 표기 Very Good 👍

'뭐지? 도대체 왜 붙이는거지?' 하면서 의문을 가지고 있다가 나중에서야 '아 함수형 인터페이스니 추가 개발하지 말라는 어노테이션이구나..' 하고 깨달은 그것..

ㅋㅋㅋㅋㅋㅋㅋㅋㅋ 몸소 느껴버린 애너테이션이네요 ??
그만큼 더 오랫동안 기억에 남으시겠네요 🤣

예외에 따라 Dao 에서 Optional을 반환하도록 변경

Hyeon9mak · 2021-09-29T14:12:20Z

로키쵸키로키산맥!!!!! 정성스러운 리뷰 감사합니다!!!!!!! 감동이에요!!!!!!!!!! 🥲 (감동의 눈물)

각각 리뷰 남겨주신 곳에 코멘트를 모두 남겨보았어요!!
최대한 제 생각도 포함해서 남겨두었어요!! 함께 더 이야기 나눠봐도 재밌을거 같아요!!
(특히 ModelAndView getter 부분!!)

트랜잭션은 오늘 (아니 어제...?라고 해야겠네요) 구구가 트랜잭션 부분은 구현하지 않아도 된다고 하셨으니 구현은 꼭 하실 필요는 없지만(?) 혹시나 욕심나신다면 진행해보셔도 좋을 것 같아요.

트랜잭션을 직접 관리하는 방법에 대해서 찾아보니 아래와 같은 내용을 확인할 수 있었어요!

void setAutoCommit(boolean autoCommit) throws SQLException
If a connection is in auto-commit mode, then all its SQL statements will be executed and committed as individual transactions.

즉 별도로 setAutoCommit(false)를 하지 않는 이상, 각각의 쿼리가 모두 트랜잭션으로 관리가 된다는거 같아요!
setAutoCommit(false)를 사용해서 쿼리 여럿을 묶어 트랜잭션으로 관리하는 경우는 서비스 계층에서 @Transactional 어노테이션을 활용할 때와 비슷한 상황인거 같은데, 아쉽게도 현재 app 모듈에는 쿼리 여럿을 묶는 메서드가 없네요.. 😞

이 부분에 대해서 에어와 영이가 굉장히 흥미로운 대화를 하셨더라구요! 로키도 읽어보시면 재밌을거 같아서 링크 남겨욧!

이번 리뷰도 잘 부탁드려요!!!!!!! 🙇‍♂️ 🙇‍♂️ 🙇‍♂️ 🙇‍♂️ 🙇‍♂️ 🙇‍♂️
미키쿠키와이키키로키 중에 최고는 로키 화이팅!! 👍 🙌 🙌 🙌

Autowired 어노테이션이 붙은 생성자를 이용하도록 변경 ComponentContainer 생성 위치를 AnnotationHandlerMapping 클래스로 변경 UserDao rowMapper를 static하지 않게 변경 (UserDao가 싱글톤이라 static일 필요 없음)

sonarqubecloud · 2021-10-04T12:34:59Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
13 Code Smells

74.1% Coverage
0.0% Duplication

Rok93 · 2021-10-04T14:04:57Z

app/src/main/java/com/techcourse/config/DataSourceConfig.java

-    public static javax.sql.DataSource getInstance() {
+    private DataSourceConfig() {}
+
+    @Initialize


저는 단순히 @Bean 애너테이션을 사용했었는데, @Configuration과 @Initialize가 더 적절했겠네요 👍

@Configuration은 피케이 코드를 리뷰하다가 영감을 얻었고, @Initialize는 '초기화를 하면서 딱 한 번만 수행될 메서드를 표현할게 뭐가 있을까...' 고민하다가 직관적인 이름을 쓰자는 생각으로 접근했어요! 👍

Rok93 · 2021-10-04T14:06:59Z

mvc/src/main/java/nextstep/web/ComponentContainer.java

+        try {
+            return type.getDeclaredConstructor();
+        } catch (NoSuchMethodException e) {
+            throw new ComponentContainerException("기본 생성자 탐색에 실패했습니다.");


우와... 👍 이 예외는 진짜 현구막의 꼼꼼함이 느껴지네요 😧

Rok93 · 2021-10-04T14:14:29Z

mvc/src/main/java/nextstep/web/ComponentContainer.java

-            .toArray(Class<?>[]::new);
+    private static Constructor<?> getConstructor(Class<?> type) {
+        return Arrays.stream(type.getConstructors())
+            .filter(constructor -> constructor.isAnnotationPresent(Autowired.class))


이런식으로 빈들을 주입해줄 (특정한) 생성자를 지정하고 그 생성자들에 빈을 주입하는거군요 ? 🤔
생각해보니 저희가 Spring프레임워크를 쓸 때도 하나의 생성자만 존재하는 경우에는 예외적으로 @Autowired를 생략하는 것이지 실제로는 @Autowired가 붙어있는거였죠?
이런 관점으로 생각하니 현구막이 구현한 방식이 어쩌면 더 스프링스러운거 같네요 😃

Rok93 · 2021-10-04T14:28:32Z

app/src/main/java/com/techcourse/AppWebApplicationInitializer.java


    private static final Logger LOG = LoggerFactory.getLogger(AppWebApplicationInitializer.class);
    private static final String BASE_PACKAGE = "com.techcourse";



해당 클래스에 불필요한 import 문들이 있는데 제거하면 좋을 것 같아요 👀

앗... 꼼꼼한 리뷰 감사합니다 ㅠㅠ 요즘 120자 제한을 두고도 가독성을 위해 조금씩 넘겨서 코드를 짜다보니
코드 리포맷팅을 사용하면 코드 라인이 넘어가는게 싫어서 리포맷팅을 잘 안누르게 되네요.. 이상한 버릇이 생겨버렸어요..
넘길 땐 넘길 줄 알아야 하는데!!!!!

Rok93 · 2021-10-04T14:36:16Z

mvc/src/main/java/nextstep/web/ComponentContainer.java

+        initializeComponents(reflections.getTypesAnnotatedWith(Repository.class));
        initializeComponents(reflections.getTypesAnnotatedWith(Service.class));
        initializeComponents(reflections.getTypesAnnotatedWith(Controller.class));


그럴 일이 있을지는 모르겠지만 새로운 애너테이션이 생성된다면 계속해서 한 줄 한 줄 코드가 추가될 것 같아요.
Repository, Service, Controller class 를 리스트로 두는 상수 (ex) ANNOTATION_COMPONENTS를 두고 추후에는 아래와 같은 구조로 변경을 해볼 수도 있을 것 같네요 😃

for (Class<? extends Annotation> clazz : ANNOTATION_COMPONENTS) { initializeComponents(reflections.getTypesAnnotatedWith(clazz)); }

아마도 이 코드는

Controller와 Service를 생성하는 메서드가 중복이 됨에도 현상을 유지한 가장 큰 이유는 가독성 때문이었어요!
말씀해주신대로 Repositories는 DataSource의 존재로 Controller, Service와 조금 다른 로직을 수행하게 되는데
통일성을 유지 시키고 싶었어요.

라던 현구막의 생각이 반영된 코드가 아닌가 싶어서 나중에는 이렇게 바뀔 수도 있겠네요~ 라고 언급하고 넘어가는 정도로 할게요 😉😃

리스트는 순서를 보장할 수 있으니 충분히 가능하겠네요!!!
initializeComponents(reflections.getTypesAnnotatedWith()); 자체도 사실 따지고 보면 코드 중복이니...
리팩토링 해보겠습니다!!

말씀해주신대로 상수로 관리하려하다가, 당장은 사용하는 곳이 한 곳 뿐이라 지역변수로 두었습니다!

public static void initialize(Object... basePackage) throws Exception { Reflections reflections = new Reflections(basePackage); initializeConfigurations(reflections.getTypesAnnotatedWith(Configuration.class)); List<Class<? extends Annotation>> annotations = List.of(Repository.class, Service.class, Controller.class); for (Class<? extends Annotation> annotation : annotations) { initializeComponents(reflections.getTypesAnnotatedWith(annotation)); } }

Rok93 · 2021-10-04T14:39:52Z

mvc/src/main/java/nextstep/web/ComponentContainer.java

+    public static void initializeComponents(DataSource dataSource, Object... basePackage) throws Exception {
+        Reflections reflections = new Reflections(basePackage);
+        initializeRepositories(reflections.getTypesAnnotatedWith(Repository.class), dataSource);
+        initializeServices(reflections.getTypesAnnotatedWith(Service.class));
+        initializeControllers(reflections.getTypesAnnotatedWith(Controller.class));
+    }
+
+    private static void initializeRepositories(Set<Class<?>> repositoryTypes, DataSource dataSource) throws Exception {
+        for (Class<?> repository : repositoryTypes) {
+            Object instance = repository.getConstructor(DataSource.class).newInstance(dataSource);
+            COMPONENTS.put(repository, instance);
+        }
+    }
+
+    private static void initializeServices(Set<Class<?>> serviceTypes) throws Exception {
+        for (Class<?> service : serviceTypes) {
+            Class<?>[] fieldTypes = getFieldTypes(service);
+            Object[] fieldObjects = getFieldObjects(fieldTypes);
+
+            Object serviceInstance = service.getConstructor(fieldTypes).newInstance(fieldObjects);
+            COMPONENTS.put(service, serviceInstance);
+        }
+    }
+
+    private static void initializeControllers(Set<Class<?>> controllerTypes) throws Exception {
+        for (Class<?> controller : controllerTypes) {
+            Class<?>[] fieldTypes = getFieldTypes(controller);
+            Object[] fieldObjects = getFieldObjects(fieldTypes);
+
+            Object controllerInstance = controller.getConstructor(fieldTypes).newInstance(fieldObjects);
+            COMPONENTS.put(controller, controllerInstance);
+        }
+    }


저는 @Repository가 필요로하는 필드 요소들도 @Bean으로 등록시켜주면 문제가 해결되지 않을까? 라는 단순한 접근으로 시도해봤던건데 하나를 알려주면 열을 안다더니... 현구막을 두고 나온 속담이었군요? 👍

Rok93

현구막 늦어서 죄송합니다 🥲
피드백 잘 반영해주셨네요 👍
가볍게 툭 던지고 갔는데 결국 더 좋은 방향으로 코드를 변경하셨네요 😆
하나를 알면 열을 안다는 현구막 저 또한 이번 미션에서 현구막 코드를 리뷰하면서 많이 배웠네요 😉
정말 고생많으셨습니다 💪

혹시라도 제가 빠트리고 넘어간 부분이나 더 토론해보고 싶은 내용들이 있다면 코멘트 남겨주시면 계속 팔로우하고 제 의견을 남겨보겠습니다 🙏

Rok93 · 2021-10-04T14:51:12Z

app/src/main/java/com/techcourse/domain/User.java

+    private Account account;
+    private Password password;
+    private Email email;


원시값을 포장하셨군요 👍 (레벨 1 이후로 오랜만에 듣는 정겨운 멘트 아닌가요? 😆)
간만에 보는 불변객체네요 😢

Rok93 · 2021-10-04T14:58:37Z

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

+        if (results.isEmpty()) {
+            throw new DataAccessException("데이터 조회에 실패했습니다.");
+        }


queryForObject 메서드의 결과 값을 Optional로 반환하면 어떨까요?
값이 조회되지 않는 경우에 대해서 프레임워크 혹은 라이브러리(JdbcTemplate)가 아닌 개발자가 직접 조작할 수 있다는 장점이 있지 않을까요 ? 🤔

현구막은 어떻게 생각하시나요 😃

이전에는 Optional을 사용했었는데, #34 (comment) 로키가 남겨주신 리뷰를 읽고 생각을 해보니 '통일성'에 대한 생각이 가장 먼저 떠올랐어요.

1개의 값이 조회되는 경우엔 Optional.of()

값이 조회되지 않는 경우는 Optional.empty()

2개 이상이 값이 조회되는 경우엔 예외...?

queryForObject 메서드에서 조회에 실패하는 경우도, 2개 이상의 값을 조회하는 경우도 모두 예외 상황인데,
한 쪽은 Optional을 반환하고, 한 쪽은 예외를 던진다는게 부자연스럽다고 느껴졌어요!
그래서 Optional을 벗겨내고, 2개의 상황에서는 예외를 던지도록 했습니다!!
(제가 Optional을 다르게 사용하는 방법을 몰라서... 로키의 뜻을 또 놓치는 걸수도... 😅)

만약 Optional을 통해서 2개 이상의 값이 조회된 것을 표현 가능하다면 다시 Optional을 적용할 의사가 있습니다!!

Hyeon9mak added 22 commits September 22, 2021 10:02

test: 학습 테스트 완성

22072d5

refactor: User 클래스 멤버 변수 설정 리팩토링

d9cfaa1

refactor: UserDao.insert시 id(primary key)를 포함한 User를 반환하도록 변경

6d62613

feat: UserDao.update 구현

80140bd

feat: UserDao.findAll, UserDao.deleteAll 구현

c158788

feat: UserDao.findById, UserDao.findByAccount 구현

2d3fb19

refactor: UserDao.deleteAll, UserDao.update 책임 분리

91695f5

refactor: JdbcTemplate 생성 및 데이터베이스 작업 수행 책임 분리

acf57da

refactor: queryForObject, query 구현 및 JdbcTemplate 패키지 위치 변경

d9225b1

docs: Todo 리스트 업데이트

54898b1

feat: 커스텀 예외처리 적용

8fab7ae

feat: SimpleJdbcInsert 구현

4e4415a

refactor: SimpleJdbcInsert 커스텀 예외 추가 및 변수명 리팩토링

4b17405

refactor: DatabasePopulatorUtils 메서드 분리

308315d

refactor: mvc 모듈 리팩토링

dbf9765

refactor: jdbcTemplate 에서 Optional을 반환하도록 변경

1f0c15d

refactor: 사용되지 않는 import 제거

37087bf

refactor: app 모듈 리팩토링

832fb74

refactor: App 컴포넌트 초기화 단계에서 Datasource 설정을 초기화 한 후 사용하도록 변경

e85d4d8

refactor: Handler URL 메소드 변경, Conflict 핸들링 페이지 추가

85c9ffa

chore: jacoco 테스트 설정 추가

a6111c0

refactor: ComponentContainer 초기화시 필요한 DataSource 모킹

f1b96c1

Hyeon9mak requested a review from Rok93 September 26, 2021 01:45

Hyeon9mak self-assigned this Sep 26, 2021

kang-hyungu requested changes Sep 27, 2021

View reviewed changes

refactor: JdbcTemplate에서 커스텀 예외를 사용하도록 변경

e0e3b96

Rok93 suggested changes Sep 28, 2021

View reviewed changes

Hyeon9mak added 3 commits September 29, 2021 19:27

refactor: JdbcTemplate for문 기준 변수를 미리 추출해서 사용하도록 변경

86eb046

refactor: JdbcTemplate 에서 단일 오브젝트 조회 결과가 1개를 초과할 때 예외 추가

9cb151c

예외에 따라 Dao 에서 Optional을 반환하도록 변경

refactor: Optional을 사용할 때 isPresent 대신 ifPresent를 사용하도록 변경

a7fa076

Hyeon9mak added 4 commits September 29, 2021 21:22

refactor: VO 불변 유지를 위한 원시값 포장

7defbd9

refactor: 커스텀 예외에 메세지를 포함하도록 수정

92a2d11

refactor: JsonView 에서 복수/단수 데이터 조회시 예외 상황 변경

ced9630

refactor: ComponentContainer 중복 메서드 제거

4d36e3e

Rok93 reviewed Oct 4, 2021

View reviewed changes

Rok93 approved these changes Oct 4, 2021

View reviewed changes

Rok93 merged commit 0528e2d into woowacourse:hyeon9mak Oct 4, 2021

		@@ -0,0 +1,5 @@
		package nextstep.mvc.exception;

		public class ComponentContainerException extends RuntimeException {


		private static final Logger LOG = LoggerFactory.getLogger(AppWebApplicationInitializer.class);
		private static final String BASE_PACKAGE = "com.techcourse";

[JDBC 라이브러리 구현하기 - 1, 2단계] 현구막(최현구) 미션 제출합니다. #34

[JDBC 라이브러리 구현하기 - 1, 2단계] 현구막(최현구) 미션 제출합니다. #34

Uh oh!

Conversation

Hyeon9mak commented Sep 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SimpleJdbcInsert

모듈들을 이식했어요

인스턴스 싱글톤 유지를 위한 ComponentContainer

트랜잭션 관리는 어떻게...?

Uh oh!

kang-hyungu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rok93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyeon9mak Sep 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Rok93 Sep 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Hyeon9mak commented Sep 29, 2021

Hyeon9mak commented Sep 26, 2021 •

edited

Loading

Hyeon9mak Sep 29, 2021 •

edited

Loading

Rok93 Sep 28, 2021 •

edited

Loading

Hyeon9mak Oct 4, 2021 •

edited

Loading