Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[럿고] 4단계 - HTTP 웹 서버 리팩토링 미션 제출합니다. #193

Merged
merged 5 commits into from
Nov 24, 2020

Conversation

ksy90101
Copy link

안녕하세요 홍시!
마지막 단계 리뷰 요청합니다!

멀티모듈로 적용하다 보니 예전에 만들어 놓은 ControllerMapper부분이 애매해진 상황이 와서 찾아보다가 실제 Spring처럼 RequestMapping어노테이션을 이용해 리플랙션으로 저장해놓고 컨트롤러 매핑을 해주면 된다는걸 깨닫고 구현해봤습니다.

리플랙션을 처음으로 구현하다 보니 부족한 부분이 있는데, 가감없는 리뷰 부탁드리겠습니다.

하다보니 몇가지 질문이 생겼는데 혹시 시간나시면 답변 부탁드리겠습니다~

  1. 리플랙션을 할때 Reflections reflections = new Reflections(DEFAULT_PACKAGE_SCAN);에서 디폴트 패키지를 생성하지 않으면 리플랙션이 되지 않더로고요. 그러다 보니 was모듈이 user-service 모듈에 의존이 되버렸는데 혹시 이거에 대한 다른 방법이 없을까요?

  2. 리플랙션 후 메서드를 실행하려고 하면 해당 인스턴스들이 있어야 한다고 합니다. 사실 spirng은 IoC로 가능할거 같은데 저는 아직 거기까지 구현하기가 힘들더라고요. 혹시 간단하게 구현할 방법이 있을까요?(현재는 static메서드로 null을 넣어서 실행하는 방법으로 했습니다.)

마지막 단계 리뷰 잘 부탁드리겠습니다~ 감사합니다!

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4단계 요구사항에 맞춰 모듈을 잘 분리하셨습니다! 👍
피드백 남겼으니, 확인하셔서 반영해주세요!


public class UserApplication {
public static void main(String[] args) throws Exception {
WebServer.main(args);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리플랙션을 할때 Reflections reflections = new Reflections(DEFAULT_PACKAGE_SCAN);에서 디폴트 패키지를 생성하지 않으면 리플랙션이 되지 않더로고요. 그러다 보니 was모듈이 user-service 모듈에 의존이 되버렸는데 혹시 이거에 대한 다른 방법이 없을까요?

의존성을 해결하기 위해선 의존성을 역전시켜 실행 시점에 의존값을 주입하면 됩니다.
아래와 같이 was 를 라이브러리로 형태로 변경하고 실행 시점에 필요한 파라미터를 주입받도록 바꿔보면 어떨까요?

public class UserApplication {
    public static void main(String[] args) throws Exception {
        WebServer webSever = new WebServer(설정); // Application만 알 수 있는 설정을 실행 시점에 주입
        webServer.start();
    }
}

import http.was.exception.DuplicatedMappedRequestException;

public class RequestMapper {
private static final Map<MappedRequest, Method> requestMapper = new HashMap<>();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리플랙션 후 메서드를 실행하려고 하면 해당 인스턴스들이 있어야 한다고 합니다. 사실 spirng은 IoC로 가능할거 같은데 저는 아직 거기까지 구현하기가 힘들더라고요. 혹시 간단하게 구현할 방법이 있을까요?(현재는 static메서드로 null을 넣어서 실행하는 방법으로 했습니다.)

가장 간단한 방법은 Method 와 함께 Method 의 인스턴스를 생성해서 하나의 클래스에 같이 저장하면 인스턴스를 매번 생성하지 않고도 실행할 수 있지 않을까요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오... 그렇게 어려운 방법이 아니였네요 ㅎㅎ 감사합니다 👍

private static final Map<MappedRequest, Method> requestMapper = new HashMap<>();
private static final String DEFAULT_PACKAGE_SCAN = "user/service/controller";

public static void scanRequestMappingAnnotatedMethod() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

어노테이션 방식 구현 👍

Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

럿고~ 피드백 반영을 잘 해주셨네요! 👍
몇 가지 피드백을 추가로 드렸으니, 확인하셔서 반영해주세요!

.orElseThrow(() -> new IllegalArgumentException("해당하는 인스턴스가 업습니다."));
}

public void scanRequestMappingAnnotatedMethod() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RequestMapper 는 이미 요청과 관련된 책임을 가지고 있기 때문에 컨트롤러를 찾아서 컨트롤러 인스턴스를 만드는 책임은 다른 클래스로 위임해보면 어떨까요?

was/src/main/java/http/was/webserver/RequestMapper.java Outdated Show resolved Hide resolved
Copy link

@hongsii hongsii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피드백 반영을 깔끔하게 해주셨네요! 💯
미션 진행하시느라 고생 많으셨습니다. 머지할게요!

@hongsii hongsii merged commit 5f4975c into woowacourse:ksy90101 Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants