[MVC 구현하기 - 3단계] 우르(김현우) 미션 제출합니다. #535
Conversation
- Controller, ManualHandlerXXX 삭제
There was a problem hiding this comment.
안녕하세요 우르!
이번에도 빠르고 깔끔하게 구현해주셨네요👍🏻
커밋을 신경써주신 만큼 리뷰 하기 편했습니다. 감사해요!
이번 요구사항은 코드에 큰 변화가 있는건 아니라서 제가 아는 선에서는 리뷰할 내용이 잘 떠오르지 않았습니다ㅠ 그래서 다른 리뷰들을 조금 참고해서 함께 확인하면 좋은 부분들만 공유해두었습니다!
IO관련 코멘트와 인덴트 관련 코멘트를 제외하고 나머제 코멘트는 이번에도 우르의 의견이 궁금해 코멘트를 남겨두었고, 리팩토링을 요구하는 코멘트는 아닙니다!
편하게 확인하시고 리뷰요청 주세요! 👍🏻
| @@ -55,7 +53,6 @@ protected void service(final HttpServletRequest request, final HttpServletRespon | |||
| final Object handler = handlerMappingComposite.getHandler(request); | |||
| final Object handleValue = handlerAdapterComposite.handle(request, response, handler); | |||
|
|
|||
| // TODO : 3단계 리팩터링 | |||
| renderByHandlerValueType(request, response, handleValue); | |||
There was a problem hiding this comment.
해당 클래스 전반적으로 인덴트 정렬이 잘못 적용된 것 같아요!
인덴트가 2로 되어있는 것 같습니다
There was a problem hiding this comment.
제가 google style 을 사용하고 있어서 인덴트가 2로 되어있습니다 !!
| final String body = objectMapper.writeValueAsString(model); | ||
|
|
||
| if (hasSingle(model)) { | ||
| response.setContentType("text/html"); |
There was a problem hiding this comment.
글렌의 리뷰에서 발견한 내용인데 우르의 상황과 동일한 것 같아 링크 첨부합니다!
JsonView에서 왜 text/html 타입의 응답을 반환하고 있는지와
Map으로 반환되는 json 포맷과 객체로 반환되는 json 포맷이 어떻게 다른지 생각해보시면 좋을 것 같아 코멘트 남깁니다.
(저도 몰라서 리뷰 하면서 찾아봤습니다...)
There was a problem hiding this comment.
Map으로 반환되는 json 포맷과 객체로 반환되는 json 포맷이 어떤건지 제대로 이해를 못했습니다 ㅠㅠ
Map의 key와 value과 그대로 json 의 key, value로 가게 되고,
자바 POJO 객체가 json 포맷으로 변경될 경우에는 필드명이 key 값으로 가고, 그 객체가 가지고 있는 값이 value로 갈텐데 어떤 관점에서 다르다는 것을 생각해보면 좋을까요??
getter의 여부에 따른 역직렬화 부분일까요?
There was a problem hiding this comment.
JsonView라 해서 무조건 content type을 JsonView 로 반환해야하는걸까요??
값이 1개 밖에 존재하지 않을 때, 즉, model 의 key값이 한개밖에 존재하지 않을 때는 값만을 반환해야하는데, 이때는 값이라는 의미가 JSON 뷰가 아닌 String 값이라 생각이 들었어요.
그래서 text/plain 이 적합하지 않나라는 생각이 듭니다!!
model에 데이터가 1개면 값을 그대로 반환하고 2개 이상이면 Map 형태 그대로 JSON으로 변환해서 반환한다.
요구사항에서는 2개 이상이면 Map 형태 그대로 JSON으로 변환하라는 말이 직접적으로 있었고,
'1개일 경우에는 값을 그대로 반환하라'라는 말이 있어서 두 요구사항은 엄연히 다른 것이라 생각이 들었어요..
그래서 차이를 줘야한다는 생각이었습니다 !!
|
|
||
| if (hasSingle(model)) { | ||
| response.setContentType("text/html"); | ||
| response.getWriter().write(body); |
There was a problem hiding this comment.
이것도 글렌의 리뷰에서 주워온 내용입니다..ㅎ
getWriter()로 PrintWriter를 가져와 응답값을 설정하는 것은 IO 작업이니만큼 IO 작업에 필요한 예외 처리가 있으면 좋을 것 같습니다.
| @@ -32,14 +32,12 @@ public DispatcherServlet() { | |||
| public void init() { | |||
| handlerMappingComposite = new HandlerMappings( | |||
| List.of( | |||
| new ManualHandlerMapping(), | |||
| new AnnotationHandlerMapping("com.techcourse") | |||
There was a problem hiding this comment.
이것도 글렌의 리뷰에서 주워온 내용입니다..ㅎㅎ..
프레임워크 패키지에 속한 클래스가 app 패키지의 경로를 항상 알고 있어야 한다는 점에서 의존성 분리가 더 필요해보인다는 내용인 것 같은데, 다른 크루들의 PR도 확인해봤지만 제가 패키지 구조를 관리하는 것이 아직 익숙하지 않아서 그런지 해결 방법은 아직 못 찾은 상태입니다..ㅎㅎ..
지토의 PR에서 뭔가 힌트를 얻으실 수 있을 것 같아 우선 첨부해봅니다..ㅎㅎ....
이 부분은 리팩토링 요청은 아니고, 다른 크루분들이 패키지 구조에 대해서도 많이 고민하는 것 같아 함께 참고하시면 좋을 것 같아 코멘트 달아둡니다!
혹시 관련해서 공유하고싶으신 의견이 있다면 공유 해주시면 감사하겠습니다
There was a problem hiding this comment.
오,, 역시 다른 크루분들은 엄청난 생각을 하시네요,,
전 이런 생각도 못하고 있었습니다,,, ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ
app 패키지의 경로를 항상 알고 있어야 한다는 점
그 정도는 알아도 되지 않을까 라는 생각이드네요,,
이게 맞는진 모르겠지만 우리도 스프링을 통해 애플리케이션을 만들 때, XXXApplication 이 있는 패키지나 그 하위 패키지에 controller들을 만드는 이유가 @ComponentScan에서 명시를 해준다고 생각을 했습니다.
그래서 결국 그 의존성은 절대 없애지 못하지 않을까라는 생각이 듭니다!!
| @@ -32,62 +31,37 @@ public DispatcherServlet() { | |||
| public void init() { | |||
| handlerMappingComposite = new HandlerMappings( | |||
| List.of( | |||
| new ManualHandlerMapping(), | |||
| new AnnotationHandlerMapping("com.techcourse") | |||
| ) | |||
There was a problem hiding this comment.
추가로 요구사항의 힌트에서 UserController를 생성해 정상적으로 동작하는지 테스트 해볼 수 있다고 해서,
이 부분도 진행해보시면 좋을 것 같습니다!
안녕하세요 메리 !!
이번 단계에서는 최대한 커밋을 보기 쉽게 해보도록 노력했습니다.
미션 요구사항이 JspView, JsonView, 레거시 MVC 제거하기 등이었는데
순차대로 커밋을 해놓았습니다.
메리도 보기 편하시길 바라면서 이번 리뷰도 감사히 잘 받겠습니다 !