[MVC 구현하기 - 2단계] 리비(이근희) 미션 제출합니다. #779

Libienz · 2024-09-28T11:11:01Z

안녕하세요 아루..! 2단계 첫 제출 드립니다.

초반에 구현을 많이 헤매다가 결국 힌트를 보고 따라갔는데요~ 그래서 학습이 조금 얕았을 까 걱정이에요
이번에도 리뷰 잘 해주시면 학습에 도움 많이 될 것 같습니다!

감사합니다 🙇🏻‍♂️

…vlet으로 리팩터링

donghoony

안녕하세요 리비 😄
2단계 잘 작성해주셨으나 /register GET 요청에서 오류가 발생합니다! 한 번 확인해주시고 다시 리뷰 요청 주세요~

몇 가지 코멘트도 함께 달아둡니다. 아자! 🔥

donghoony · 2024-09-28T11:46:04Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/ControllerScanner.java

 import org.reflections.Reflections;

 public class ControllerScanner {

    private final Object[] basePackage;
-    private final Map<Class<?>, Object> controllers = new ConcurrentHashMap();
+    private final Map<Class<?>, Object> controllers = new HashMap<>();


Parallelize Component Scanning to Improve Spring Startup Time [SPR-9887] spring-projects/spring-framework#14520

(스프링에서 병렬적으로 빈을 등록하면 어떨까? 에 대한 이슈도 10년 전에 올라왔었습니다 ㅎㅎ)

많은 빈을 운용하는 경우 Spring 애플리케이션의 시작 시간이 점점 느려지고 있어 이슈를 열었던 사람이 있군요!
이슈 생성자가 병렬 처리를 통해 Spring의 컴포넌트 스캔과 빈 초기화 작업을 개선할 필요가 있다고 제안한 것 확인했습니다.

현재 스프링에서 병렬적으로 빈을 등록하지는 않고 백그라운드 작업등을 제공하고 있는 것으로 확인했어요~
좋은 코멘트 감사합니다 🙇🏻‍♂️

donghoony · 2024-09-28T11:46:53Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/ControllerScanner.java

        Reflections reflections = new Reflections(basePackage);
-        Set<Class<?>> annotatedControllerTypes = reflections.getTypesAnnotatedWith(Controller.class);
-        annotatedControllerTypes.forEach(this::addController);
+        reflections.getTypesAnnotatedWith(CONTROLLER_ANNOTATION)


Controller.class를 상수로 뽑은 이유는 무엇인가요? 한 곳에서밖에 사용하지 않고, 매직 넘버도 아니지 않은가요?

어떤 대상을 스캔하는지 클래스 상단에서 보이면 좋겠다고 생각했어요..!
이런 측면에서는 매직넘버라고 저는 생각했습니다~

donghoony · 2024-09-28T11:47:53Z

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

+        log.debug("Request Mapping Uri : {}", request.getRequestURI());
+        return controllers.get(request.getRequestURI());


로그와 get을 할 때 모두 쓰인다면 변수로 뽑는 게 가독성에는 좋겠네요!

합리적이라고 생각해요! 반영했습니다~

donghoony · 2024-09-28T11:50:00Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/HandlerMappingRegistry.java

+                .filter(hm -> hm.getHandler(httpServletRequest) != null)
+                .map(hm -> hm.getHandler(httpServletRequest))


get을 두 번 해오네요!
아래와 같은 형식은 어떨까요?

Suggested change

.filter(hm -> hm.getHandler(httpServletRequest) != null)

.map(hm -> hm.getHandler(httpServletRequest))

.map(hm -> hm.getHandler(httpServletRequest))

.filter(Objects::nonNull)

오 생각하지 못하던 처리법입니다.

훨씬 깔끔하네요~ 반영했습니다!

donghoony · 2024-09-28T12:04:35Z

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

(생각해볼 점)
DispatcherServlet은 우리가 mvc 프레임워크에서 담당하는 것으로 알고 있죠. 왜 지금은 해당 파일을 프레임워크 패키지가 아니라, 웹 개발자가 다루는 패키지에 두었을까요?

결국 웹개발자가 만들고 있는 것은 DispatcherServlet이라고 생각합니다~
저희가 만든 DispatcherServlet이 톰캣의 서블릿 컨테이너에 배포됨으로써 웹 서버가 동작하는 것이죠.

이와 같은 흐름으로 볼 때 DispatcherServlet을 클라이언트가 만드는 것이 위화감 없다는 것이 제 생각이에요!

다만 스프링에서 만든 mvc프레임워크는 저희의 코드로부터 DispatcherServlet을 만드는 과정을 추상화해놓았습니다.
때문에 지금까지 저희가 이와 같은 형태의 코드를 만나보지 못했던 것일테죠!

결론

어쨌든 웹 개발자가 만드는 것은 DispatcherServlet이다!

다만 DispatcherServlet을 만드는 과정은 잘~ 만든 MVC 프레임워크에서 추상화되어 있다.

아직 추상화가 부족한 현재의 미션 코드에서 DispatcherServlet을 만드는 과정이 app 모듈에 있는 것은 어색하지 않다~

제 결론은 이런데 어색한 부분이 있다면 아루가 코멘트 더해주시면 감사하겠습니다~

거의 일치합니다 👍🏻 웹 개발자는 자신이 비즈니스 로직이 아닌 부분까지 구현하고 싶지 않았고, 그래서 DispatcherServlet을 만들어 모든 처리를 위임했다고 볼 수도 있겠네요. 아직은 제어 권한이 웹 개발자에게 있다는 이야기군요! 💯

donghoony · 2024-09-28T14:53:28Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/ControllerScanner.java

@@ -33,6 +36,6 @@ private void addController(Class<?> annotatedControllerType) {
    }

    public Map<Class<?>, Object> getControllers() {
-        return controllers;
+        return Collections.unmodifiableMap(controllers);


donghoony · 2024-09-28T14:54:28Z

mvc/src/main/java/com/interface21/webmvc/servlet/mvc/tobe/adapter/HandlerAdapterRegistry.java

+        return handlerAdapters.stream()
+                .filter(handlerAdapter -> handlerAdapter.supports(handler))
+                .findFirst()
+                .orElseThrow(() -> new NoSuchElementException("핸들러를 처리할 수 있는 Adapter를 찾지 못했습니다"));


Exception은 언제 일어나야 할까요? 핸들러어댑터를 등록하고 핸들러를 찾아오는 객체와 같은 일급 컬렉션이 존재하지만, 이 친구가 핸들러를 못 찾아 직접 예외를 터뜨리는 게 나을지, 그 책임을 사용하는 객체에게 위임하는 것이 나을지도 생각해볼 만한 점이겠네요!

재밌는 관점이네요~

저는 핸들러를 찾는 쪽에서 직접 예외를 터뜨리는 게 낫다고 생각합니다.
자신이 보유하고 있는 핸들러를 아는 객체가 이런거 없어요~라고 판단하는 편이 자연스럽기 때문이에요.

그걸 호출하는 쪽에서 null을 잡는다면 객체의 책임이 분산된 결과 아닐까요!?

아루의 구현도 확인했는데요~ 아루는 저와 다른 생각을 하고 계신 것 같아요..! (추가적으로 스프링도 아루와 비슷한 생각을 한 것 같아요!)
아루의 생각도 들려주실 수 있을까요??

ㅎㅎㅎ 저는 XXXRegistry를 하나의 컬렉션으로 보았습니다. 실제 자바 Collection들의 명세를 살펴보면 내부 값을 가져오는 메서드가 @Nullable한 것을 확인할 수 있어요. HandlerAdapter를 저장하고, 반환하는 그 자체가 목표를 완성한 것이라고 보고, 더 이상의 책임을 주지 않아도 좋지 않을까? 라는 생각을 하게 되었어요.

그걸 호출하는 쪽에서 null을 잡는다면 객체의 책임이 분산된 결과 아닐까요!?

저는 개인적으로 객체 스스로간의 응집도를 조금 내려두고, DispatcherServlet을 조금 절차적으로 가져가면서 위에서 아래로 흐름을 읽기 쉽도록 하는 데에 의미를 두었어요. null을 받는다면 DispatcherServlet에서 적절하게 처리하고 (핸들러가 없는 경우 404를 담아준다는 등) 뷰도 직접 받아서 렌더하는 방식으로요. 객체지향과는 조금 멀 수 있지만, 코드를 유지보수한다는 점에서는 DispatcherServlet만을 확인하면 알 수 있기 때문에 괜찮지 않을까? 라고 보았어요.

아루가 말씀하신 것처럼 하나의 메서드를 읽기 위해 여러 클래스의 명세를 참고해야 한다면 유지보수 비용이 있을 수 있겠습니다..
아루는 하나의 메서드에서 위에서 아래로 이어지는 흐름을 중요시 하신 것 같네요!

다만 아직까지 저는 null 여부에 따른 분기를 선호하진 않아서 현행 유지하고자 합니다.
자바의 컬렉션과 스프링 진영에 맞서는 느낌이라 영 찝찝하긴 한데요..! 우리 모두가 하나의 정답을 따라야 하는 것은 아니니까요~

donghoony · 2024-09-28T14:55:37Z

mvc/src/test/java/com/interface21/webmvc/servlet/mvc/tobe/ControllerScannerTest.java

+    @DisplayName("컨트롤러가 unmodifiableMap으로 반환되는지 확인")
+    @Test
+    void controllersReturnedAsUnmodifiableMap() {
+        Map<Class<?>, Object> controllers = controllerScanner.getControllers();
+        assertThatThrownBy(() -> controllers.put(Object.class, new Object()))
+                .isInstanceOf(UnsupportedOperationException.class);
+    }


이 부분은 테스트가 필요할까요? UnmodifiableCollections를 테스트하는 것은 아닐까요?

아 다르고 어 다른 부분이라고 생각해요.

DisplayName을 다음처럼 수정한다면 충분히 테스트 해볼만 한 부분 아닐까요?
@DisplayName("컨트롤러스캐너에서 반환한 컨트롤러 정보를 변경하려고 한다면 예외가 발생한다")

객체의 명세니까 테스트하기에 충분하다는 개인적인 생각입니다~

donghoony · 2024-09-28T14:56:24Z

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

+            ModelAndView modelAndView = handlerAdapter.handle(request, response, handler);
+            modelAndView.render(request, response);


modelAndView가 null일 가능성이 존재할까요?

오..
null일 가능성이 존재합니다. handle메서드가 반환하는 것이 절대적으로 null이 아니라고는 판단할 수 없으니까요.

다만 null인 경우는 예외적인 상황인 듯 해요.
Null인 경우의 분기를 통해 명세로써 명확히 드러내라는 코멘트로 해석했는데요..! 현재의 형식대로 null을 rendering하려고 하는 경우 발생하는 예외를 잡는 것이 더 괜찮은 처리일 수 있지 않을까요?

언제 modelAndView가 null이 될 수 있을까요?

프레임워크를 처음부터 사용하지 않았다면, 그러한 레거시 코드는 내부 핸들러에서 Request, Response를 모두 처리할 것이라고 생각해요. 이 경우에는 modelAndView를 리턴할 필요가 있을까요?

저는 null이 돌아오는 상황을 예외 상황이라고 보지 않았습니다 🥴

으아 ㅋㅋㅋㅋ 머리가 아프군요.
하지만 아루의 코멘트 이제는 이해했습니다.

제가 생각이 짧았네요
말씀주신 것처럼 레거시 코드는 Request, Response코드를 직접 처리할 수 있고 이 경우에는 ModelAndView가 충분히 null일 수 있습니다.

이러한 맥락에서는 null이 들어오는 상황도 예외가 아닙니다.

modelAndView가 null일 수 있음을 고려해서요..! DispatcherServlet의 흐름을 바꾸어보았습니다.

개선 합리적이었는지 살펴봐주세요 🙇🏻‍♂️

donghoony · 2024-09-28T14:59:38Z

mvc/src/main/java/com/interface21/webmvc/servlet/ModelAndView.java

+        try {
+            view.render(model, httpServletRequest, httpServletResponse);
+        } catch (Exception e) {
+            throw new RuntimeException(e);
+        }
+    }


View가 null일 가능성은 없을까요?

ModelAndView에게 렌더링할 책임이 있을까요? ModelAndView의 책임은 어디까지일까요?

예외 처리가 불명확하게 일어나고 있네요 🥹

null일 가능성이 있다고 해서, 생성 과정에서 예외 처리를 하라는 의도가 아닙니다. 어디까지가 객체의 역할인지, 그 이후는 누가 책임져야하는지를 생각해 보면 좋겠어요. 이 객체를 사용하는 DispatcherServlet이 일정 부분을 덜어가도 좋지 않을까요?

View가 null일 가능성은 없을까요?

생성 당시에 막지 않는 현재의 구현 상 null일 가능성이 있습니다..!

ModelAndView에게 렌더링할 책임이 있을까요? ModelAndView의 책임은 어디까지일까요?

ModelAndView는 model과 view를 합성하는 객체라고 생각해요. view가 렌더링에 책임이 있다면 그것을 합성하고 있는 ModelAndView도 렌더링에 책임이 있다고 생각해요! 어느 객체를 감싸는 객체는 public api로 자신이 보유한 객체의 기능을 제공할 수 있다는 것이 제 생각입니다. 그렇지 않다면 불필요한 게터 메서드만 추가되니까요

예외 처리가 불명확하게 일어나고 있네요 🥹

바로 위의 코멘트와 비슷한 맥락입니다. 만약 null이라면 예외가 발생할 것이고 해당 예외는 ServletException으로 rethrown되게 됩니다. null인 경우의 분기가 try-catch로 적절히 되고 있다라는 것이 제 생각입니다.

전반적으로 안전한 예외처리에 대한 코멘트가 이해가 잘 가지 않아요..!
아루의 코멘트가 잘못되었다는 것이 아니라 제가 특히 약한 부분이 예외처리인 것 같다는 말입니다.

현재와 같은 방식의 예외처리가 가지는 맹점, 개선 했을 때 얻을 수 있는 이점을 기반으로 조금 더 자세한 코멘트 주실 수 있을까요..?
학습 방향을 올바로 잡고 더 좋은 코드 작성해보고 싶습니다..!

ModelAndView는 단순 요청-응답 간에 생성되는 모델과 뷰를 묶어놓은 것으로 보았어요. 그 자체로 책임을 다했다고 보았습니다 ㅎㅎ Render할 책임이 있다는 것은 곧 해당 예외를 처리할 책임도 생길 수 있다는 것인데요, View에서 ServletException이 발생하는 건 갸우뚱해져서요. 그렇다고 RuntimeException을 던지기에도 너무 큰 범위라고 생각하고요. 그래서 상위 호출하는 친구에게 책임을 넘겼습니다.

혹시 기억나시나요? 3월 초에 진행했던 좋은 코드, 좋은 예외 처리 강의 중에 한 내용인데요, 예외를 마주하는 방식은 다음과 같은 네 가지 방법이 있었습니다 (잠시 그때의 필기를 보며...)

복구: 예외 상황 발생 시 재시도 회피: 외부에 예외 처리를 100% 위임 (말씀하신 대로 응집도가 떨어질 수도 있어요) 무시: 내부적으로 꿀꺽 예외 상황을 삼켜버려 외부에서 알지 못합니다. 회피보다 책임감이 떨어지겠네요 전환: 회피와 비슷하나, 추상화 수준에 맞는 다른 예외로 변환해 던지기

이거 다시 봐도 좋은 예시들이네요... 이곳에서 한 번 더 학습하셔도 좋은 내용이라고 생각해요. 저도 한 번 훑어보고 왔습니다 😋

중요한 건, 네 가지 방법 모두 의도가 명확하다면 훌륭한 코드가 될 수 있다는 점이예요. 의도를 잘 전달할 수 있는 코드를 작성하는 데에 초점을 맞춰간다면 다른 사람이 보기 좋은 코드가 될 수 있을 겁니다 👍🏻

결국.... modelAndView의 책임을 더 좁히기로 결정했습니다.

Render할 책임이 있다는 것은 곧 해당 예외를 처리할 책임도 생길 수 있다는 것인데요

이게 제일 와닿았네요.
modelAndView의 책임을 좁게 �잡았을 때 유리한 지점이 훨씬 큰 것 같습니다.
개선해보았는데요~ 적절한 개선이었는지 확인해주시면 감사하겠습니다 🙇🏻‍♂️

Libienz · 2024-09-29T07:56:09Z

안녕하세요 아루 🔥

매번 양질의 코멘트 감사합니다 😆
이번 리뷰에서는 특히 아루의 생각과 제 생각이 다른 부분이 많이 있었던 것 같아요..!
그래서 우선은 반영하지 않은 리뷰도 꽤 있답니다.

아루의 리뷰를 반영하지 않겠다는 뜻이 아니에요!
아루와 이야기해보고 결론을 내려보고 싶어 우선은 반영하지 않은 것이라 이해해주시면 감사하겠습니다 👍

코멘트 밑에 담백하게 제 생각 담아보았는데 아루의 의견도 들려주시면 학습에 많은 도움 될 것 같아요.
이번 리뷰도 잘 부탁드립니다~

donghoony

안녕하세요 리비! 적절하게 잘 반영해주셨습니다. 반영하지 않은 사항들에 대해 제 생각을 더 덧붙였으니, 혹시나 생각이 바뀌셨거나 추가로 토론하고 싶은 사항이 있다면 이번에 반영해주세요! 다음 리뷰 요청 때에는 머지하겠습니다. 고생 많았어요~! 🥹

donghoony · 2024-09-29T12:01:28Z

app/src/main/java/com/techcourse/controller/RegisterController.java

+    @RequestMapping(value = "/register", method = RequestMethod.GET)
+    public ModelAndView registerPage(final HttpServletRequest req, HttpServletResponse res) {
+        JspView jspView = new JspView(REGISTER_JSP);
+        return new ModelAndView(jspView);
+    }


해당 코드가 들어오면서 필요없어진 RegisterViewController는 이제 제거해도 되겠네요!

req에는 final이 있는데 res에는 없네요!

Libienz · 2024-09-29T16:11:29Z

안녕하세요 아루 🔥
아루에게 설득되어 반영한 코멘트들이 많아요 ㅋㅋㅋ

개선이 합리적이었는지 검토해주시면 감사하겠습니다 👍
이번 리뷰도 잘 부탁드려요~

donghoony

오래 달려오느라 수고했습니다! 이번 단계에서 학습할 것은 모두 가져간 것 같아 저도 뿌듯하네요 😁 다음 단계에서 봅시다! ✌🏻

Libienz added 22 commits September 28, 2024 15:23

refactor: controllers 불변 맵을 반환하도록 수정

a63c0b4

refactor: 불필요한 ConcurrentMap HashMap으로 변경

72d2c84

refactor: 의미 있는 상수 변수 추출 개선 및 inline 개선

a7b85bf

feat: HandlerMapping 타입 인터페이스 정의 및 AnnotationHandlerMapping 클래스 타입 정의

b88810f

feat: ManualHandlerMapping이 HandlerMapping 인터페이스를 구현하도록 수정

396230b

feat: HandlerMappingRegistry 구현

7afb900

feat: DispatcherServlet이 복수의 HandlerMapping을 처리하도록 구현

cfb38ea

feat: HandlerAdapter 타입 선언

9c2d0ca

feat: RequestMappingHandlerAdapter 구현

a45941e

feat: ManualHandlerAdapter 구현

c00a55e

feat: HandlerAdapterRegistry 구현

b872375

temp:

e708747

feat: HandlerMapping 인터페이스에 initialize 추가

60da14b

feat: HandlerAdapterRegistry, HandlerMappingRegistry 기반 DispatcherSer…

24b4318

…vlet으로 리팩터링

feat: RegisterController 어노테이션 기반으로 구현 변경

16f7583

refactor: HandlerMapping정보를 가져오는 방식 추상화

973cb28

test: HandlerAdapterRegistryTest 작성

89e7045

test: ManualHandlerAdapterTest 작성

92dc9ee

test: RequestMappingHandlerAdapterTest 작성

e572cfc

test: HandlerMappingRegistryTest 작성

a1fe327

test: ControllerScannerTest 불변 테스트 추가 작성

f1705f1

test: 실패하는 테스트 수정

c037eaf

donghoony assigned Libienz Sep 28, 2024

donghoony self-requested a review September 28, 2024 11:42

donghoony requested changes Sep 28, 2024

View reviewed changes

Libienz added 5 commits September 29, 2024 15:13

refactor: 여러 곳에서 사용되는 속성 변수 추출 개선

0e4f6f3

refactor: Stream 처리에서 반복적인 get 호출 제거

636b21f

feat: DispatcherServlet 예외 처리 로그 추가

84e7424

test: DisplayName 수정

3ea2fba

refactor: logging과 rethrow를 함께 하지 않도록 DispatcherServlet 메서드 수정

7575bb6

Libienz added 2 commits September 29, 2024 16:43

refactor: 불필요한 try-catch 제거 개선

df3008c

feat: RegisterController 페이지 반환 기능 구현

a9de050

donghoony requested changes Sep 29, 2024

View reviewed changes

Libienz added 5 commits September 30, 2024 00:42

refactor: 컨트롤러 스캔 대상을 Application 클래스로 제한

6258284

refactor: modelAndView가 null인 경우의 분기를 처리하도록 DispatcherServlet로직 개선

723f5c4

refactor: modelAndView 책임을 모델과 뷰를 묶어놓는 전달 객체로 한정

f159ffe

refactor: 사용하지 않는 RegisterViewController 제거 개선

7ed1d12

refactor: 메서드 파라미터 final 키워드 사용 통일 개선

fea477e

donghoony approved these changes Sep 30, 2024

View reviewed changes

donghoony merged commit 5362b4d into woowacourse:libienz Sep 30, 2024

		log.debug("Request Mapping Uri : {}", request.getRequestURI());
		return controllers.get(request.getRequestURI());

		.filter(hm -> hm.getHandler(httpServletRequest) != null)
		.map(hm -> hm.getHandler(httpServletRequest))

		ModelAndView modelAndView = handlerAdapter.handle(request, response, handler);
		modelAndView.render(request, response);

[MVC 구현하기 - 2단계] 리비(이근희) 미션 제출합니다. #779

[MVC 구현하기 - 2단계] 리비(이근희) 미션 제출합니다. #779

Conversation

Libienz commented Sep 28, 2024 • edited Loading

donghoony left a 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

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

donghoony Sep 29, 2024 • 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

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

Libienz commented Sep 29, 2024

donghoony left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Libienz commented Sep 29, 2024

donghoony left a comment • edited Loading

Choose a reason for hiding this comment

Libienz commented Sep 28, 2024 •

edited

Loading

donghoony Sep 29, 2024 •

edited

Loading

donghoony left a comment •

edited

Loading