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단계] 가비 미션 제출합니다. #609

Merged
merged 9 commits into from
Sep 25, 2023

Conversation

iamjooon2
Copy link
Member

안녕하세요 히이로!

JspView는 2단계에서 미리 해버렸더니..

  1. JsonView 구현
  2. LegacyMVC 제거

이렇게 두가지 작업을 하게 됐습니다

여유로우실때 편하게 읽어주셨으면 좋겠습니다..!

@iamjooon2 iamjooon2 self-assigned this Sep 25, 2023
Copy link

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

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

안녕하세요 가비, 히이로입니다 🎵

전반적으로 요구사항 잘 수행해주셨으나 현재 application 실행 시 정상적으로 수행이 되지 않는 부분과 JsonView에서 좀 더 생각해보셨으면 하는 부분이 있어서 빠르게 리뷰드렸습니다. 아마 바로 수정해주실 수 있으실 것 같은데 반영해주신 뒤 리뷰 요청 주시면 바로 merge하게 되지 않을까 싶네요...!

마지막 미션 좀만 더 힘내서 달려보시죠!

import web.org.springframework.web.bind.annotation.RequestMethod;
import webmvc.org.springframework.web.servlet.ModelAndView;
import webmvc.org.springframework.web.servlet.view.JspView;

Choose a reason for hiding this comment

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

여기 @controller 어노테이션이 누락된 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

정말 죄송합니다
마음이 급했습니다


private static final Logger log = LoggerFactory.getLogger(LoginController.class);

@RequestMapping(value = "/login", method = RequestMethod.GET)

Choose a reason for hiding this comment

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

loginController와 loginViewController가 합쳐지면서 /login/view URI에 대한 요청을 처리해 줄 컨트롤러가 사라진 것 같습니다. application 구동시켜서 동작시켜보시면 현재 안되는 부분들이 있는데 확인해서 수정 부탁드릴게요!

Copy link
Member Author

@iamjooon2 iamjooon2 Sep 25, 2023

Choose a reason for hiding this comment

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

의도하고 날린 부분이었는데요
*/view 와 같은 방식으로 경로에 뷰임을 지정해줘야 하는 것도 레거시라고 생각했습니다

다만 API 명세 자체를 바꿔버린 꼴이 됐네요

요구사항 잘못 이해하고 바꾼 것 같습니다 다시 바꿔둘게요


private Object createResult(Map<String, ?> model) {
if (model.size() == SINGLE_JSON_SIZE) {
return model.values();

Choose a reason for hiding this comment

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

image

model에 담긴 value가 하나일 때 values()를 통해 배열을 반환하고 있네요! 현재 프로젝트 요구사항에서는 model에 값이 하나일 때 값을 그대로 반환한다고 되어 있는데 그대로 반환한 것일까요? 그리고 User 객체를 json 응답으로 내려주는데 account 정보 하나만 반환되는 것이 괜찮을까요? 저도 여기 부분에서 어떻게 처리해야할지 고민을 조금 했었는데 model이 하나의 값을 가질 때 어떻게 응답을 해줄지 좀 더 고민해보셔도 좋을 것 같습니다. 😃

Copy link
Member Author

@iamjooon2 iamjooon2 Sep 25, 2023

Choose a reason for hiding this comment

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

아고 이해하는데 좀 걸렸네요
toString으로 user 모델의 모든 값들을 그대로 출력하도록 변경했습니다...!

image

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability B 1 Vulnerability
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

35.4% 35.4% Coverage
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

idea Catch issues before they fail your Quality Gate with our IDE extension sonarlint SonarLint

Copy link

@MoonJeWoong MoonJeWoong left a comment

Choose a reason for hiding this comment

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

안녕하세요 가비, 정신이 많이 없으실텐데 마지막까지 꼼꼼하게 요구사항 잘 챙겨주셔서 감사합니다 ㅎㅎ 오늘 푹 쉬시고 내일부터 새 미션에 집중하시죠! 고생 많으셨습니다~ 👍

@MoonJeWoong MoonJeWoong merged commit 73baf44 into woowacourse:iamjooon2 Sep 25, 2023
1 of 2 checks passed
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