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

hyojaekim · 2019-10-04T08:06:34Z

안녕하세요! 효오 입니다.

아직 부족하지만 많은 지적 부탁드리겠습니다!

먼저 PR보내놓고 계속해서 리팩토링 하겠습니다.

woowahanCU

안녕하세요! 정훈님이 개인적인 사정이 있어서 대신 리뷰남겨요~ 😀
상당부분 잘 구현하셨는데요. 이번 미션 진행하면서 구현한 기능들에 대한 테스트코드를 작성하지 않아 아쉽네요.
어렵더라도 작성해보는 연습을 해보시는게 좋을거 같아요.
몇가지 피드백 남겨두었으니 확인해보시고 궁금한 점 있으면 Comment남기시면 포비가 답변주실거에요 😈

woowahanCU · 2019-10-04T17:55:49Z

study/src/test/java/reflection/ReflectionTest.java

+        Field[] fields = clazz.getDeclaredFields();
+        for (Field field : fields) {
+            logger.debug("필드 이름 : {}, 접근 제어자 : {}", field.getName(), field.getModifiers());
+        }
+
+        Constructor<?>[] constructors = clazz.getDeclaredConstructors();
+        for (Constructor<?> constructor : constructors) {
+            logger.debug("생성자 이름 : {}, 접근 제어자 : {}, 파라미터 타입 : {}", constructor.getName(), constructor.getModifiers(), constructor.getParameterTypes());
+        }
+
+        Method[] methods = clazz.getDeclaredMethods();
+        for (Method method : methods) {
+            logger.debug("메서드 이름 : {}, 접근 제어자 : {}, 파라미터 타입 : {}", method.getName(), method.getModifiers(), method.getParameterTypes());
+        }


미션이 출력을 하는 것들 이었지만, 값을 비교하는 테스트를 작성하는 것이 더 좋다고 생각합니다.
테스트 코드를 작성하는 것은 리팩토링하는 과정을 돕기도 하지만, 배포 파이프라인에서도 의미를 갖는다고 생각하는데요.
단순히 출력하는 테스트는 리팩토링 과정에서는 사용자의 확인을 필요로하고, 배포 파이프라인에서는 별다른 의미를 갖지 않게 된다고 생각합니다.

woowahanCU · 2019-10-04T18:57:24Z

nextstep-mvc/src/main/java/nextstep/mvc/tobe/AnnotationHandlerMapping.java

+        addController("/users", RequestMethod.POST);
+        addController("/users", RequestMethod.GET);
+        addController("/users/loginForm", RequestMethod.GET);
+        addController("/users/login", RequestMethod.POST);
+        addController("/users/logout", RequestMethod.GET);


Reflection을 적극적으로 사용하면 RequestMethod를 인자로 받는 부분을 없앨 수 있지 않을까요
그리고 현재는 addController 메서드에서 매번 ControllerScanner.scan 메서드를 호출하고 있네요.
아래와 같이 Controller별로 Annotation이 붙은 메서드만 찾는 것은 어떨까요

Method[] methods = Arrays.stream(controller.getDeclaredMethods()) .filter(it -> it.isAnnotationPresent(RequestMapping.class)) .toArray(Method[]::new);

다시 생각해보니 인자도 받을 필요가 없었고, 매번 ControllerScanner의 메소드를 호출할 필요도 없었네요! 좋은 피드백 감사합니다!

궁금한게 있습니다. 저는 Controller에서 Mapping(@GetMapping, @RequestMapping....) 관련 어노테이션이 붙은 메서드 접근 제어자를 public으로 두고,
로직을 분리할 때 분리한 메서드 접근 제어자를 대부분 private로 선언 했었습니다.

그래서 접근 불가능한 메서드를 가져오지 않아도 괜찮다고 생각해서 getMethods를 사용했는데
피드백 주신 코드에서 getDeclaredMethods()를 사용한 이유가 있을까요?

아니요. getMethods()를 사용하시면 됩니다 ㅎㅎ;

woowahanCU · 2019-10-04T18:58:05Z

nextstep-mvc/src/main/java/nextstep/mvc/tobe/AnnotationHandlerMapping.java

    }

+    @Override
    public HandlerExecution getHandler(HttpServletRequest request) {


method 순서에 대해서 다시 한번 고민해보셔요.

https://stackoverflow.com/questions/4668218/are-there-any-java-method-ordering-conventions

woowahanCU · 2019-10-04T18:59:41Z

nextstep-mvc/src/main/java/nextstep/mvc/tobe/JspView.java

+import java.util.Map;
+
+public class JspView implements View {
+    private static final String DEFAULT_REDIRECT_PREFIX = "redirect:";


JspView와 redirect는 별개로 보이는데요. redirect의 경우 dispatch도 필요없어 보이구요
RedirectView를 별도로 작성하는 것은 어떨까요

woowahanCU · 2019-10-04T19:32:49Z

nextstep-mvc/src/main/java/nextstep/web/annotation/RequestMethod.java

-    GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, TRACE
+    GET, HEAD, POST, PUT, PATCH, DELETE, OPTIONS, TRACE;
+
+    public static RequestMethod of(String requestMethod) {


enum의 valueOf 메서드를 활용해도 되지 않을까요

woowahanCU · 2019-10-04T19:39:15Z

nextstep-mvc/src/main/java/nextstep/mvc/tobe/AnnotationHandlerMapping.java

+        if (handlerExecutions.containsKey(handlerKey)) {
+            return handlerExecutions.get(handlerKey);
+        }
+        throw new IllegalArgumentException();


RequestMapping에 Method를 설정하지 않았을 시에는 요구사항에 있는 것처럼 해당 URL에 대해 모든 Method로 요청할 수 있습니다. 이 기능이 구현되지 않았네요.
그리고 특정 Method로 설정해두었을 경우 그 외의 Method로 요청시에는 405 로 응답합니다.

https://tools.ietf.org/html/rfc7231#section-6.5.5

@Target({ElementType.METHOD, ElementType.TYPE}) @Retention(RetentionPolicy.RUNTIME) public @interface RequestMapping { String value() default ""; RequestMethod[] method() default {}; }

woowahanCU · 2019-10-04T19:46:01Z

nextstep-mvc/src/main/java/nextstep/mvc/HandlerMapping.java


 public interface HandlerMapping {
    void initialize();

-    Controller getHandler(String requestUri);
+    Object getHandler(HttpServletRequest request);


HandlerExecution으로 리턴 받는다면 DispatcherServlet에서 if 분기 처리하는 상당부분, instaceof 등이 제거될 수 있겠어요

woowahanCU · 2019-10-04T19:54:21Z

slipp/src/main/java/slipp/controller/tobe/UpdateLoginController.java

+        if (user == null) {
+            request.setAttribute("loginFailed", true);
+            return new ModelAndView(new JspView("/user/login.jsp"));
+        }


아래의 코드와 중복이네요

woowahanCU · 2019-10-04T19:58:19Z

nextstep-mvc/src/main/java/nextstep/mvc/DispatcherServlet.java


 @WebServlet(name = "dispatcher", urlPatterns = "/", loadOnStartup = 1)
 public class DispatcherServlet extends HttpServlet {
    private static final long serialVersionUID = 1L;
    private static final Logger logger = LoggerFactory.getLogger(DispatcherServlet.class);
    private static final String DEFAULT_REDIRECT_PREFIX = "redirect:";

-    private HandlerMapping rm;
+    private List<HandlerMapping> handlerMappings;


woowahanCU · 2019-10-04T19:59:31Z

nextstep-mvc/src/main/java/nextstep/mvc/DispatcherServlet.java

@@ -11,40 +14,59 @@
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;

 @WebServlet(name = "dispatcher", urlPatterns = "/", loadOnStartup = 1)
 public class DispatcherServlet extends HttpServlet {


DispatcherServlet은 HandlerMapping 인터페이스의 getHandler 메서드의 리턴타입 변경 후 구조적으로 상당부분 개선이 될 것으로 보입니다.

리턴 타입을 HandlerExecution 으로 바꾸면서, 여러번 코드를 작성했다 지웠다를 반복했습니다.
이러한 과정을 겪으니 확실히 코드가 전보다 깔끔해지고, 이해도가 높아졌습니다.

리턴 타입만 바꿨는데 DispatcherServlet에서 분기 처리하는 부분이 없어져서 전보다 깔끔하네요!
좋은 피드백 감사합니다!

- 매개변수 제거 - 매번 ControllerScanner 메서드 호출 -> 1번 호출

- UpdateLoginController 상수 제거 - JspView redirect 로직 제거

- Object -> HandlerExecution - method.invoke(...) -> 리턴 타입 String -> ModelAndView로 변경 - DispatcherServlet 불필요한 메서드 제거

hyojaekim · 2019-10-07T07:49:55Z

테스트 코드는 PR 보내고 추가하겠습니다!

woowahanCU

피드백 반영 잘해주셨네요! 👍
다음 단계 진행하시면 됩니다~ :)

woowahanCU · 2019-10-07T12:14:11Z

nextstep-mvc/src/main/java/nextstep/mvc/tobe/AnnotationHandlerMapping.java

+        ControllerScanner.scan(basePackage, Controller.class).forEach(controller ->
+                Arrays.stream(controller.getMethods())
+                        .filter(method -> method.isAnnotationPresent(RequestMapping.class))
+                        .forEach(method -> putHandlerExecution(controller, method))


stream api의 foreach는 가급적 출력 용도로만 활용해주시기 바랍니다.
로직을 수행할 거면 반복문을 사용하시는게 객체지향적인 설계를 가능케 한다고 생각합니다.
foreach에서 로직이 수행될 경우 동시성 보장이 어려워지고 가독성이 떨어져 effective java item 46에서는 주의해서 사용하길 권하고 있습니다.

추가적으로, stream api는 지연연산을 하므로 일반적인 loop와 다르게 동작하므로 결과가 예측과 다를 수 있습니다.
https://www.popit.kr/java8-stream은-loop가-아니다/

gch01410 added 15 commits October 1, 2019 13:56

feat: 클래스 정보 출력

a938e63

feat: test로 시작하는 메소드 실행

753a31c

feat: @mytest 애노테이션 있는 메소드 실행

57f0d31

feat: private field에 값 할당

f237fc3

feat: 인자를 가진 생성자의 인스턴스 생성

28b6c54

feat: component scan 구현

2a98476

feat: 애노테이션 기반 MVC 프레임워크 구현

59fd2f9

feat: JspView 추가

dca02f6

feat: handlerMapping 클래스들의 getHandler 메서드 공통부분 추상화

70d20ff

feat: LoginForm 요청을 annotation을 통해 처리

dcd5a4c

feat: annotationHandlerMapping 클래스 추가

7fe8816

feat: ControllerScanner 추가

789d8a2

style: 컨트롤러 이름 수정

446d75b

feat: Login Post 요청을 annotation 적용

bdc1c36

feat: Logout Get 요청을 annotation 적용

cf70c43

woowahanCU suggested changes Oct 4, 2019

View reviewed changes

hyojaekim added 8 commits October 5, 2019 17:31

refactor: Login 메소드 중복된 로직 제거 및 상수 추가

00618a0

refactor: Enum valueOf 메서드 활용

b98bb19

refactor: AnnotationHandlerMapping initialize 메서드 로직 변경

cf22e88

- 매개변수 제거 - 매번 ControllerScanner 메서드 호출 -> 1번 호출

feat: RedirectView 추가

8e4dc3f

- UpdateLoginController 상수 제거 - JspView redirect 로직 제거

feat: Annotation으로 회원가입 페이지 이동

c481a63

refactor: HandlerMapping getHandler 리턴 타입 변경

7f31f0c

- Object -> HandlerExecution - method.invoke(...) -> 리턴 타입 String -> ModelAndView로 변경 - DispatcherServlet 불필요한 메서드 제거

test: 단순 출력 -> 값 비교

e25c984

feat: method 설정이 되어 있지 않으면 모든 HTTP method 지원

b0c1241

woowahanCU approved these changes Oct 7, 2019

View reviewed changes

woowahanCU merged commit dc0692d into woowacourse:hyojaekim Oct 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hyojaekim commented Oct 4, 2019 •

edited

Loading

woowahanCU left a comment

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

hyojaekim Oct 5, 2019 •

edited

Loading

woowahanCU Oct 7, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019 •

edited

Loading

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

woowahanCU Oct 4, 2019

hyojaekim Oct 6, 2019

hyojaekim commented Oct 7, 2019

woowahanCU left a comment

woowahanCU Oct 7, 2019

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

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

Conversation

hyojaekim commented Oct 4, 2019 • edited Loading

woowahanCU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyojaekim Oct 5, 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

woowahanCU Oct 4, 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

hyojaekim commented Oct 7, 2019

woowahanCU left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyojaekim commented Oct 4, 2019 •

edited

Loading

hyojaekim Oct 5, 2019 •

edited

Loading

woowahanCU Oct 4, 2019 •

edited

Loading