[MVC 구현 - 3단계] 아코(안석환) 미션 제출합니다. #567
Conversation
There was a problem hiding this comment.
아코 안녕하세요!
2단계 때 드렸던 질문에도 상세히 답변해주시고 3단계 구현도 빠르게 해주셨네요~~ 최고입니다 :D
깔끔하게 구현해주셔서 저도 빠르게 리뷰할 수 있었어요!
우선 아코가 고민하셨던 부분인 추상화 구조의 유지에 대한 제 생각은 다음과 같습니다.
저는 지금 구조를 그대로 유지하셨으면 좋겠어요.
요구사항이었던 점진적 리팩터링을 위해 추상화를 하셨지만, 결국 그 추상화 덕분에 app 모듈에서 서블릿 구현에 대한 제약이 사라진 것이 중요하다고 생각해요.
아코가 2단계때 말씀하셨던 것처럼, 저도 프레임워크를 개발하는 관점에서 생각해봤어요. 그랬을 때, 추상화가 아코의 프레임워크를 이용할 개발자들이 더 자유롭게, 그들이 지향하는 방향으로 개발할 수 있도록 돕는 역할을 하기 때문에 꼭 되돌리지 않아도 좋다는 결론을 내렸습니다.
또한 가능성이 적긴 하지만 controller와 마찬가지로 annotation mapping 방식도 레거시가 될 수도 있다는 생각도 들었구요.
그런 관점에서, DispatcherServlet에서 AnnotationMapping 을 만들어주는 것에서 HandlerMapping 구현체를 리플렉션을 통해 찾아보는 식으로 한 번 더 추상화를 하는 것도 좋겠다는 생각도 해봤답니다 ㅎㅎ
하지만 제 생각은 지극히 개인적은 의견이고 실제 장/단점을 비교하진 못한 것 같아 크게 신경쓰진 않으셔도 될 것 같아요. (아코 덕분에 이런 고민을 해볼 수 있어서 감사합니다 :D)
궁금한 점도 코멘트 남겼으니 함께 확인 부탁드려요!
| final PrintWriter writer = response.getWriter(); | ||
| final String body = makeBody(model); | ||
| writer.write(body); | ||
| writer.close(); |
There was a problem hiding this comment.
아코의 코드를 보고 PrintWriter를 닫아줘야하는가에 대한 의문이 생겼던 것 같아요.
찾아보니 비슷한 질문이 있더라구요.
https://stackoverflow.com/questions/1159168/should-one-call-close-on-httpservletresponse-getoutputstream-getwriter
저는 첫번째 답변(미리 닫는 것에 대한 부수효과)을 보고 음 그렇구나, 했고
두번째 답변을 보고는 확신을 얻을 수 있었어요.
The general rule of them is this: if you opened the stream, then you should close it. If you didn't, you shouldn't.
저도 PrintWriter는 HttpServletResponse가 소유하고 있고, 관리할 책임을 가지고 있다는 것에 동감합니다. 아코의 생각은 어떤지 궁금합니다!
There was a problem hiding this comment.
제가 처음에 close를 했던 이유는 PrintWirter가 Writer를 상속했고 Writer가 Closeable를 구현하고 있어서 큰 고민 없이 close를 했었습니다. 그런데 로지가 제시해준 stackflow를 보고 생각이 변경되었습니다. 여기서 PrintWriter를 close하게 된다면 스프링을 기준으로 이후에 interceptor의 afterCompletion()이나 filter에서는 추가적인 작업이 불가능해지는 것 같습니다. 또한 servlet container 기본적으로 관리를 해준다고 하니 굳이 명시적으로 close를 할필요가 없는 것 같습니다!
꼼꼼한 리뷰 감사합니다!🙇♂️🙇♂️
mvc/src/main/java/webmvc/org/springframework/web/servlet/view/JsonView.java
Show resolved
Hide resolved
| @@ -37,13 +35,12 @@ public void init() { | |||
|
|
|||
| private void initHandlerMapping() { | |||
| handlerMapping = new HandlerMappings( | |||
| List.of(new AnnotationHandlerMapping(Application.class.getPackageName() + ".*"), | |||
| new ManualHandlerMapping())); | |||
| List.of(new AnnotationHandlerMapping("com"))); | |||
There was a problem hiding this comment.
원래는 이 부분을 Application.java가 있는 패키지를 basepackage로 하려고 했었는데 DistpatcherServlet을 mvc 모듈로 옮기게 되다보니 위의 Application.class.getPackageName() 부분이 적용이 되지 않아서 불가피하게 com이라는 패키지로 수정하게 되었습니다. 혹시 더 좋은 방법이 있을까요? 저도 수정하면서 좋지 않은 방향이라고 느껴졌습니다.
There was a problem hiding this comment.
저는 app 모듈의 DispatcherServletInitializer에서 (현재는 리팩터링해서 이름이 다르긴합니다)
DispathcerServlet을 초기화해줄 때 패키지이름을 가져와서 문제를 겪지 않았어요.
허브 코드를 보니, 저와는 또 다른 방법을 써주셨더라구요
허허. 리플렉션 쓰려면 아무래도 패키지 이름을 가져오는 건 app 모듈에서 해야하나봐요. 저도 다른 방법은 모르겠네요🤔
There was a problem hiding this comment.
초기화를 해주는 부분을 DispatcherServletInitalizer에서 처리를 하면서 모듈이 변경되지 않아도 되어서 저와 같은 문제를 격지 않으셨나 보네요! 저도 두분의 방식처럼 수정을 하던 중에 곰곰히 생각을 해보았는데 dispatcherServlet의 init 메소드에서 HandlerMapping과 HandlerAdpater를 초기화하는 것이 불필요한 노출을 막을 수 있다고 생각이들어서 내부에서 초기화는 것이 조금 더 괜찮은 방식이라고 느껴졌습니다. 혹시 기존의 init()에서 초기화를 하는 것이 아닌 dispatcherServletInitailizer에서 HandlerMapping을 초기화를 해주신 점이 궁금합니다!
There was a problem hiding this comment.
현재는 수정하지 않고 제 방식을 유지하고 있는 상태입니다!
There was a problem hiding this comment.
저는 패키지를 명시해주는 것을 mvc 모듈에서 해주는 것을 아예 배재했기 때문에 해당 방법을 썼어요.
하지만 아코의 의견이 더 맞는 것 같다는 생각이 드네요.👍🏻
다만 그럼 궁금해지는 것은, 실제 스프링은 어떻게 하는지..? 인데 이건 제가 좀 더 학습을 해보겠습니다!!
mvc/src/main/java/webmvc/org/springframework/web/servlet/view/JsonView.java
Show resolved
Hide resolved
|
리뷰 주신 것에 대해서 반영을 하고 handlerMapping을 초기화하는 주체(dispatcherServlet와 dispatcherServletInitializer)에 대해서 의견을 남겨보았습니다! 꼼꼼한 리뷰 감사합니다!🙇♂️ |
| @@ -19,3 +19,5 @@ | |||
| - [x] Method 실행 시 불필요한 Controller 인스턴스 생성 제거 | |||
| - [x] HandlerAdapter 커스템 예외 추가(HandlerAdapter Not Found) | |||
| - [x] 예외처리 메세지 명확하게 수정(404페이지 리다이렉션) | |||
| - [x] 불필요한 자원 close 제거 | |||
There was a problem hiding this comment.
이제서야 말씀드리지만 꼼꼼히 요구사항 적어주시는거 보고
저도 다시 열심히 해봐야겠다는 생각을 했습니다. 감사합니다.😃
| assertThat(byteArrayOutputStream.toString()).isEqualTo(expect); | ||
| } | ||
|
|
||
| private static Stream<Arguments> givenModelAndExpectResult() { |
안녕하세요 로지!
아코입니다!
지난 피드백 반영
2단계 마지막 피드백을 반영한 사항입니다.
#478 (comment)
3단계를 진행하면서 고민했던 점
레거시 였던 MannualHandlerMapping과 Adapter가 사라지게 되면서 기존의 구현했던 추상화 구조를 유지하는 것이 좋을지 아니면 Annotation 기반만 남은 만큼 지워야 할지 고민이 되었습니다.(현재는 확장성을 고려해서 남겨두었습니다.) 이 부분에 대한 로지의 의견을 듣고 싶습니다!
이번 리뷰도 잘 부탁드립니다🙇♂️🙇♂️