-
Notifications
You must be signed in to change notification settings - Fork 305
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 구현하기 - 2,3단계] 토리(천은정) 미션 제출합니다. #632
Conversation
- initialize 내부 메소드 가독성을 위해 for문 대신 stream 사용 - RequestMapping 어노테이션의 method을 배열로 가져오도록 변경
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 토리 👧🏻
시간이 많이 부족했음에도 불구하고 잘 구현해주신거 같아요!!
2단계, 3단계 커밋도 나눠주셔서 보기 편했어요 ㅎㅎ
요구사항 위주로 리뷰 조금 남겼는데 고민해보시고 반영해주시면 좋을거 같아요 ㅎㅎ
다음 미션이 기다리고 있으니 .. 다시 요청 주시면 바로 머지하도록 하겠습니당 🚀
궁금하신 점 있으시면 언제든 DM 주세요 ~~
수고하셨습니당 마지막까지 파이팅 🙃 !!
@Controller | ||
public class LogoutController { | ||
@RequestMapping(value = "/logout", method = RequestMethod.GET) | ||
public ModelAndView show(final HttpServletRequest req, final HttpServletResponse res) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그아웃 역할을 담은 메서드명으로 수정해보는건 어떨까요?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET
으로 들어오는 요청에 대해서는 각 Controller에 대한 화면을 출력해준다는 의미로 통일성을 주고 싶었는데,
생각해보니 로그아웃은 화면을 띄워주는게 아니라 로그아웃 역할을 하는 부분이니 수정하는게 더 역할에 맞는 네이밍이겠네요 !!
logout
으로 수정했습니다 ~ ˙ᵕ˙ 🙇♀️
.collect(Collectors.toMap(Function.identity(), this::instanticateClazz)); | ||
} | ||
|
||
private Object instanticateClazz(final Class<?> clazz) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오타가 👀👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오마이갓... 바로 수정했습니다 !
private String bodyToJson(final Object body) throws JsonProcessingException { | ||
final ObjectMapper objectMapper = new ObjectMapper(); | ||
return objectMapper | ||
.writerWithDefaultPrettyPrinter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 .. writerWithDefaultPrettyPrinter()
!!! 이런 메서드도 존재하는군요! 좋은데요 ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
json을 예쁘게 출력하기 ! 저도 찾아보면서 배웠습니다 👍
@Override | ||
public void render(final Map<String, ?> model, final HttpServletRequest request, HttpServletResponse response) throws Exception { | ||
final String body = bodyToJson(model); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항의 힌트에 아래와 같은 설명이 써있어요!
model에 데이터가 1개면 값을 그대로 반환하고 2개 이상이면 Map 형태 그대로 JSON으로 변환해서 반환한다.
저는 이 힌트를 이렇게 해석했는데요, 단순히 string으로 반환하는 크루들도 보이더라구요!
토리가 생각하신 방향대로 모델에 데이터가 1개일 경우를 고려해서 수정해보시면 좋을거 같아요 ~~!
// 모델에 데이터가 1개일 경우
{
"account" : "gugu",
"password" : "1234"
}
// 모델에 데이터가 여러개일 경우
{
"user1" : {
"account" : "gugu",
"password" : "1234"
},
"user2" : {
"account" : "raon",
"password" : "1234"
},
"user3" : {
"account" : "tori",
"password" : "1234"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음에 어떤식으로 변경해야할지 헷갈려 기존의 방식대로 구현하였습니다 🥲
라온의 리뷰와 오늘 강의를 듣고 아래와 같이 출력되도록 리팩토링하였습니다 ˙ᵕ˙
// 모델에 데이터가 1개일 경우
{
"id" : 1,
"account" : "gugu",
"password" : "password",
"email" : "[email protected]"
}
// 모델에 데이터가 여러개일 경우
{
"user1" : {
"id" : 1,
"account" : "gugu",
"password" : "password",
"email" : "[email protected]"
},
"user2" : {
"id" : 1,
"account" : "raon",
"password" : "raonlove",
"email" : "[email protected]"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요구사항대로 DispatcherServlet을 mvc 패키지로 옮겨볼까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ManualHandlerMapping의 패키지도 생각해볼까요 ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
asis
패키지로 이동했는데 어떨까요 .. ! 패키지가 제일 어려워요... 🥲
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞아요 .. 패키지 너무 어려워요 ㅠㅠ
저도 asis 패키지에 위치하는게 괜찮은거 같아요 ㅎㅎ
private static final Logger log = LoggerFactory.getLogger(ManualHandlerMapping.class); | ||
|
||
private static final Map<String, Controller> controllers = new HashMap<>(); | ||
|
||
public void initialize() { | ||
controllers.put("/", new ForwardController("/index.jsp")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ForwardController는 legacy 코드가 아닌지, mvc 패키지에 그대로 위치하는 것이 좋을지 고민해보면 좋을거 같아요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HomeController
클래스를 생성하여 레거시 코드를 제거해주었습니다 ˙ᵕ˙ !!
private static final String DEFAULT_SERVLET_NAME = "dispatcher"; | ||
|
||
@Override | ||
public void onStartup(final ServletContext servletContext) { | ||
final var dispatcherServlet = new DispatcherServlet(); | ||
dispatcherServlet.addHandlerMapping(new ManualHandlerMapping()); | ||
dispatcherServlet.addHandlerMapping(new AnnotationHandlerMapping()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AnnotationHandlerMapping은 생성자로 reflection의 대상이 되는 basePackage를 주입받는데 ..
여기서 주입을 해주지 않아도 app 모듈의 컨트롤러를 정상적으로 불러오네용 ..
이러한 경우에는 어떻게 주입이 되는지 궁금해서 디버깅을 해봤는데, 비어있는 경우 아래 사진에 있는 if문에 걸려/jwp-dashboard-mvc/app/src/main/webapp/WEB-INF/classes/
경로에 있는 클래스를 모두 로딩해오지만, basePackage를 지정한 경우에는 inputsFilter로 한번 걸러지는거 같아요 ㅎㅎ
신기해서 공유드립니다 ㅎㅎ 관련해서 더 알고계시다면 공유해주세요 ~~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 사실 저도 이 부분에 대해선 자세히 몰랐는데 라온 덕분에 하나 더 배워갑니다... 🙇♀️
- model에 데이터가 1개면 값을 그대로 반환, 2개 이상이면 Map 형태 그대로 JSON으로 변환해서 반환하도록 수정
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 토리 👧🏻
리뷰 반영 빠르게 해주셨네요 👍👍
간단한 커멘트 조금 남겼는데, 확인만 해주셔도 좋을거 같아요 ㅎㅎ
요구사항도 모두 만족한거 같아서 이번 미션은 머지하도록 하겠습니다!
mvc 미션 매우매우 수고 많으셨습니당 😊
jdbc도 파이팅 !!
protected void service(final HttpServletRequest request, final HttpServletResponse response) throws ServletException { | ||
log.debug("Method : {}, Request URI : {}", request.getMethod(), request.getRequestURI()); | ||
try { | ||
final Object handler = handlerMappingRegistry.getHandler(request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
스프링의 경우 리소스가 존재하지 않는 경우 아래처럼 반환해주는 것처럼
Handler를 찾지 못한 경우, getHandler()
에서 예외를 던지는 대신 404.jsp를 반환해주는 방법도 있을거 같아요 ㅎㅎ
{
"timestamp": "2023-09-26T08:59:35.244+00:00",
"status": 404,
"error": "Not Found",
"path": "/api"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 좋은 방법이네요 !! 감사합니다 🙇♀️👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
public class HomeController { | ||
@RequestMapping(value = "/", method = RequestMethod.GET) | ||
public ModelAndView show(HttpServletRequest req, HttpServletResponse res) { | ||
return new ModelAndView(new JspView("redirect:/index.jsp")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"redirect:/index.jsp"를 사용한 이유가 있을까용 ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
레거시 코드에서 "/"로 가는 경우 index.jsp 로 되어있어서 이렇게 구현하였습니다 .. !
안녕하세요 라온 !!
2,3단계 미션 한 번에 보내게 되었는데 양해해주셔서 감사합니다 🙇♀️
미션이 저한텐 조금 어렵게 다가와서 요구사항을 중점으로 힌트를 많이 참고해서 구현했어요..
2단계 미션 커밋
3단계 미션 커밋
오늘도 좋은 하루 되세요 ~~ 감사합니다 ❤️🌈