[톰캣 구현하기 3, 4단계] 이오(이지우) 미션 제출합니다. #474
Conversation
There was a problem hiding this comment.
안녕하세요 이오!
미션 고생하셨습니다
훌륭하게 구현을 하신 것 같아요
이오의 의견이 궁금한 부분이 있어 request changes로 하겠습니다!
자잘한 의견을 드리기는 했는데 굳이 적용하실 필요는 없고...다음 PR 요청 주시면 바로 approve 하도록 하겠습니다
LoginController과 RegisterController 중복 관련된 부분은 doPost()로 보이는데, 이 메서드를 대상으로 한 고민이라고 이해하고 제 생각을 말씀드리겠습니다
이거는 저는 우발적인 중복이라고 생각합니다!
Login 정책과 Register 정책은 하나로 통합할 수 없는 정책이고, 이는 다른 변경 주기를 가질 것이라고 생각합니다(라이프 사이클이 다른 느낌으로?)
예를 들어 지금은 register를 해도 그냥 index로 보내지만 만약 register를 할 때 로그인 페이지로 보내버려야한다면 LoginController의 doPost()와는 다른 주기로 RegisterController의 doPost()가 변경될 것 같습니다
makeAuthorizedResponse() 메서드에 리소스 이름까지 전달한다면 코드 상으로는 메서드를 하나로 묶을 수는 있겠지만...이게 비즈니스 로직 흐름에서 과연 하나로 묶어도 괜찮을지는 잘 모르겠네요. 저는 어색하다고 생각합니다
그래서 결론은 코드가 중복되어서 신경쓰이는건 공감하지만 굳이 공통으로 묶을 필요는 없다고 생각합니다!
만약 굳이 공통 처리한다면 말씀하신대로 AuthController를 생성해서 상속 방식을 쓰면 될 것 같습니다.
AbstractController를 상속하는 추상 클래스인 AuthController를 만들고, AuthController를 RegisterController랑 LoginController가 다시 상속받으면 될 것 같은데 추가 상속을 고민하신 부분은 잘 이해를 하지 못했습니다.(AbstractController -> AuthController -> RegisterController / LoginController)
고려하신 방법대로 하면 묶을 수 있지 않을까요..?
| import org.apache.coyote.request.HttpRequest; | ||
| import org.apache.coyote.response.HttpResponse; | ||
|
|
||
| public class HomeController extends AbstractController { |
There was a problem hiding this comment.
이 부분에 대해 이오와 토론?을 해보고 싶은 부분이 있습니다!
저는 /와 같은 Context Path로 요청이 왔을 때 Hello world!를 반환해주는 것을 Tomcat의 역할로 인지를 했었습니다.
이와 비슷하게 css, js, iamge등의 정적 리소스를 제공해주는 기능 또한 Tomcat의 역할로 인지를 했었습니다.
그 근거로는 Tomcat의 web.xml에서 servlet, welcome page, static resource path등을 지정해주기 때문이었습니다.
그런데 제가 못찾은 것일 수도 있지만, 다른 분들 코드를 살짝 훔쳐보니 대부분 이오와 같은 식으로 구현을 하신 분들이 많더라고요..
그래서 제가 잘못된 접근을 했나 불안해서, 이오가 왜 이런 식으로 welcome page나 정적 리소스를 controller의 책임으로 구현하셨는지 궁금합니다!
There was a problem hiding this comment.
지토의 방향이 맞을 것 같습니다! 저는 Tomcat에서 해당 책임을 져야 한다는 사실을 미처 생각하지 못해서 현재와 같은 방식으로 구현했었습니다. 다시 생각해보니 Path에 따라 Controller를 매핑해준 것 처럼, 정적 리소스 또한 Tomcat에서 지정해 주는 것이 맞다는 생각이 드네요.
제가 놓치고 있던 사실을 짚어주셔서 감사합니다 👍 미처 생각하지 못했던 부분이라 토론은 못해드려 죄송하네요 😂
There was a problem hiding this comment.
해당 부분으로 반영을 시도했는데, Tomcat이 controller를 알고 있는 현재의 방식에도 문제가 있다는 생각이 들었습니다. 어떤 Controller가 있을 지를 controller가 알면 안된다고 생각해서요. 이 부분도 함께 수정하려다 보니 MVC 까지 가게 되었고, 이는 이번 미션에서 구현할 범위를 넘는 것 같아 오늘 시작될 미션에서 반영하고자 합니다.🥲
지토 덕에 좋은 고민을 많이 하게 되었네요. 감사합니다! 😊
There was a problem hiding this comment.
넵 굳이 반영은 해주실 필요 없고 그냥 의견이 궁금했던 거였습니다
조금 더 명확하게 메세지에 적어놓을걸 그랬네요...의견 공유해주셔서 감사합니다!
| protected void doPost(final HttpRequest request, final HttpResponse response) { | ||
| } | ||
|
|
||
| protected void doGet(final HttpRequest request, final HttpResponse response) { | ||
|
|
||
| } |
There was a problem hiding this comment.
해당 부분에서 AbstractController를 상속하는 하위 클래스들 중에 사용하지 않는 메서드의 경우 throw new PageNotFoundException(request.getPath());를 던지는 것으로 보입니다.
그렇다면 AbstractController에서 doGet()과 doPost()에서 throw new PageNotFoundException(request.getPath());를 기본적으로 던지도록 하고, 이를 상속받는 하위 클래스에서 사용할 수 있는 메서드에 대해서만 재정의를 하는 방식은 어떠실까요?
There was a problem hiding this comment.
좋은 방식이네요! 확실히 지금 방식은 비효율적이었던것같아요. 반영하였습니다 :)
|
|
||
| public class LoginController extends AbstractController { | ||
|
|
||
| private final UserService userService = new UserService(); |
There was a problem hiding this comment.
DI..를 고려해도 좋을 것 같은데 굳이 적용하실 필요는 없고 그렇습니다 ㅎㅎ;
There was a problem hiding this comment.
저도 살짝 생각은 했지만..? 다음 미션 마지막 강의에 DI가 있길래 스킵했습니다..ㅎㅎ...
한다면 userService 를 외부에서 주입받을 수 있도록 생성자로 받을 것 같네요!
| @Override | ||
| protected void doPost(final HttpRequest request, final HttpResponse response) { | ||
| final Map<String, String> bodyFields = parseFormData(request.getBody()); | ||
| final User user = userService.logIn(bodyFields); |
There was a problem hiding this comment.
UserService를 통해 비즈니스 로직과 HTTP 통신을 분리하신 부분이 매우 좋네요 👍
| if (path.equals("/")) { | ||
| return new HomeController(); | ||
| } | ||
| if (path.equals("/index.html")) { | ||
| return new IndexController(); | ||
| } | ||
| if (path.equals("/login")) { | ||
| return new LoginController(); | ||
| } | ||
| if (path.equals("/register")) { | ||
| return new RegisterController(); | ||
| } | ||
| return new ResourceController(); | ||
| } |
There was a problem hiding this comment.
해당 부분에 대해서도 이오의 의견이 궁금한 점이 있습니다!
제 경우 이러한 매핑 관련된 데이터는 해당 Controller에 관리하도록 했었는데, 이오가 이렇게 Controller 외부에서 이렇게 매핑 정보를 처리하신 이유가 궁금합니다!
There was a problem hiding this comment.
이유는 별거없지만 저희 미션 안내에서 RequestMapping이 클래스로 분리되어있어서였습니다!
Contoller를 미션 요구사항과 동일하게 구현해야한다는 말이 있었어서요. 그런데 다시 보니 RequestMapping은 힌트에 있던 내용이라 꼭 지킬 필요는 없었을지도..?
There was a problem hiding this comment.
저는 그 @RequestMapping이나 @WebServlet가 생각나서 이오의 의견이 궁금해서 질문드린건데 지금 미션에서는 필요 없는 내용이었던 것 같습니다...해당 코멘트는 무시해주셔도 될 것 같습니다 ㅎㅎ; 죄송합니다..
| } | ||
|
|
||
| public Cookies() { | ||
| this.cookies = new LinkedHashMap<>(); |
There was a problem hiding this comment.
혹시 Cookie에 LinkedHashMap을 사용하신 이유가 있으실까요?
보기로는 헤더 순서를 지키기 위한 것으로 보이는데 제가 알기로는 헤더 순서를 명확하게 지킬 필요는 없는 걸로 알아서...
근데 확실하지가 않아서 혹시 관련된 정보가 있으시다면 궁금하네요!
There was a problem hiding this comment.
순서를 지키기 위해서가 맞습니다!
사실 쿠키에서는 필요가 없는 내용인데, 헤더의 테스트코드를 통과하기 위해서는 순서를 보장할 필요가 있었기 때문에 헤더와 동일한 구조를 가져간 쿠키에서도 LinkedHashMap을 사용하게 되었습니다.
테스트코드를 순서와 상관없이 contain으로 통과하게 만들어도 되지만 뼈대코드의 테스트코드는 일종의 요구사항이라고 생각했기 때문에 수정하지 않았습니다...
| public static Cookies from(final String line) { | ||
| final Map<String, String> cookies = new HashMap<>(); | ||
| if (line == null || !line.contains(COOKIE_DELIMITER)) { | ||
| return new Cookies(cookies); |
| ); | ||
| try { | ||
| return handleRequestWithMethod(request); | ||
| controller.service(request, response); | ||
| return response; | ||
| } catch (final PageNotFoundException e) { | ||
| log.error(e.getMessage()); | ||
| return handleResponseWithException(NOT_FOUND); | ||
| return handleResponseWithException(response, NOT_FOUND); | ||
| } catch (final UnauthorizedException e) { | ||
| log.error(e.getMessage()); | ||
| return handleResponseWithException(UNAUTHORIZED); | ||
| return handleResponseWithException(response, UNAUTHORIZED); |
There was a problem hiding this comment.
리펙토링 깔끔하게 잘 해주신 것 같습니다 👍
ControllerAdvice 처럼 예외를 중앙 집중형으로 예외를 처리해주신 것 같은데, 제가 생각한 의도가 맞으실까요?
There was a problem hiding this comment.
해당 부분의 예외는 각각 401, 404, 500 에러라 비즈니스 로직과 상관없이 HTTP에서 처리해야 할 오류라고 생각하여 Http11Processor에 두었습니다. 😄
모든 Controller에서 발생할 수 있는 오류를 처리한다는 점에서 말씀하신 ControllerAdvice와 유사한 것 같네요!
| boolean isExist = true; | ||
| for (final String parameterName : parameterNames) { | ||
| isExist = isExist && queryParams.containsKey(parameterName); | ||
| } | ||
| return isExist; |
There was a problem hiding this comment.
음...이거 parameterNames로 넘겨준 모든 값이 QueryParameter에 존재하는지 확인하는 로직인 것 같은데 맞나요?
맞다면 depth를 하나 더 추가하는 대신에 for문 안에 if문으로 ealry return해줄 수 있을 것 같은데 이렇게 구현하신 이유가 궁금합니다!
| } | ||
| } | ||
|
|
||
| public String stringify() { |
There was a problem hiding this comment.
문자열로 변환하는 메소드가 필요했지만 toString을 오버라이드해서 쓰기엔 toString만의 느낌적인 역할(ex, log 출력용)이 있기 때문에 다른 이름을 가진 메소드가 필요하다고 느껴 문자열로 변환한다는 의미의 stringify를 사용하였습니다 😂
There was a problem hiding this comment.
아 이건 뭔가 문제가 있다는게 아니라 신기해서? 남겨 봤습니다!
센스있던 네이밍이었던 것 같아요 👍
|
Kudos, SonarCloud Quality Gate passed!
|
|
고생하셨습니다! approve 하겠습니다! |
안녕하세요 지토!
톰캣 구현 3,4단계 미션 제출합니다.
변경된 내용에 비해 커밋이 많은건 HttpResponse에 빌더패턴을 적용했다가 Controller에 맞추느라 다시 뜯어고쳤기 때문입니다..😂
크게 변경된 사항은 Headers, Cookies 등 일급 컬렉션을 생성한 것과 RequestLine, StatusLine 클래스를 분리한것 정도입니다!
변경된 부분
Header 인터페이스 추가
headers를 만들면서requestHeader와responseHeader를 함께 묶을 수 있는게 필요해져서 인터페이스 적용하였습니다.Cookies
cookie가 들어가서 중복이긴 하지만, 헤더에 담겨 있는게 의미상 틀리진 않아서 굳이Headers에서 제거하지 않았습니다.ContentType
ContentType을Enum으로 변경하고 내부에서 적절한ContentType을 찾도록 구조를 변경하였습니다! 😀고민한 부분
LoginController에서 쓰이는 메서드와RegisterController에서 쓰이는 메서드가 중복인데, 이 중복을 어떻게 제거할 수 있을지 고민중입니다.AuthController를 생성해서 상속하면 좋겠지만, 이미AbstractController를 상속받고 있어서 추가 상속이 불가능하더라구요.인터페이스를 구현하는 방식으로 하기에는 결국 내부 동작 코드는 다시 작성해야해서 의미가 없게 느껴졌습니다.
혹시 좋은 방법이 있을까요?