[MVC 구현하기 - 1단계] 카피(김상혁) 미션 제출합니다. #678

tkdgur0906 · 2024-09-19T07:46:30Z

안녕하세요 켈리!
리뷰이로 만나게 되어 반갑네요~

리플렉션이 처음이라 사용하는데 약간 어려움이 있었네요!
MVC 미션동안 잘 부탁드립니다!

kelly6bf

안녕하세요 카피! 리뷰가 늦어져서 정말 죄송합니다!! 전체적으로 코드 깔끔하게 잘 구현해주신거 같아요!!

몇가지 보완하면 좋겠다 싶은 내용과 질문 위주로 리뷰 달아봤습니다! 앞으로 잘 부탁 드려요!

kelly6bf · 2024-09-21T05:55:34Z

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

+        Reflections reflections = new Reflections(basePackage);
+        Set<Class<?>> classes = reflections.getTypesAnnotatedWith(Controller.class);
+
+        for (Class<?> c : classes) {


이부분을 스트림으로 묶으면 라인을 줄일 수 있을거 같습니다! 😆

반영했습니다!

kelly6bf · 2024-09-21T05:56:50Z

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


-    public Object getHandler(final HttpServletRequest request) {
-        return null;
+    private void scanClass(Class<?> c) {


코드가 전체적으로 final을 제법 사용하는거 같은데 카피의 컨벤션일까요? 🤔 final이 붙은 파라미터도 있고 아닌 것도 있어서 통일해주시면 좋을거 같습니다!

기존에 코드를 가지고 오다보니 그대로 유지되었네요!
저는 final을 안붙이는 스타일이라 제거했습니다!

kelly6bf · 2024-09-21T05:59:57Z

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

+    }
+
+    public HandlerExecution getHandler(final HttpServletRequest request) {
+        return handlerExecutions.get(


입력된 HttpServletRequest를 기반으로 적절한 값을 Map에서 찾고있으니 이것 역시 일종의 책임이니까 테스트 코드가 필요해보이네요! AnnotationHandlerMapping에 이 책임을 주고 테스트를 작성해도 괜찮지만 필드로 존재하는 Map을 일급 컬렉션으로 만들어서 이 책임을 주는건 어떨까요? 🤔

해당 메서드에 대한 책임을 분리할 수 있겠군요!
Map에 대한 일급 컬렉션을 생성해서 책임을 분리한 뒤 테스트 코드도 작성했습니다!

kelly6bf · 2024-09-21T06:00:22Z

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

 public class HandlerExecution {

+    private final Class<?> controller;
+


불필요한 공백이네요!

kelly6bf · 2024-09-21T06:01:13Z

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

+
+    private final Method method;
+
+    public HandlerExecution(Class<?> controller, Method method) {


역시 전체적으로 메서드 인자에 final을 붙인 친구도 있고 아닌 친구도 있네요! 의도된게 아니라면 컨벤션 통일을 해주셔야 할 거 같습니다!

kelly6bf · 2024-09-21T06:04:09Z

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

+    public void render(final Map<String, ?> model, final HttpServletRequest request, final HttpServletResponse response)
+            throws Exception {
+        if (viewName.startsWith(JspView.REDIRECT_PREFIX)) {
+            response.sendRedirect(viewName.substring(JspView.REDIRECT_PREFIX.length()));


viewName에 대한 검증 말고도 값을 변형하는 로직이 존재하네요! 테스트 코드가 필요한 작업이고요! 위에 드린 코멘트랑 연결지어서 생각해보면 좋을거 같아요! 😎

같이 반영헀습니다!
의도하신바가 맞는지 확인 부탁드려요!

kelly6bf · 2024-09-21T06:04:53Z

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

        });

-        // todo
+        final var requestDispatcher = request.getRequestDispatcher(viewName);


카피는 var을 선호하시나요? 전 개인적으로 구체 타입을 사용하는걸 선호하는데 카피는 어떨지 궁금하네요!

해당 부분도 그냥 생각없이 가지고 왔던 것 같네요!
저도 var을 사용하는 것보다 구체 타입을 명시하는 것을 선호해서 통일해주었습니다!

kelly6bf · 2024-09-21T06:08:16Z

study/src/main/java/servlet/com/example/CharacterEncodingFilter.java

+    public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain)
+            throws IOException, ServletException {
        request.getServletContext().log("doFilter() 호출");
+        response.setCharacterEncoding("UTF-8");


response는 뭔가 내부 비즈니스가 끝나고 상대에게 결과를 보낼때 사용하는 객체니 비즈니스 로직이 끝나고 나갈때 처리하면 좋을거 같은데 왜 비즈니스 시작전 필터에서 response관련 설정을 해주는걸까요? 🤔

hint : java docs

KoreanServlet 내부를 보면 response에 문자열을 write하는 로직이 있는 것을 볼 수 있어요.
write할 때 스트림이 열리게 되는데, 열린 이후에는 인코딩 방식을 변경할 수 없습니다!
그래서 응답이 나갈 때 설정하는 것이 아닌 요청이 올 때 미리 설정해야하는 것 같네요!

내부 로직을 보면서 확인하긴 했는데, java docs에서는 찾지 못하였네요😅
혹시 어느 부분에 있는지 알려주실 수 있을까요?

대답하신 내용이 맞습니다! ServletResponse 문서를 보시면 관련 내용이 포함되어 있습니다!

https://docs.oracle.com/javaee/7/api/javax/servlet/ServletResponse.html

kelly6bf · 2024-09-21T06:10:00Z

study/src/test/java/servlet/com/example/ServletTest.java

        // expected를 0이 아닌 올바른 값으로 바꿔보자.
        // 예상한 결과가 나왔는가? 왜 이런 결과가 나왔을까?
-        assertThat(Integer.parseInt(response.body())).isEqualTo(0);
+        assertThat(Integer.parseInt(response.body())).isEqualTo(3);


왜 3이 나올까요? 카피의 대답은?!

/shared-counter과 매핑하는 서블릿 내부에 sharedCounter 필드가 있어요!
서블릿은 요청마다 새로 생성되는 것이 아닌 하나의 서블릿이 계속 사용되는데,
서블릿 내부 코드를 보면 요청이 올 때마다 sharedCounter를 ++하고 있어요.
그래서 요청이 올 때 마다 값이 변하게 됩니다.

서블릿은 여러 스레드에서 접근이 가능하므로 의도하지 않는 이상 인스턴스 변수를 사용하면 안될 것 같네요!

kelly6bf · 2024-09-21T06:10:17Z

study/src/test/java/servlet/com/example/ServletTest.java

        // expected를 0이 아닌 올바른 값으로 바꿔보자.
        // 예상한 결과가 나왔는가? 왜 이런 결과가 나왔을까?
-        assertThat(Integer.parseInt(response.body())).isEqualTo(0);
+        assertThat(Integer.parseInt(response.body())).isEqualTo(1);


이것도 궁금하네요! 카피의 대답으으으으은? 🤔

해당 서블릿은 인스턴스 변수가 아닌 비즈니스 로직 내의 로컬 변수를 사용하고 있어요!
그래서 여러 스레드가 서블릿에 접근하더라도 항상 새로운 로컬 변수를 사용하기 때문에 같은 값이 나옵니다!

tkdgur0906 · 2024-09-22T12:33:44Z

켈리 ~ 리뷰 반영 완료했습니다!
final, var 관련 파일 수정이 커서 변경사항은 링크를 확인하시면 좋을 것 같아요!
리뷰 잘 부탁드립니다😀

kelly6bf

카피! 미션 고생하셨습니다! 코멘트 드렸던 부분 모두 반영 해주셨으니 이만 머지 할게요! 다음 단계 진행해주세요!

tkdgur0906 added 10 commits September 18, 2024 18:13

test: Junit3TestRunner 학습 테스트 작성

8aff47b

test: Junit4TestRunner 학습 테스트 작성

4227de7

test: ReflectionTest 학습 테스트 작성

7016c1c

test: ReflectionsTest 학습 테스트 작성

dd71ae2

feat: handlerExecutions에 핸들러 등록하는 기능 구현

4e96d6a

feat: 핸들러 반환하는 기능 구현

58d12af

feat: RequestMapping method 설정 없을 시 모든 http method 지원하도록 구현

1327ff7

feat: JspView에서 뷰처리하도록 구현

a959d6c

test: ServletTest 수행

6ee8e3c

test: FilterTest 수행

c018289

kelly6bf requested changes Sep 21, 2024

View reviewed changes

tkdgur0906 added 6 commits September 22, 2024 15:21

refactor: stream으로 로직 간결화

8fee6c9

refactor: HandlerExcution Map을 일급 컬렉션으로 분리

b9d7e32

style: 불필요 공백 제거

c227954

refactor: viewName에 대한 검증 ViewName 객체로 책임 분리

f3c066f

style: final, var 컨벤션 통일

1e005f2

test: 테스트에 displayname 추가

2613438

kelly6bf approved these changes Sep 23, 2024

View reviewed changes

kelly6bf merged commit c991012 into woowacourse:tkdgur0906 Sep 23, 2024

		public class HandlerExecution {

		private final Class<?> controller;


		private final Method method;

		public HandlerExecution(Class<?> controller, Method method) {

[MVC 구현하기 - 1단계] 카피(김상혁) 미션 제출합니다. #678

[MVC 구현하기 - 1단계] 카피(김상혁) 미션 제출합니다. #678

Uh oh!

Conversation

tkdgur0906 commented Sep 19, 2024

Uh oh!

kelly6bf 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!

kelly6bf Sep 21, 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!

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!

tkdgur0906 commented Sep 22, 2024

Uh oh!

kelly6bf left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

kelly6bf Sep 21, 2024 •

edited

Loading