-
Notifications
You must be signed in to change notification settings - Fork 382
[MVC 구현하기 - 3단계] 카피(김상혁) 미션 제출합니다. #839
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
Conversation
kelly6bf
left a comment
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.
카피! 리뷰가 늦어서 죄송합니다. 🙇
리뷰에는 언급드리지 않았지만 시간이 되신다면 전체적으로 Controller들이 ModelAndView를 반환하는게 아니라 ViewName(ex: index, login)만 반환하도록 개선해보면 좋을 챌린지가 될 거 같습니다! (우선 시간이 부족하실거 같아 따로 코멘트로 남기지는 않았습니다!)
마지막까지 파이팅입니다!
| import com.interface21.webmvc.servlet.mvc.tobe.adapter.AnnotationHandlerAdapter; | ||
| import com.interface21.webmvc.servlet.mvc.tobe.adapter.HandlerAdapters; | ||
|
|
||
| public class HandlerAdaptersInitializer { |
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.
Initializer의 존재는 쓸만하신가요? 저도 생성을 전문으로 담당해주는 객체 분리를 고민중인데 카피 의견이 궁금하네요!
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.
초기화하는 객체를 만드니까 더 명확해지는 장점이 있는 것 같아요!
결국 어딘가에서는 초기화를 해줘야 하는데, 다른 곳에 있으면 찾기 힘들수도 있을 것 같긴 합니다.
근데 이정도는 취향차이인 것 같긴합니다!
| return "redirect:/index.jsp"; | ||
| @RequestMapping(value = "/login", method = RequestMethod.GET) | ||
| public ModelAndView loginView(HttpServletRequest request, HttpServletResponse response) { | ||
| return new ModelAndView(new JspView(UserSession.getUserFrom(request.getSession()) |
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.
아무래도 생성자에 복잡한 로직이 추가되니 가독성이 조금 떨어지기도 하고, 클래스 전체적으로 ModelAndView를 생성하는 로직이 두 번 이상 사용되니 메서드 분리를 고민해보면 좋을거 같네요!
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 Set<Object> createBeans(final Set<Class<?>> classes) { | ||
| final Set<Object> beans = new HashSet<>(); | ||
| for (Class<?> aClass : classes) { | ||
| try { |
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로 만들 수 없는지 고민해볼까요!
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 void setField(Object bean, Field field) { | ||
| for (Object injectBean : beans) { | ||
| if (field.getType().isAssignableFrom(injectBean.getClass())) { |
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로 유지하는 방법을 고민해보시면 좋을거 같아요!
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.
메서드 분리를 통해 줄였습니다!
|
켈리~ 리뷰 해주신 것들 반영했습니다! |
kelly6bf
left a comment
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로 유지할 수 있으니 조금 더 고민해보시는걸 추천드립니다! 고생 많으셨어요!
안녕하세요 켈리~
드디어 마지막 단계네요!
학습테스트도 pr에 같이 포함시켰으니 확인 부탁드립니다!
리뷰 잘 부탁드립니다~