[DI - Step 1] 효오 미션 제출합니다. #11

hyojaekim · 2019-10-31T06:56:02Z

안녕하세요! 효오라고 합니다.

미션을 진행하면서 궁금한게 있습니다.
BeanScanner와 BeanFactory를 생성하는 위치에 대해 많이 고민했었습니다.
현재는 DispatcherServlet을 생성해주는 SlippWebApplicationInitializer에 스캐너와 팩터리를 생성해주고 있는데 위치가 적절한지 궁금합니다!

많은 지적 부탁드립니다!

- UserService 추가

kingbbode

안녕하세요!
요구사항 잘 구현해주셔서 바로 머지하겠습니다 👍
피드백 몇가지도 남겨두었습니다 :) 확인 부탁드려요.

kingbbode · 2019-10-31T14:53:14Z

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

+            }
+            return createInstance(injectedConstructor);
+        } catch (Exception e) {
+            logger.error("### Bean create fail : {}", e.getMessage());


예외를 추적하기 위한 정보가 e.getMessage() 면 충분할까요? 계획한 수준의 예외가 아니라면 Exception 상태를 보존해주면 좋을 것 같아요.

https://rules.sonarsource.com/java/tag/error-handling/RSPEC-1166
https://www.slipp.net/questions/350

kingbbode · 2019-10-31T15:13:08Z

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

+            putBean(parameterType);
+            parameters.add(getBean(parameterType));


putBean, getBean 의 흐름이 어색해보이네요. putBean 메서드 자체의 의미는 충분히 있고 getBean 메서드에도 자체의 의미가 분명 있습니다. 그러나 createInstance 에서는 최적화되어 있기보다는 방어 로직으로 putBean을 사용하여 불필요할 수도 있는 행위를 수행하는 코드이지는 않을까요? 메서드의 행위에 대한 최적화가 더 의미있지 않을까 생각해봅니다 :)

getBean 메소드에서 존재하지 않는 클래스 타입인 경우 만들어서 가져오도록 로직을 추가했습니다!

kingbbode · 2019-10-31T15:32:43Z

slipp/src/main/java/slipp/controller/UserController.java

+    private final UserService userService;
+    private UserDao userDao;


final은 어떤 기준으로 작성 되었나요?

빼먹은 부분이 있었습니다! 감사합니다.

kingbbode

안녕하세요!
요구사항 구현이 잘 되긴 하였으나 기능상의 결점이 보여 피드백을 남겼습니다 :)
확인 부탁드려요!

kingbbode · 2019-10-31T15:48:41Z

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

+
+    private void putBean(Class<?> preInstanticateBean) {
+        if (!beans.containsKey(preInstanticateBean)) {
+            beans.put(preInstanticateBean, createBean(preInstanticateBean));


(장황하게 작성하여 코멘트를 수정합니다!)
의도한 Bean 의 유형과 범위에서만 생성되고 있나요?
유형 : Controller, Service, Repository Annotation 이 작성된 class
범위 : package

이에 대한 테스트케이스도 작성이 되면 좋겠어요.

kingbbode · 2019-10-31T15:52:18Z

처음에 안보였던 케이스가 나중에 보여 상태를 변경하였습니다 :)
추가로 현재 BeanFactory 테스트가 작성하신 기능에 비해 많이 부족한 점이 보이는 것 같은데요. 구현하신 요구사항들이 모두 드러날 수 있는 테스트 부탁드려요.
시간 여유가 되신다면 Reflection 을 사용하지 않고 BeanFactory 에 대한 테스트도 작성해보시는 것을 추천드려요 :)(필수는 아닙니다!)

- BeanFactory와 BeanScanner의 생명주기를 같이 가져가기 위해 BeanFactory 내부에서 BeanScanner 생성

- getBean 실패하는 테스트 추가

- getControllersFail 실패 테스트 추가 - getBean 성공 테스트 추가

hyojaekim · 2019-11-06T03:53:34Z

안녕하세요!

예외를 추적하기 위한 정보가 e.getMessage() 면 충분할까요? 계획한 수준의 예외가 아니라면 Exception 상태를 보존해주면 좋을 것 같아요.
습관적으로 e.getMessage() 써왔는데 위와 같은 피드백으로 예외에 대한 많은 고민 해봐서 좋았습니다!

kingbbode · 2019-11-07T00:54:07Z

말씀드린 의도가 잘못 전달된 것 같네요 :)

@Service
public class TestService {
    public TestService(A a) {
        ...
    }
}

public class A {
}

A class 의 객체는 생성되면 안됩니다 :)

kingbbode

의도가 잘못 전달된 듯 하여 코멘트를 조금 더 상세하게 남겼습니다!
확인 부탁드려요 :)

kingbbode · 2019-11-07T00:58:00Z

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

+        }
+
+        if (!preInstanticateBeans.contains(requiredType)) {
+            throw new IllegalArgumentException("해당 클래스를 찾을 수 없습니다.");


조금 더 상세한 로그를 남겨주는 것이 추적하기 쉬울 것 같아요 :)

kingbbode

내용 확인하여 머지합니다!
구현 잘 해주셔서 감사합니다 :) 👍

hyojaekim added 7 commits October 31, 2019 12:58

feat: BeanFactory initialize 구현

1cfc172

feat: BeanFactory DI 구현

00b44b2

refactor: BeanFactory DI

fb8709a

feat: BeanFactory getControllers 기능 추가

b4870de

feat: MVC 프레임워크 DI 적용

6aa1b9b

refactor: ControllerScanner 제거

8506e7f

refactor: Slipp 모듈 DI 적용

091a1df

- UserService 추가

kingbbode approved these changes Oct 31, 2019

View reviewed changes

kingbbode self-requested a review October 31, 2019 15:42

kingbbode suggested changes Oct 31, 2019

View reviewed changes

vsh123 and others added 9 commits November 4, 2019 15:26

refactor: Exception logging 변경

6050ab9

refactor: UserDao 싱글턴 제거

1bff5d4

fix: Controller UserDao final 적용

7217148

refactor: BeanFactory, BeanScanner

cea4809

- BeanFactory와 BeanScanner의 생명주기를 같이 가져가기 위해 BeanFactory 내부에서 BeanScanner 생성

fix: UserDao 싱글턴 제거에 따른 에러 수정

179e91b

refactor: BeanFactory.createInstance()

11f986e

feat: validateClassType 추가

79ae8e4

- getBean 실패하는 테스트 추가

test: BeanFactory 불필요한 필드 및 메서드 제거

c0e31ef

test: BeanFactory 테스트 추가

30fd57c

- getControllersFail 실패 테스트 추가 - getBean 성공 테스트 추가

kingbbode assigned kingbbode and unassigned kingbbode Nov 7, 2019

kingbbode suggested changes Nov 7, 2019

View reviewed changes

kingbbode reviewed Nov 7, 2019

View reviewed changes

kingbbode approved these changes Nov 7, 2019

View reviewed changes

kingbbode merged commit f9d0e45 into woowacourse:hyojaekim Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DI - Step 1] 효오 미션 제출합니다. #11

[DI - Step 1] 효오 미션 제출합니다. #11

hyojaekim commented Oct 31, 2019

kingbbode left a comment

kingbbode Oct 31, 2019

kingbbode Oct 31, 2019

hyojaekim Nov 4, 2019

kingbbode Oct 31, 2019

hyojaekim Nov 4, 2019

kingbbode left a comment

kingbbode Oct 31, 2019 •

edited

Loading

kingbbode commented Oct 31, 2019

hyojaekim commented Nov 6, 2019

kingbbode commented Nov 7, 2019

kingbbode left a comment

kingbbode Nov 7, 2019

kingbbode left a comment

		putBean(parameterType);
		parameters.add(getBean(parameterType));

		private final UserService userService;
		private UserDao userDao;

[DI - Step 1] 효오 미션 제출합니다. #11

[DI - Step 1] 효오 미션 제출합니다. #11

Conversation

hyojaekim commented Oct 31, 2019

kingbbode left a comment

Choose a reason for hiding this comment

kingbbode Oct 31, 2019

Choose a reason for hiding this comment

kingbbode Oct 31, 2019

Choose a reason for hiding this comment

hyojaekim Nov 4, 2019

Choose a reason for hiding this comment

kingbbode Oct 31, 2019

Choose a reason for hiding this comment

hyojaekim Nov 4, 2019

Choose a reason for hiding this comment

kingbbode left a comment

Choose a reason for hiding this comment

kingbbode Oct 31, 2019 • edited Loading

Choose a reason for hiding this comment

kingbbode commented Oct 31, 2019

hyojaekim commented Nov 6, 2019

kingbbode commented Nov 7, 2019

kingbbode left a comment

Choose a reason for hiding this comment

kingbbode Nov 7, 2019

Choose a reason for hiding this comment

kingbbode left a comment

Choose a reason for hiding this comment

kingbbode Oct 31, 2019 •

edited

Loading