[@MVC 구현하기 - 3단계] 리건(이한길) 미션 제출합니다. by hangillee · Pull Request #810 · woowacourse/java-mvc

hangillee · 2024-09-29T15:41:27Z

안녕하세요, 에버! 👋
폭풍 같았던 데모데이가 지나가고 조금 여유로워졌네요..
에버에게 본의 아니게 리뷰를 재촉한 것 같아 죄송합니다..🥲

할일이 너무 많아서 조급해진 감이 없지 않아 있었던 것 같습니다.
평소라면 놓치지 않았을 코드 실수도 잦았던 것 같아요..

에버 덕분에 자잘한 실수와 생각해볼 거리가 많았던 미션이었습니다!
마지막 단계 리뷰 잘 부탁드립니다!

p.s. 최종 리뷰 이후의 변경 사항 링크입니다! 다음부터는 rebase 잊지 않겠습니다..🥲
p.s.2 최종 리뷰 이후 변경 사항 링크 👍

…키지 이동

helenason

안녕하세요 리건!

시간이 정말 빠르네요..

몇가지 리뷰 남겼으니, 자유롭게 고민 후 리뷰 요청 주세요!

전체적으로 테스트 코드가 보완되면 좋겠다는 생각을 했습니다.
시간 괜찮으시면 테스트 코드를 추가해보는 건 어떨까요 😁

추가로, 사소한 부분이지만 깃 커밋 기록이 1단계부터 누적되어 잡히는데요!
병렬로 진행하고 싶은 리건의 마음은 너무 이해되지만,
rebase 후 진행하시면 리뷰어의 입장에서 조금 더 깔끔한 커밋 기록을 확인할 수 있어 리뷰하기 더욱 편할 것 같습니다!

좋은 한 주 되세요! 이번주도 파이팅입니다~~~~~~ :) 🍀

helenason · 2024-09-29T17:51:41Z

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


-    @Override
-    public String execute(final HttpServletRequest req, final HttpServletResponse res) throws Exception {
+    @RequestMapping(value = "/login/view", method = RequestMethod.GET)


uri를 통일하고 method로 요청을 분리하는 방법도 있었을텐데, 위와 같이 /view를 추가하신 이유가 있으신가요? 😲

JSP 코드에 맞춰서 작성했습니다. JSP쪽을 수정하는게 더 낫겠네요!

helenason · 2024-09-29T17:54:09Z

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

+
+    @RequestMapping(value = "/", method = RequestMethod.GET)
+    public ModelAndView index(final HttpServletRequest request, final HttpServletResponse response) {
+        return new ModelAndView(new JspView("/index.jsp"));


들어오는 모든 요청에 대해, 같은 파일임에도 JspView 인스턴스를 새롭게 생성하면 중복으로 불필요하게 메모리를 차지할 것 같은데요. 이를 방지하는 방법이 있을까요? 제가 이것에 대해 고민했어서 리건에게도 여쭤보고 싶었습니다!

이 상황에서는 캐싱이나 View 클래스에 정적 필드를 두어 활용하는 것도 좋을 것 같습니다. 제 생각에도 매 요청마다 View 인스턴스를 새로 만드는 건 좋지 않은 것 같네요!

저는 캐싱을 통해 개선했는데, View 클래스에 정적 필드를 두는 리건의 방법도 좋네요! 배워갑니다 👍

helenason · 2024-09-29T18:03:43Z

mvc/src/main/java/com/interface21/webmvc/servlet/view/JsonView.java

+    public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response)
+            throws Exception {
+        ObjectMapper objectMapper = new ObjectMapper();
+        String body = objectMapper.writeValueAsString(model);
+
+        response.setContentType(MediaType.APPLICATION_JSON_UTF8_VALUE);
+        response.getWriter().write(body);


model에 데이터가 1개면 값을 그대로 반환하고 2개 이상이면 Map 형태 그대로 JSON으로 변환해서 반환한다.

위 요구사항을 만족시켜주세요! 😊

앗..! 놓쳤네요.. 고쳐보겠습니다!

helenason · 2024-09-29T18:04:47Z

mvc/src/main/java/com/interface21/webmvc/servlet/view/JsonView.java

+    public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response)
+            throws Exception {
+        ObjectMapper objectMapper = new ObjectMapper();
+        String body = objectMapper.writeValueAsString(model);
+
+        response.setContentType(MediaType.APPLICATION_JSON_UTF8_VALUE);
+        response.getWriter().write(body);


사용이 끝난 writer는 종료시켜주는 것이 어떨까요?!

지적 감사합니다! 미처 생각하지 못했네요..

helenason · 2024-09-29T18:08:34Z

mvc/src/test/java/com/interface21/webmvc/servlet/HandlerMappingTest.java

+    @Test
+    @DisplayName("매핑은 HTTP 요청을 처리할 핸들러 목록을 초기화한다.")
+    void getHandler() throws NoSuchFieldException, IllegalAccessException {
+        Field filed = handlerMapping.getClass().getDeclaredField("handlerExecutions");


사소한 오타가 있네요! 🧐

helenason · 2024-09-29T18:11:37Z

mvc/src/test/java/com/interface21/webmvc/servlet/HandlerMappingTest.java

+
+        final var handlerExecutions = (Map<HandlerKey, HandlerExecution>) filed.get(handlerMapping);
+
+        assertThat(handlerExecutions.keySet()).hasSize(12);


keySet() 메서드가 없어도 Map의 사이즈를 검증할 수 있기 때문에, 위 코드에서의 keySet 호출은 불필요해보입니다!

helenason · 2024-09-29T18:12:09Z

mvc/src/test/java/com/interface21/webmvc/servlet/HandlerMappingTest.java

+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+class HandlerMappingTest {


아래 테스트는 HandlerMapping보다는 AnnotationHandlerMapping 클래스를 테스트하는 코드라고 생각되는데, 리건은 어떻게 생각하시나요?

제 생각에도 HandlerMapping보다는 AnnotationHandlerMapping에 대한 테스트에 가까워보이긴 합니다. 인터페이스에 대한 테스트 보다는 구현 클래스에 대한 테스트로 전환해놓겠습니다!

helenason · 2024-09-29T18:15:52Z

mvc/src/test/java/com/interface21/webmvc/servlet/HandlerAdapterTest.java

 import org.junit.jupiter.api.Test;
-import samples.TestSimpleController;

 class HandlerAdapterTest {


Legacy 어댑터가 사라졌기 때문에, 현재는 List로 어댑터를 관리할 필요가 없어보여요! 현재는 AnnotationHandlerAdapterTest로 작성하는 것이 좋아보이는데, 리건의 생각이 궁금하네요!

동의합니다. 위의 코멘트와 마찬가지로 구현체 테스트로 이동하겠습니다!

helenason · 2024-09-29T18:18:00Z

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

@@ -1,4 +1,4 @@
-package com.techcourse;
+package com.interface21.webmvc.servlet.mvc;


mvc 모듈 전체적으로 어떤 기준으로 클래스의 위치를 선정하셨는지 궁금합니다!

기본적으로 Spring의 클래스 구조를 따랐습니다!
추가적으로 인터페이스와 같은 추상화 계층은 상위 패키지에, 이를 구현한 구현체들은 하위 패키지로 세분화 하였습니다.

helenason

리건 안녕하세요!

간헐적으로 깨지는 테스트가 하나 있어 RC 드립니다.

추가된 학습테스트에 대한 리뷰도 남겼으니 확인해주세요 ☺️

아마 시간상 마지막 RC가 될 것 같네요 :)

마지막까지 화이팅입니다!

helenason · 2024-10-03T07:29:32Z

mvc/src/test/java/com/interface21/webmvc/servlet/view/JsonViewTest.java

+    @Test
+    @DisplayName("데이터가 2개 이상인 JSON 응답을 생성한다.")
+    void renderOverTwoData() throws Exception {
+        // given
+        HttpServletRequest request = mock(HttpServletRequest.class);
+        HttpServletResponse response = mock(HttpServletResponse.class);
+
+        StringWriter stringWriter = new StringWriter();
+        PrintWriter writer = new PrintWriter(stringWriter);
+        when(response.getWriter()).thenReturn(writer);
+
+        // when
+        Map<String, ?> model = Map.of("test", "value", "hello", "world");
+        jsonView.render(model, request, response);
+
+        // then
+        assertThat(stringWriter).hasToString("{\"test\":\"value\",\"hello\":\"world\"}");
+    }


위 테스트가 간헐적으로 실패하네요!

helenason · 2024-10-03T07:33:05Z

...rc/test/java/com/interface21/webmvc/servlet/mvc/annotation/AnnotationHandlerMappingTest.java

+    @Test
+    @DisplayName("매핑은 HTTP 요청을 처리할 핸들러 목록을 초기화한다.")
+    void getHandler() throws NoSuchFieldException, IllegalAccessException {
+        Field filed = handlerMapping.getClass().getDeclaredField("handlerExecutions");
+        filed.setAccessible(true);
+
+        final var handlerExecutions = (Map<HandlerKey, HandlerExecution>) filed.get(handlerMapping);
+
+        assertThat(handlerExecutions.keySet()).hasSize(12);
+    }
+


이전에 드렸던 두 리뷰가 반영되지 않은 것 같아요! 😲

#810 (comment)

#810 (comment)

helenason · 2024-10-03T07:39:38Z

study/src/test/java/di/stage3/context/DIContainer.java

+    private Object instantiateClass(final Class<?> clazz) {
+        try {
+            Constructor<?> constructor = clazz.getDeclaredConstructor();
+            constructor.setAccessible(true);


생성자의 접근제어자를 임시로 열어둔다면, 프로그램이 종료되기 전까지 열린 접근 제어자로 프로그램이 실행될 것 같아요! 인스턴스를 생성한 후 다시 닫아주는 것이 어떨까요?

+) field도 마찬가지로요!

helenason · 2024-10-03T07:41:04Z

study/src/test/java/di/stage3/context/DIContainer.java

+        } catch (NoSuchMethodException | IllegalAccessException | InstantiationException |
+                 InvocationTargetException e) {


저는 개인적으로, 코드의 가독성을 위해 위와 같이 예외의 수가 많아지는 경우 상위 클래스로 묶어 예외를 잡는 편인데, 리건은 위 방식을 선호하시는 이유가 있으신가요?

어떤 예외가 발생하는지 상세하게 알려주는 점에서 괜찮다고 생각하는 편입니다! 그래도 확실히 가독성은 떨어지는 것 같아서 각 예외 클래스의 상위 클래스인 ReflectiveOperationException으로 대체하겠습니다.

helenason · 2024-10-03T07:47:49Z

study/src/test/java/di/stage3/context/DIContainer.java

+    private void initFields(final Object bean) {
+        final Field[] fields = bean.getClass().getDeclaredFields();
+        try {
+            for (Field field : fields) {
+                initField(bean, field);
+            }
+        } catch (IllegalAccessException e) {
+            throw new IllegalArgumentException("초기화할 수 없는 필드입니다.");
+        }
+    }


객체지향 생활 체조 원칙 - 한 메서드에 오직 한 단계의 들여쓰기만 한다

depth를 1로 줄여보는 것이 어떨까요? 아래 initField 메서드도 마찬가지로요!

di 패키지에 있는 클래스들을 활용해서 고쳤습니다!

helenason · 2024-10-03T07:54:14Z

study/src/test/java/di/stage3/context/DIContainer.java

+        field.setAccessible(true);
+        final Class<?> fieldType = field.getType();
+        for (Object o : beans) {
+            if (fieldType.isAssignableFrom(o.getClass())) {


저도 제 리뷰어에게 받은 리뷰인데요. 이번 미션을 기회로 Class.isInstance vs Class.isAssignableFrom vs instanceof 차이점에 대해 학습해보아요!

helenason · 2024-10-03T07:58:27Z

study/src/test/java/di/stage4/annotations/ClassPathScanner.java

    public static Set<Class<?>> getAllClassesInPackage(final String packageName) {
-        return null;
+        Reflections reflections = new Reflections(packageName);
+        Set<Class<?>> classes = new HashSet<>();
+
+        classes.addAll(reflections.getTypesAnnotatedWith(Repository.class));
+        classes.addAll(reflections.getTypesAnnotatedWith(Service.class));
+
+        return classes;
    }


헉 저와 완전히 똑같이 구현하셨네요!

하지만 제가 리뷰 반영하면서 고민해보게 된 포인트인데요. Repository와 Service 클래스 추출 로직은 외부로 빼는 것이 맞다고 생각해요! 메서드명이 getAllClassesInPackage이기 때문에 파라미터로 받은 패키지 내부의 모든 클래스를 반환하는 역할을 갖는 것이 맞다고 생각합니다.

Reflections의 Scanners.SubTypes.filterResultsBy(); 메소드를 활용해서 수정했습니다!

helenason · 2024-10-03T08:01:04Z

study/src/test/java/di/stage4/annotations/DIContainer.java

stage4의 DIContainer 클래스에 대해서도 전체적으로 stage3의 DIContainer 클래스에 남겨드린 리뷰 반영해보시면 좋을 것 같아요 :)

helenason

수고하셨습니다 리건!

이만 머지하도록 할게요 ~~~~

다음 단계도 파이팅입니다 👊

hangillee added 30 commits September 16, 2024 20:06

test: 학습 테스트 풀이

7ef4eae

feat: Reflection API를 활용한 컨트롤러 요청 매핑 구현

7b2c2ba

refactor: View 렌더링 책임을 적절하게 위임

58d41e4

feat: method가 주어지지 않았을 때 모든 메소드를 매핑하는 기능 구현

4159ef1

refactor: 잘못된 HTTP method에 대한 예외 처리

63bc3f1

refactor: 코드 가독성 개선

1018120

test: 예외 상황에 대한 테스트 작성

21cc05a

test: Servlet 학습 테스트 풀이

3f6f44e

refactor: 정적 팩토리 메소드 리팩토링 및 여러 method가 주어졌을 경우 모두 매핑해주도록 변경

825c164

refactor: 코드 가독성 개선 및 HandlerKey 생성 로직 변경

7ca5cc1

test: 누락된 테스트 작성 및 코드 개

a343ca9

refactor: 인터페이스 기반 컨트롤러와 어노테이션 기반 컨트롤러 모두 지원하도록 리팩토링

7ff1417

refactor: @MVC 방식을 활용하도록 리팩토링

35045e4

feat: 모든 핸들러의 반환값을 ModelAndView로 통일 시키는 어댑터 구현

b5523bc

feat: 어댑터를 전역으로 관리하는 저장소 역할을 하는 레지스트리 클래스 구현

8b12320

feat: 핸들러 매핑 객체 추상화 및 리팩토링

a173d8b

feat: 핸들러 매핑 객체를 전역으로 관리하는 저장소 역할 레지스트리 객체 구현

8ec48e7

test: 어노테이션 기반 핸들러 매핑 테스트 의존성 변경

604659d

test: HandlerAdapter 테스트 작성

5a1d35f

fix: git conflict 해결

c91553d

feat: 컨트롤러 탐색 및 인스턴스화 역할 분리

076772f

refactor: 리뷰 반영

b9c6c69

feat: Json 응답도 가능하도록 만드는 JsonView 구현

4aa26e8

refactor: Legacy MVC에서 어노테이션 기반 MVC로 리팩토링

1e4ec1b

refactor: 불필요한 Legacy MVC 코드 제거

4dc4e7f

refactor: 외부에서 사용되지 않는 상수 필드 private으로 전환

f9a2c24

test: 핸들러 어댑터 테스트 이름 수정

8a6589e

test: 핸들러 매핑 테스트 작성

c982e01

chore: 반환하는 View name 수정

3b79442

refactor: 컨트롤러 스캐닝을 하지 못하는 오류를 수정하기 위해 DispatcherServletInitializer 패…

2654c0d

…키지 이동

hangillee requested a review from helenason September 29, 2024 15:41

hangillee self-assigned this Sep 29, 2024

fix: git conflict 해결

ec66853

helenason requested changes Sep 29, 2024

View reviewed changes

hangillee added 9 commits September 30, 2024 17:22

test: 3단계 학습 테스트 풀이

cdd9339

refactor: API URI 수정 및 View 정적화

d08ae17

refactor: JSON 반환 형식 요구사항 반영

9f8d559

chore: 불필요한 개행 제거

c917627

refactor: 적절한 클래스로 테스트 코드 이동

0cb2a1b

chore: 불필요한 개행 제거

5ad8d7e

chore: 의존관계 수정

f2ce515

test: 컨트롤러 스캐너 테스트 작성

7182d4b

test: JsonView 테스트 작성

043f08d

helenason requested changes Oct 3, 2024

View reviewed changes

hangillee added 4 commits October 4, 2024 17:07

refactor: 메소드 indent 개선

e9336ad

refactor: 외부에서 스캔 대상 지정하도록 개선

62a8347

test: 오타 및 검증 대상 수정

400c0fc

refactor: 테스트 데이터의 정합성이 보장되도록 수정

d88f858

helenason approved these changes Oct 4, 2024

View reviewed changes

helenason merged commit 3025ce6 into woowacourse:hangillee Oct 4, 2024


		final var handlerExecutions = (Map<HandlerKey, HandlerExecution>) filed.get(handlerMapping);

		assertThat(handlerExecutions.keySet()).hasSize(12);

		@@ -1,4 +1,4 @@
		package com.techcourse;
		package com.interface21.webmvc.servlet.mvc;

		} catch (NoSuchMethodException \| IllegalAccessException \| InstantiationException \|
		InvocationTargetException e) {

Conversation

hangillee commented Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

helenason left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenason Sep 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenason left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hangillee Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

helenason left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

hangillee commented Sep 29, 2024 •

edited

Loading

helenason left a comment •

edited

Loading

helenason Sep 29, 2024 •

edited

Loading

hangillee Oct 4, 2024 •

edited

Loading