[DI - Step 2] 베디 미션 제출합니다. #51

dpudpu · 2019-11-11T17:54:41Z

진행하면서 고민했던 부분이 두 가지 있습니다. 어떻게 생각하시는지 알려주시면 감사하겠습니다.

@ComponentScan 처리를 어디서 할지 고민하다가 @Configuration과 항상 같이 사용되어야 한다고 생각해서ConfigurationBeanScanner 에서 해줬습니다.
@Bean이 붙은 메소드를 @Configuration만 아닌 모든 Component 에서 사용가능한 거 같아서 BeanFactory에서 해줬습니다.

이번에도 피드백 잘 부탁드립니다.
항상 감사합니다.

@Inject

* feat: BeanFactory 빈 생성 구현 * feat: BeanScanner * feat: AnnotationHandlerMapping DI 프레임워크 적용 * feat: Slipp 모듈에 DI 프레임워크 적용 * refactor: BeanFactory Bean 싱글 인스턴스로 관리되게 변경 * refactor: BeanFactory 변수 이름 변경 * feat: interface의 concrete 클래스 @Inject 처리 및 테스트 추가 * refactor: BeanFactory 전체적인 리팩토링 * refactor: BeanScanner의 컴포넌트 어노테이션 상수화 * refactor: 람다식 내의 check exception 처리 - 메소드 추출로 가독성 향상 * refactor: AnnotationHandlerMapping - createHandlerExecution for문 로직 메소드 추출 * refactor: BeanFactory.getControllers() 리턴 타입 Map -> List<Class> 변경 * refactor: BeanFactory.findMethodsByAnnotation 추가 * refactor: 싱글 인스턴스 적용

byjo

이번 미션 쉽지 않았을텐데 테스트와 함께 잘 구현하셨어요. 👏
특히 bean 생성에 필요한 정보들을 추상화 하여, 기능이 추가되었지만 생성 로직은 훨씬 간결해졌네요. 💯
고민하신 부분에 대한 제 생각과 피드백 몇 가지 남겼으니 확인해주세요.
좀 더 이야기 나눠보고 싶은 점 있으시면 댓글이나 DM 주시구요!

byjo · 2019-11-12T11:10:58Z

nextstep-di/src/main/java/nextstep/di/factory/ConfigurationBeanScanner.java

+    private final Set<Class<?>> classTypes;
+
+    public ConfigurationBeanScanner(Class<?>... configurations) {
+        this.basePackages = initBasePackages(configurations);


@componentscan 처리를 어디서 할지 고민하다가 @configuration과 항상 같이 사용되어야 한다고 생각해서ConfigurationBeanScanner 에서 해줬습니다.

basePacakge는 다른 scanner에도 쓰이는 정보인데 특정 scanner에서 처리를 해주는 것이 개인적으로 어색하게 느껴지네요. ClasspathBeanScanner가 ConfigurationBeanScanner에 의존성을 갖게 되면서, 같은 인터페이스의 구현체인 두 스캐너가 동등한 레벨로 존재하지 않는 점도 어색하구요.

또한, ConfigurationBeanScanner의 변경이 객체의 주 목적인 scan이 아닌 basePackage 관련 로직에 의해서도 발생할 수 있다는 점에서는 단일 책임 원칙을 위반하는 것 아닐까요?

두 스캐너의 상위 레이어인 ApplicationContext에서 처리하는 것은 어떨까요?

ComponentScanner 클래스를 따로 만들어서 ApplicationContext에서 처리해줬습니다

byjo · 2019-11-12T11:24:01Z

nextstep-di/src/main/java/nextstep/di/factory/DefaultBeanFactory.java

+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+public class DefaultBeanFactory implements BeanFactory {


기존의 BeanFactory는 bean 인스턴스를 만드는 일만을 했었는데, 이번에 BeanDefinition을 만드는 책임이 추가되면서 복잡도가 늘었네요.
BeanFactory는 bean 인스턴스를 생성, 저장, 조회와 같은 bean 관리에 초점을 두고, BeanDefinition에 대한 책임은 다른 객체가 갖도록 해보면 어떨까요?

@configuration에만 @bean을 사용할 수 있게 변경하고, BeanDefinition 생성은 각 BeanScanner에서 하게 변경했습니다.
생성한 BeanDefinition은 BeanDefinitionRegistry에 등록하게 변경했습니다.
이런 흐름 제어는 ApplicationContext에서 해줬습니다.

byjo · 2019-11-12T12:09:42Z

nextstep-di/src/main/java/nextstep/di/factory/DefaultBeanFactory.java

+
+    private void initMethodBeanDefinition(final Class<?> clazz, final DefaultBeanDefinition dbd) {
+        Stream.of(clazz.getMethods())
+                .filter(method -> method.isAnnotationPresent(Bean.class))


@bean이 붙은 메소드를 @configuration만 아닌 모든 Component 에서 사용가능한 거 같아서 BeanFactory에서 해줬습니다.

예를 들어, @controller 어노테이션이 있는 클래스에서 메소드에 @bean 어노테이션을 사용해 빈 생성을 할 수 있도록 지원한다는 것일까요?
어떤 경우에 이렇게 사용할 것으로 예상하시며, 이렇게 사용할 경우 어떤 장점이 있을까요?

예를 들어, @controller 어노테이션이 있는 클래스에서 메소드에 @bean 어노테이션을 사용해 빈 생성을 할 수 있도록 지원한다는 것일까요?

네 맞습니다. 필요 없고 이상하다고 생각하지만, 스프링에서는 되길래 이렇게 했습니다. 이점은 없고 오히려 혼란만 발생할 거 같아서 @configuration에서만 처리하는 게 낫다고 판단하여 수정했습니다.

byjo · 2019-11-12T12:17:11Z

nextstep-di/src/main/java/nextstep/di/factory/DefaultBeanFactory.java

+
+    }
+
+    private Object createBean(BeanDefinition beanDefinition) {


BeanDefinition의 추상화를 통해 빈 생성 로직을 깔끔하게 정리하셨네요 💯💯

감사합니다 ㅎㅎ

byjo · 2019-11-12T12:17:43Z

nextstep-di/src/main/java/nextstep/di/factory/DefaultBeanFactory.java

+        T bean = (T) beans.get(requiredType);
+
+        if (bean != null) {
+            return bean;
+        }


해당 로직 필요할까요?
아래 두 줄로 충분하지 않을까요?

public <T> T getBean(final Class<T> requiredType) { Class<?> concreteClass = BeanFactoryUtils.findConcreteClass(requiredType, beans.keySet()); return (T) beans.get(concreteClass); }

변경했습니다. 감사합니다!

byjo · 2019-11-12T12:17:56Z

slipp/src/main/java/slipp/support/config/MyConfiguration.java

+
+@Configuration
+@ComponentScan(basePackages = "slipp")
+public class MyConfiguration {


아래 문제 상황을 해결하기 위해, JdbcTemplate의 DataSource를 변경하고 이에 따른 어플리케이션 설정을 추가하면 좋을 것 같아요.

JdbcTemplate를 분석하다보니 데이터베이스 Connection을 생성하는 부분도 static으로 구현되어 있다. 또한 데이터베이스 설정 정보 또한 하드 코딩으로 관리하고 있어 특정 데이터베이스에 종속되는 구조로 구현되어 있다. 데이터베이스에 종속되지 않도록 구현하고 Connection Pooling을 지원하기 위해 Connection 대신 javax.sql.DataSource 인터페이스에 의존관계를 가지도록 지원하면 좋겠다.

@Configuration @ComponentScan(basePackages = "slipp") public class MyConfiguration { @Bean public DataSource dataSource() { BasicDataSource ds = new BasicDataSource(); ds.setDriverClassName("org.h2.Driver"); ds.setUrl("jdbc:h2:~/jwp-framework;MVCC=TRUE;DB_CLOSE_ON_EXIT=FALSE"); ds.setUsername("sa"); ds.setPassword(""); return ds; } @Bean public JdbcTemplate jdbcTemplate(DataSource dataSource) { return new JdbcTemplate(dataSource); } }

이렇게 변경해줬습니다.

궁금한 부분이 있습니다. ContextLoaderListener에서 jwp.sql 파일을 읽어서 DB에 쿼리를 날려주기위해서 DataSource가 필요한데 WebApplicationInitializer 에서 생성 뒤에 ServletContext 의 attribute를 통해서 전달했는데 괜찮을까요?

public class SlippWebApplicationInitializer implements WebApplicationInitializer { @Override public void onStartup(ServletContext servletContext) throws ServletException { ApplicationContext applicationContext = new ApplicationContext(MyConfiguration.class); servletContext.setAttribute(SERVLET_CONTEXT_DATASOURCE, applicationContext.getBean(DataSource.class)); .... } @WebListener public class ContextLoaderListener implements ServletContextListener { @Override public void contextInitialized(ServletContextEvent sce) { ServletContext servletContext = sce.getServletContext(); DataSource datasource = (DataSource) servletContext.getAttribute(SERVLET_CONTEXT_DATASOURCE); ... } }

네 현재 구조에서는 ServletContext의 attribute를 통해 처리할 수 있겠네요.

좀 더 나아가보면, ContextLoaderListener는 bean인 DataSource에 의존성을 갖고 있기 때문에 ContextLoaderListener 또한 bean으로 만드는 방법도 있을 것 같아요.

di 모듈의 annotation 패키지에 PostConstructor 어노테이션이 정의되어 있는데요.
프레임워크가 빈을 생성한 후 @PostConstructor 어노테이션이 붙은 메서드를 실행하도록 지원해주면, 주어진 요구사항이 해결될 것 같습니다.

위 기능을 구현해보는 것도 좋은 도전이 될 것 같아요. 👏

byjo · 2019-11-12T12:26:27Z

nextstep-di/src/main/java/nextstep/di/factory/BeanScanner.java

@@ -1,8 +1,5 @@
 package nextstep.di.factory;


di의 하위 패키지를 좀 더 세분화 해보면 어떨까요?

scanner 패키지 추가했습니다. 감사합니다!

byjo · 2019-11-12T12:30:54Z

nextstep-di/src/main/java/nextstep/di/factory/BeanScanner.java

-    public Set<Class<?>> getBeans() {
-        return clazz;
-    }
+public interface BeanScanner {


BeanScanner 구현체의 getClassTypes() 메소드가 단순히 멤버 변수를 리턴하고 있고,
두 구현체 간의 차이가 (Component 스캔 로직 외에) 거의 없는 것을 보면,
BeanScanner 추상화의 방향에 약간 의문을 갖게 되네요.

위에 드린 피드백과 함께 BeanScanner 객체를 만든 목적과 이 객체의 역할이 무엇일지 한 번 고민해보면 좋을 것 같아요.

BeanScanner 에서는 BeanDefinition을 생성하게 변경했습니다.

dpudpu · 2019-11-14T02:21:29Z

피드백 반영하다보니 변경된 점이 많아서 각 피드백에 어떻게 변경했는지 댓글 남겼습니다.
좋은 피드백 주셔서 감사합니다.

byjo

피드백 반영과 리팩토링 잘 진행하셨어요.
객체를 책임에 맞게 잘 설계하시는 것 같아요. 💯
문의주신 사항에 대해 피드백 드렸으니 확인도 함께 부탁드려요.
di 미션 마무리 할게요. 고생 많으셨습니다!

byjo · 2019-11-15T04:37:49Z

nextstep-di/src/main/java/nextstep/di/bean/BeanDefinitionRegistry.java

+import java.util.Collection;
+import java.util.List;
+
+public interface BeanDefinitionRegistry {


byjo · 2019-11-15T04:43:56Z

slipp/src/main/java/slipp/support/config/MyConfiguration.java

+
+@Configuration
+@ComponentScan(basePackages = "slipp")
+public class MyConfiguration {


네 현재 구조에서는 ServletContext의 attribute를 통해 처리할 수 있겠네요.

좀 더 나아가보면, ContextLoaderListener는 bean인 DataSource에 의존성을 갖고 있기 때문에 ContextLoaderListener 또한 bean으로 만드는 방법도 있을 것 같아요.

di 모듈의 annotation 패키지에 PostConstructor 어노테이션이 정의되어 있는데요.
프레임워크가 빈을 생성한 후 @PostConstructor 어노테이션이 붙은 메서드를 실행하도록 지원해주면, 주어진 요구사항이 해결될 것 같습니다.

위 기능을 구현해보는 것도 좋은 도전이 될 것 같아요. 👏

pobiconan and others added 10 commits October 29, 2019 09:10

apply slingeton pattern to UserDao, JdbcTemplate

950ea6e

refactor: jdbc url

85c8a2f

feat: ConfigurationBeanScanner 구현

1de73e9

feat: BeanDefinition

d1f7fad

feat: BeanDefinitionException

ca56aa9

refactor: 전체적으로 변수, 메소드 이름 변경 및 메소드 추출

63e8001

merge from ike-branch

dd1a35a

test: slipp 모듈 DispatcherServletTest di-2단계 적용

14df6f3

refactor: DefaultBeanDefinition

f0aec05

byjo suggested changes Nov 12, 2019

View reviewed changes

dpudpu added 5 commits November 13, 2019 00:17

refactor: JdbcTemplate Connection -> DataSource로 변경 및 애플리케이션 적용

815587f

refactor: DefaultBeanFactory.getBean() 로직 변경

1e5d308

refactor: di 하위패키지 세분화

1795b1e

feat: ComponentScanner 추가

18b31fb

feat: BeanDefinitionRegistry

bfc8349

refactor: BeanScanner의 default 메소드 BeanUtils 로 이동

0831f13

dpudpu force-pushed the dpudpu branch from b82d214 to 0831f13 Compare November 14, 2019 02:35

byjo self-assigned this Nov 15, 2019

byjo approved these changes Nov 15, 2019

View reviewed changes

byjo merged commit b121fad into woowacourse:dpudpu Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI - Step 2] 베디 미션 제출합니다. #51

[DI - Step 2] 베디 미션 제출합니다. #51

dpudpu commented Nov 11, 2019 •

edited

Loading

byjo left a comment

byjo Nov 12, 2019

dpudpu Nov 12, 2019 •

edited

Loading

byjo Nov 12, 2019

dpudpu Nov 14, 2019 •

edited

Loading

byjo Nov 12, 2019

dpudpu Nov 13, 2019 •

edited

Loading

byjo Nov 12, 2019

dpudpu Nov 12, 2019

byjo Nov 12, 2019

dpudpu Nov 12, 2019 •

edited

Loading

byjo Nov 12, 2019

dpudpu Nov 12, 2019

byjo Nov 15, 2019

byjo Nov 12, 2019

dpudpu Nov 12, 2019

byjo Nov 12, 2019

dpudpu Nov 14, 2019

dpudpu commented Nov 14, 2019

byjo left a comment

byjo Nov 15, 2019

byjo Nov 15, 2019

[DI - Step 2] 베디 미션 제출합니다. #51

[DI - Step 2] 베디 미션 제출합니다. #51

Conversation

dpudpu commented Nov 11, 2019 • edited Loading

byjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu Nov 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu Nov 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu Nov 13, 2019 • 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

dpudpu Nov 12, 2019 • 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

dpudpu commented Nov 14, 2019

byjo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpudpu commented Nov 11, 2019 •

edited

Loading

dpudpu Nov 12, 2019 •

edited

Loading

dpudpu Nov 14, 2019 •

edited

Loading

dpudpu Nov 13, 2019 •

edited

Loading

dpudpu Nov 12, 2019 •

edited

Loading