Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MVC 구현하기 - 3단계] 채채 (신채원) 미션 제출합니다 #613

Merged
merged 12 commits into from
Oct 10, 2023

Conversation

chaewon121
Copy link

@chaewon121 chaewon121 commented Sep 25, 2023

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

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

@yeonkkk yeonkkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

고생하셨습니다!🫡

Comment on lines +44 to +46
private void render(ModelAndView modelAndView, HttpServletRequest request, HttpServletResponse response) throws Exception {
final View view = modelAndView.getView();
view.render(modelAndView.getModel(), request, response);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘 반영 해주셨네요!👍

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@chaewon121 chaewon121 Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +15 to +16
public ModelAndView index(final HttpServletRequest req,
final HttpServletResponse res) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import webmvc.org.springframework.web.servlet.view.JspView;

public class LoginViewController implements Controller {
@Controller
public class LoginViewController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return "redirect:/";
}
@Controller
public class LogoutController {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@chaewon121 chaewon121 Oct 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +17 to 20
public ModelAndView addObject(final String attributeName,
final Object attributeValue) {
model.put(attributeName, attributeValue);
return this;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

빌더 패턴 👍

@@ -14,7 +14,8 @@ public ForwardController(final String path) {
}

@Override
public String execute(final HttpServletRequest request, final HttpServletResponse response) {
Copy link
Member

@yeonkkk yeonkkk Sep 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  • app 모듈에 있는 모든 컨트롤러를 어노테이션 기반 MVC로 변경한다.
  • 그리고 asis 패키지에 있는 레거시 코드를 삭제해도 서비스가 정상 동작하도록 리팩터링하자.
  • Legacy MVC를 제거하고 나서 DispatcherServlet도 app 패키지가 아닌 mvc 패키지로 옮겨보자.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 좋은데요!?

}

private String writeValueAsString(Object object) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

writer.flush();
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@yeonkkk yeonkkk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@yeonkkk yeonkkk merged commit d0eb125 into woowacourse:chaewon121 Oct 10, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants