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 구현하기 - 2,3단계] 케빈(박진홍) 미션 제출합니다. #102

Merged
merged 7 commits into from
Sep 22, 2021

Conversation

xlffm3
Copy link

@xlffm3 xlffm3 commented Sep 18, 2021

안녕하세요 마갸 잘부탁드립니다.
크게 변경사항은 없구요

  • 기존 legacy controller 분리 및 annotation 기반 컨트롤러 생성
  • dispatcher servlet에서 기존 legacy controller mapping, adapater 추가하는 부분 주석처리
  • json view 구현

json view에 대해서 테스트를 진행하려고 했는데 request, response 때문에 어플리케이션 실행후 수동 확인밖에 못했습니다 ㅠㅠ

Copy link

@MyaGya MyaGya left a comment

Choose a reason for hiding this comment

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

고생하셨어요 케빈! 코드 상으로는 크게 이상한 부분이 보이지 않았어요. 다만, 새롭게 만든 컨트롤러의 이름이 New___ 라는건 조금 오해의 소지가 있을 수도 있을 것 같아요. 그런데 저도 마땅한 대안이 떠오르지는 않네요. 그 외에 코드상 바뀐 부분에 대한 설명이랑 jackson 버전이 올라간 부분에 대한 설명이 없는 것 같아 코멘트 달았습니다.

@@ -0,0 +1,17 @@
package com.techcourse.controller.annotation_base;
Copy link

Choose a reason for hiding this comment

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

패키지 명에 언더바가 들어가 있어요. 물론 컨벤션이나 기타 제약, 암묵적인 합의 등을 고려하셔서 짠 부분이겠지만 구글 컨벤션 스타일에서는 해당 스타일을 지양하도록 되어 있어서 관련 문서 하나 남기겠습니다.
https://stackoverflow.com/questions/3179216/what-is-the-convention-for-word-separator-in-java-package-names

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@xlffm3
Copy link
Author

xlffm3 commented Sep 22, 2021

안녕하세요 마갸, 피드백 반영했습니다.
과제 안내사항에 버전 조전에 관련된 내용이 있어서 조정했었어요 :)

@MyaGya
Copy link

MyaGya commented Sep 22, 2021

확인했습니다! 고생하셨어요. 버전 관련해서는 코어를 옮긴 부분이니 이유에 대해서도 어느정도 명시되어 있으면 좋을것 같아서 드린 말씀이었는데 저도 왜 버전을 하나 올려야하는지는 잘 모르겠더라구요... 공식 문서에서도 핫픽스 정도만 바뀐 정도였거든요. 어찌됬든 고생 많으셨습니다! 여러 악재가 겹쳐서 답변 늦어진 부분이 죄송하네요. 다음 미션도 많이 배워갔으면 좋겠습니다.
피드백을 더 잘 해드리고 싶었는데 제가 보기에는 너무 이상이 없어서 피드백을 조금 드린 것 같아 죄송합니다

@MyaGya MyaGya merged commit d5618ee into woowacourse:xlffm3 Sep 22, 2021
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