[MVC 구현하기 - 3단계] 채채 (신채원) 미션 제출합니다 by chaewon121 · Pull Request #613 · woowacourse/java-mvc

chaewon121 · 2023-09-25T07:05:57Z

조이 안녕하세요~!이번 미션도 잘부탁 드립니다..!
크게 변경된 사항은 기존의 코드를 삭제하고 어노테이션 컨트롤러로 돌아가도록 리팩토링 하였습니다.
또한 jsonview도 작성하였습니다.
이버 미션도 잘부탁드립니다!

yeonkkk

안녕하세요 채채! 조이입니다 ~
3단계도 잘 구현해주셨네요!

요구사항 관련 코멘트 조금 남겨두었으니 확인해주시면 감사하겠습니다🙂
그리고 1단계 리뷰에 대한 코멘트 확인이 안 된 것 같아서 이번에 수정하시면서 같이 확인해주시면 좋을 것 같아요 ㅎㅎ

고생하셨습니다!🫡

yeonkkk · 2023-09-25T07:09:49Z

+    private void render(ModelAndView modelAndView, HttpServletRequest request, HttpServletResponse response) throws Exception {
+        final View view = modelAndView.getView();
+        view.render(modelAndView.getModel(), request, response);


잘 반영 해주셨네요!👍

yeonkkk · 2023-09-25T07:24:37Z

@@ -4,7 +4,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import web.org.springframework.web.WebApplicationInitializer;


채채는 DispatcherServlet과 DispatcherServletInitializer를 app 패키지에 그대로 두셨군요!
mvc 패키지로 이동하는 것도 고려를 해볼 수 있을 것 같은데, 채채가 DispatcherServlet과 DispatcherServletInitializer를 app 패키지에 두신 이유가 궁금합니다!

DispatcherServletInitializer가 app패키지에 의존하고있어서 따로 분리하지 못하였습니다..! 이부분에 대해서 조이는 어떻게 처리하셨나요!? 저는 이부분에대해서 전혀 감을 잡지 못하였습니다아...

저도 DispatcherServletInitializer는 app 패키지에 그냥 두고 DispatcherServlet만 이동했어요!
그냥 진짜 생각해보셨는지 궁금하기도 하고, 안 해보셨으면 해보면 좋을 것 같아서 말씀드렸어요🙂

yeonkkk · 2023-09-25T07:25:22Z

+    public ModelAndView index(final HttpServletRequest req,
+                              final HttpServletResponse res) {


요기도 request, response로 통일해주면 좋을 것 같네용

yeonkkk · 2023-09-25T07:28:25Z


-public class LoginViewController implements Controller {
+@Controller
+public class LoginViewController {


Login 관련 메서드들을 하나의 컨트롤러에서 관리할수도 있을 것 같은데 LoginController와 LoginViewController로 구분하신 기준이 궁금합니다!

yeonkkk · 2023-09-25T07:29:43Z

-        return "redirect:/";
-    }
+@Controller
+public class LogoutController {


로그아웃 기능이 정상 작동 하지 않는 것 같아요~ 확인 한번 해보시면 좋을 것 같아요!
(아마 request method 때문인 것 같네요)

post로 되어있군요..ㅠㅠ 수정하겠습니다~!

yeonkkk · 2023-09-25T07:33:43Z

+    public ModelAndView addObject(final String attributeName,
+                                  final Object attributeValue) {
        model.put(attributeName, attributeValue);
        return this;


빌더 패턴 👍

yeonkkk · 2023-09-25T07:36:57Z

    }

    @Override
-    public String execute(final HttpServletRequest request, final HttpServletResponse response) {


이번 미션의 요구사항 3번인 Legacy MVC 제거하기 내용을 한번 확인해보면 좋을 것 같아요!
todo를 체크해보면 아래와 같겠네요!

app 모듈에 있는 모든 컨트롤러를 어노테이션 기반 MVC로 변경한다.

그리고 asis 패키지에 있는 레거시 코드를 삭제해도 서비스가 정상 동작하도록 리팩터링하자.

Legacy MVC를 제거하고 나서 DispatcherServlet도 app 패키지가 아닌 mvc 패키지로 옮겨보자.

FrontController, Controller 등 불필요한 레거시 코드는 제거하면 좋을 것 같아요~
만약 남겨두신 이유가 있다면 알려주시면 감사하겠습니다!

조이! 한번더 todo list를 체크해 주셔서 감사합니다! 덕분에 레거시 코드를 삭제해도 잘 되는지 확인해볼 수 있었습니다! 남겨주신 체크리스트에 체크하였습니다~!

yeonkkk · 2023-09-25T07:40:59Z

+    public void render(final Map<String, ?> model,
+                       final HttpServletRequest request,
+                       final HttpServletResponse response) throws Exception {
+        final String json = getJsonString(model);


변수명을 json 말고 좀 더 구체적이고 명시적으로 작성하면 어떨까요?
예를 들면 responseBody 를 생각해볼 수 있을 것 같아요!

너무 좋은데요!?

yeonkkk · 2023-09-25T07:41:39Z

+        return writeValueAsString(model);
    }
+
+    private String writeValueAsString(Object object) {


Suggested change

private String writeValueAsString(Object object) {

private String writeValueAsString(final Object object) {

사소하지만 컨벤션으로 가져가시는 것 같아서 언급해봅니다~

yeonkkk · 2023-09-25T07:41:54Z

+        writer.flush();
+    }
+
+    private String getJsonString(Map<String, ?> model) {


Suggested change

private String getJsonString(Map<String, ?> model) {

private String getJsonString(final Map<String, ?> model) {

사소하지만 컨벤션으로 가져가시는 것 같아서 언급해봅니다~

기존 코드에 컨벤션을 추가로 지키기가 쉽지않군요 껄껄.. 자세한 리뷰 감사합니다 조이!

yeonkkk

채채 ~
리뷰 반영 잘 해주신 것 확인해서 approve & merge 하겠습니다!
고생하셨어요.
남은 레벨4 미션도 화이팅 입니다🙂

yeonkkk · 2023-10-10T19:42:37Z

@@ -4,7 +4,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import web.org.springframework.web.WebApplicationInitializer;


저도 DispatcherServletInitializer는 app 패키지에 그냥 두고 DispatcherServlet만 이동했어요!
그냥 진짜 생각해보셨는지 궁금하기도 하고, 안 해보셨으면 해보면 좋을 것 같아서 말씀드렸어요🙂

chaewon121 added 8 commits September 25, 2023 15:23

refactor: 어노테이션 매핑만 남도록 컨트롤러 수정

bc9dc2a

feat: ManualHandlerMapping 클래스 제거

3923751

feat: user 컨트롤러 추가

4c1edbe

refactor: 어노테이션 컨트롤러 빼로 제거

1e23c9f

feat: jsonView 구현

a421497

refactor: 기존 초기화 제거

7b2cbbc

style: final 추가 및 개행 수정

82b6dd7

style: 통일되지 않은 네이밍 수정

008358b

chaewon121 changed the title ~~Step3~~ [MVC 구현하기 - 3단계] 채채 (신채원) 미션 제출합니다 Sep 25, 2023

yeonkkk requested changes Sep 25, 2023

View reviewed changes

chaewon121 added 4 commits October 11, 2023 01:21

refactor: 레거시 코드 제거

ef44a00

refactor: 패키지 이동

ee3455f

refactor: 변수명 수정

97fd8e5

refactor: 컨벤션에 따라 수정

7f80dae

yeonkkk approved these changes Oct 10, 2023

View reviewed changes

yeonkkk merged commit d0eb125 into woowacourse:chaewon121 Oct 10, 2023

		public ModelAndView index(final HttpServletRequest req,
		final HttpServletResponse res) {

	private String writeValueAsString(Object object) {
	private String writeValueAsString(final Object object) {

	private String getJsonString(Map<String, ?> model) {
	private String getJsonString(final Map<String, ?> model) {

Conversation

chaewon121 commented Sep 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

chaewon121 Oct 10, 2023 • 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!

chaewon121 Oct 10, 2023 • 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!

yeonkkk Sep 25, 2023 • edited by chaewon121 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!

yeonkkk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chaewon121 commented Sep 25, 2023 •

edited

Loading

chaewon121 Oct 10, 2023 •

edited

Loading

chaewon121 Oct 10, 2023 •

edited

Loading

yeonkkk Sep 25, 2023 •

edited by chaewon121

Loading