[톰캣 구현하기 1단계] 조이(김성연) 미션 제출합니다. #374
Conversation
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
안녕하세요 조이! 콩하나입니다 ㅎㅎ
전반적으로 너무 깔끔한 코드로 구성되어있어서 코드를 이해하는데,
그리고 웹서버를 이해하는 데에도 도움이 정말 많이 되었습니다!
마치 공식 문서 읽는 것 같았어요!!
이 내용이 BE-5기-코드리뷰에도 올라가겠죠!?
우테코 5기 여러분 조이 코드 보러오세요~ ㅋㅋㅋㅋㅋㅋ
구조도 너무 깔끔하고, 각 클래스별로 역할들도 잘 분배되어있다고 느껴졌습니다, 👍
코드 읽으면서 정말 많이 배웠어요 조이~
1, 2단계 내용은 승인하도록 하겠습니다!
몇 가지 간단한 질문이나 코멘트들을 남겼어요.
대부분 조이의 의견이 궁금해서 남긴 내용들이니까 시간 나실 때 차근차근 남겨주시기 바라요.
3단계가 리팩토링이니까 같이 화이팅하시죠!
고생 많으셨습니다~!
| public RequestHandler find(final HttpRequest request) { | ||
| HttpRequestUri uri = request.getUri(); | ||
|
|
||
| if (uri.contains(LOGIN_URI)) { |
There was a problem hiding this comment.
Login과 Register는 경로에 포함이 되어있는 경우에 바로 login page로 넘어가도록 구현하셨군요!
때문에 다음과 같이 login이 포함된 이상한 주소를 날렸을 때에도 접속이 되네요!
추가적으로 경로가 몇 단계 더 들어가는 경우에는 css도 적용이 안되고요!
이걸 개선해볼 수 있는 좋은 방법은 없을까요?
There was a problem hiding this comment.
아마 기존 요구사항(1단계-3, 쿼리파라미터 파싱)을 지키기 위해서 다음과 같이 구현하신 것 같아요!
url을 쿼리파라미터로 먼저 파싱한 이후에 same으로 비교를 해보면 어떨까요?
There was a problem hiding this comment.
아 추가적으로 쿼리 스트링으로 로그인했을 때 콘솔창에 회원 정보를 찍어주는 내용은 사라졌네요!
로그인관련 요구사항이 변경되었다고 생각해서 해당 내용은 제거하신 걸까요!?
There was a problem hiding this comment.
- 때문에 다음과 같이 login이 포함된 이상한 주소를 날렸을 때에도 접속이 되네요!
추가적으로 경로가 몇 단계 더 들어가는 경우에는 css도 적용이 안되고요!
이걸 개선해볼 수 있는 좋은 방법은 없을까요?
아마 기존 요구사항(1단계-3, 쿼리파라미터 파싱)을 지키기 위해서 다음과 같이 구현하신 것 같아요!
url을 쿼리파라미터로 먼저 파싱한 이후에 same으로 비교를 해보면 어떨까요?
중요한 부분인데 고려하지 못했네요..😓
콩하나 말대로 uri이 동일한지 확인하고 그게 아니라면 404 페이지를 응답해주는게 좋을 것 같아요.
감사합니다 콩하나!👍
- 아 추가적으로 쿼리 스트링으로 로그인했을 때 콘솔창에 회원 정보를 찍어주는 내용은 사라졌네요!
로그인관련 요구사항이 변경되었다고 생각해서 해당 내용은 제거하신 걸까요!?
넵 변경되었다고 생각해서 제거했습니다!
그런데 다시 곰곰히 생각해보니 회원 정보를 콘솔에 찍지 않더라도 로그인 성공 여부를 나타내는 로그 정도는 남기는 것이 좋을 것 같네요🤔
| import org.apache.coyote.http11.response.HttpResponseStatusLine; | ||
| import org.apache.coyote.http11.response.HttpStatusCode; | ||
|
|
||
| public abstract class RequestHandler { |
There was a problem hiding this comment.
RequestHandler를 만들어서 핸들러들을 이쁘장하게 꾸며주셨네요!!!!!
덕분에 코드가 아주 깔끔하게 잘 구성된 것 같아요!
|
|
||
| public abstract HttpResponse handle(final HttpRequest httpRequest) throws IOException; | ||
|
|
||
| HttpResponse getPage(HttpRequest httpRequest, String resourcePath, HttpStatusCode statusCode) throws IOException { |
There was a problem hiding this comment.
이 친구들은 접근제어자가 왜 없나 했는데,
자식 클래스에서만 해당 메소드를 호출할 수 있게끔 구성하기 위해서 다음과 같이 구현하셨던 것이군요!
| this.body = body; | ||
| } | ||
|
|
||
| public static HttpRequest of(final HttpRequestStartLine startLine, final HttpRequestHeaders header, |
There was a problem hiding this comment.
정적 팩토리 메소드를 굉장히 잘 사용하신 것 같아요.
덕분에 메소드 내에 로직이 있다는 것도 잘 느껴지고 통일성도 잘 느껴졌습니다.
| public class HttpResponse { | ||
| private final HttpResponseStatusLine statusLine; | ||
| private final HttpResponseHeaders header; | ||
| private final HttpResponseBody responseBody; | ||
|
|
There was a problem hiding this comment.
이렇게 HttpResponse나 HttpRequest 객체를 만들어서 표현하신 것도 굉장히 인상 깊었습니다!
제 코드가 많이 부끄러워지네요 ㅋㅋㅋㅋ
이렇게 클래스를 만들어서 처리하니까 이를 활용하는 다른 클래스의 코드도 굉장히 깔끔해졌네요!
| public static final int VALUE_INDEX = 1; | ||
| public static final String EMPTY = ""; | ||
| public static final String CONTENT_TYPE_HTML = "text/html;charset=utf-8"; | ||
| public static final String NOT_FOUND_RESOURCE = "/404.html"; |
There was a problem hiding this comment.
여기서 두 가지 질문이 있어요!
1. 정적 필드들을 public으로 열어두신 이유가 있으신가요?
위의 정적 필드들은 자식 클래스에서만 활용되고 있는 것처럼 보이는데요.
public으로 열어두신 이유가 궁금합니다!
2. 핸들러가 많아지고, 요청 url이나 전달되는 값들이 많아지는 경우에는 정적 필드가 점차 많아질 것 같은데 어떻게 분리해볼 수 있을까요?
제 생각에는 자식 핸들러에게 직접 값을 할당하는 방법이 있을 것 같은데요.
하지만 두 개 이상의 핸들러에서 사용하는 정적 필드의 경우(ex, ACCOUNT_KEY는 login과 register에서 사용)에는
중복된 값을 각각 핸들러에게 할당해야할 수도 있습니다.
그래서 RequestHandler와 Login/RegisterHandler 사이에 중간 핸들러를 둬서 공통된 정적 필드를 둘 수도 있겠는데요.
그런데 필드를 구분하기 위해서 상속의 depth를 깊게하는 것은 좋지 않아보이기도 합니다.
이런 상황에서 해당 값들을 구분하기 위해 어떻게 해볼 수 있을까요?
조이의 생각이 궁금합니다.
There was a problem hiding this comment.
- 정적 필드들을 public으로 열어두신 이유가 있으신가요?
단축키로 정적 필드를 빼다보니 접근자까지 미처 확인을 못 했습니다.. 😓
콩하나 말처럼 자식 클래스에서만 활용되고 있기 때문에, 지금 처럼 정적 필드로 가져간다면 default로 변경하는 편이 좋을 것 같습니다.
- 핸들러가 많아지고, 요청 url이나 전달되는 값들이 많아지는 경우에는 정적 필드가 점차 많아질 것 같은데 어떻게 분리해볼 수 있을까요?
정적 필드가 너무 많아진다면 공통 관심사를 가진 필드들을 enum으로 따로 빼서 관리하는 방법을 가져갈 것 같습니다. 현재 상황에서 대표적으로 생각해볼 수 있는 예는 정적 리소스에 대한 URI (/login, /login.html 등)을 별도로 빼보는 경우를 고려해볼 수 있겠네요!
|
|
||
| public class HttpRequest { | ||
| private final HttpRequestStartLine startLine; | ||
| private final HttpRequestHeaders header; |
There was a problem hiding this comment.
현재 헤더 정보는 쓰이고 있지는 않지만 확장성을 위해서 header를 구현하신걸까요~!?
저는 개인적으로 확장성을 고려해서 미리 구현하는 것은 선호하는 스타일은 아닌데요.
HttpRequestHeaders는 HTTP 통신을 한다면 당연히 존재하는 값이기 때문에
사용하지 않더라도 이를 미리 구현한 점이 더 좋아보이네요!
There was a problem hiding this comment.
저도 사용되지 않는 필드나 메서드는 삭제하는 것을 선호하긴 합니다 ㅎㅎ
그런데 HttpRequest에 Header가 없는 것이 부자연스럽다고 느꼈습니다.
큰 비용이 드는 작업이 아니고 확장성 측면에서도 미리 작성하는 것이 좋을 것 같다고 생각해서 이렇게 작성했어요!
꼼꼼하게 놓치지 않고 언급해주셔서 감사합니다 👍
| @Override | ||
| public String toString() { | ||
| return String.format("%d %s", code, type); | ||
| } |
There was a problem hiding this comment.
출력을 하기 위해서 toString을 적극 활용하셨네요!
일반적으로 toString은 뷰의 용도로 사용되기보단 디버깅 용도로 많이 사용하는데요.
toString을 뷰에서 사용하기로 결정하신 이유가 있으실까요?
제 의견도 살짝 이야기하자면 어차피 HttpResponse 객체들은 outputStream에서 출력을 하기위해 만들어지는 용도이니까 toString을 사용해서 적더라도 크게 상관은 없어보입니다!
조이의 의견도 궁금합니다.
There was a problem hiding this comment.
맞습니다.
일반적으로 toString() 은 디버깅 용도로 사용됩니다.
처음에 response를 위한 메서드를 따로 생성할지 toString을 오버라이딩 할지 고민을 많이 했습니다.
작성할 당시에는 해당 포맷이 디버깅 시에도 충분히 사용될 수 있기 때문에 toString을 사용해도 좋을 것 같다고 생각해서 이와 같이 작성했습니다.
미션을 하면서 좀 더 고민을 해봤는데 아무래도 toString()을 사용하기 보다 view를 위한 메서드를 분리하는게 좋을 것 같습니다..🤔
이유를 한번 정리해볼게요.
1. view에 의존하기 때문에 객체의 정보를 온전히 전달할 수 없다.
toString을 재정의할 때는 1) 객체가 가진 정보를 모두 반환할 수 있어야 하고, 2) 포맷의 의도가 명확해야합니다.
toString을 지금처럼 구현한 상황에서 만약 response의 반환 포맷이 변경 된다면 문제가 발생할 수도 있을 것 같아요.
예를 들어 response에 type은 제외하고 code에 다한 정보만 필요하다면, 구현한 toString에서 type 값을 제외하게 될 거에요.
그럼 toString은 객체의 모든 정보를 충분히 반환하지 않게 되겠네요..!!
이 부분은 디버깅할 때 치명적일 것 같네요.
2. 통일성이 깨져 혼선을 야기할 수 있다.
일부의 객체는 response(view) 포맷으로 toString이 정의되어 있고, 나머지 객체는 디버깅을 위해 일반적으로 사용하는 포맷으로 toString이 정의되어 있다면 통일성 측면에서 좋지 않고, 이로 인해 혼선이 발생할 수 있다고 생각합니다.
현재는 혼자 작업을 하고 있지만 함께 협업하는 사람이 있다면 좋은 선택은 아닌 것 같아요!!
(내일의 나는 다른 개발자라는 말이 있듯이.. 미래의 저를 위해서도 좋은 방법은 아닌 것 같네요🥲)
toString은 여러 시스템 디버깅에 사용되기 때문에 고심해서 작성했어야했는데 생각이 조금 부족했던 것 같아요.
콩하나 리뷰 덕분에 다시 한번 생각하게 되었네요.
감사합니다👍
| if (uri.contains(REGISTER_URI)) { | ||
| return new RegisterHandler(); | ||
| } | ||
| return new ResourceHandler(); |
There was a problem hiding this comment.
요구사항이 늘어날수록 분기가 늘어나고 있군요!
이 부분을 해결할 수 있는 방법은 없을까요?
RequestHandler를 상속받고 있는 핸들러를 리턴해주고 있네요.
이러한 특징을 활용해서 handler에게 직접 물어보는 방법은 없을까요?
어떻게 해볼 수 있을까요?
조이의 의견이 궁금합니다 ㅎㅎ
There was a problem hiding this comment.
콩하나~ 의견 감사드립니다!
이 부분에 대해서 해당 코드를 작성할 때도, 3~4단계를 적용하면서도 고민이 많았는데요!
콩하나 말처럼 조건이 늘어나면 분기도 함께 늘어나는 문제가 있을 것 같아요.
그래서 HandlerAdapter에서 Map으로 Handler들을 관리하는 방법으로 적용하는 것이 좋을 것 같다고 생각했습니다.
어차피 request uri가 고정적이라 범용적으로 관리하지 않고 분리하여 사용하는 handler이기 때문에 key-value(uri-handler) 형태로 가져가면 어떨까 생각해보았습니다!
3, 4단계 코드에 적용하였으니 거기에서 더 이야기 해보아요!
| package org.apache.coyote.http11.request; | ||
|
|
||
| public class HttpRequestBody { | ||
| private final String body; |
There was a problem hiding this comment.
더 이상 리뷰를 드릴 내용이 없을 때 드리는 약간 비겁한 리뷰인데요.
컨벤션에 관한 내용입니다. ㅋㅋㅋㅋ
- 어떤 필드들은 클래스 이름 바로 밑에, 또 어떤 필드들은 클래스 이름에서 두 줄 밑에 있는 경우가 있네요!
- 메소드의 파라미터에 final이 있는 경우가 있고 없는 경우도 있는데요. 의도되신걸까요!?
코드가 너무 깔끔해서 이런 부분 밖에 리뷰를 드릴게 없네요 ㅠㅠ
There was a problem hiding this comment.
둘다 급하게 작성하느라 놓친 부분이네요.. 감사합니다 ㅎㅎ 반영하겠습니다!! 🙂
There was a problem hiding this comment.
안녕하세요 조이! 콩하나입니다 ㅎㅎ
전반적으로 너무 깔끔한 코드로 구성되어있어서 코드를 이해하는데,
그리고 웹서버를 이해하는 데에도 도움이 정말 많이 되었습니다!
마치 공식 문서 읽는 것 같았어요!!
이 내용이 BE-5기-코드리뷰에도 올라가겠죠!?
우테코 5기 여러분 조이 코드 보러오세요~ ㅋㅋㅋㅋㅋㅋ
구조도 너무 깔끔하고, 각 클래스별로 역할들도 잘 분배되어있다고 느껴졌습니다, 👍
코드 읽으면서 정말 많이 배웠어요 조이~
1, 2단계 내용은 승인하도록 하겠습니다!
몇 가지 간단한 질문이나 코멘트들을 남겼어요.
대부분 조이의 의견이 궁금해서 남긴 내용들이니까 시간 나실 때 차근차근 남겨주시기 바라요.
3단계가 리팩토링이니까 같이 화이팅하시죠!
고생 많으셨습니다~!
| public RequestHandler find(final HttpRequest request) { | ||
| HttpRequestUri uri = request.getUri(); | ||
|
|
||
| if (uri.contains(LOGIN_URI)) { |
There was a problem hiding this comment.
- 때문에 다음과 같이 login이 포함된 이상한 주소를 날렸을 때에도 접속이 되네요!
추가적으로 경로가 몇 단계 더 들어가는 경우에는 css도 적용이 안되고요!
이걸 개선해볼 수 있는 좋은 방법은 없을까요?
아마 기존 요구사항(1단계-3, 쿼리파라미터 파싱)을 지키기 위해서 다음과 같이 구현하신 것 같아요!
url을 쿼리파라미터로 먼저 파싱한 이후에 same으로 비교를 해보면 어떨까요?
중요한 부분인데 고려하지 못했네요..😓
콩하나 말대로 uri이 동일한지 확인하고 그게 아니라면 404 페이지를 응답해주는게 좋을 것 같아요.
감사합니다 콩하나!👍
- 아 추가적으로 쿼리 스트링으로 로그인했을 때 콘솔창에 회원 정보를 찍어주는 내용은 사라졌네요!
로그인관련 요구사항이 변경되었다고 생각해서 해당 내용은 제거하신 걸까요!?
넵 변경되었다고 생각해서 제거했습니다!
그런데 다시 곰곰히 생각해보니 회원 정보를 콘솔에 찍지 않더라도 로그인 성공 여부를 나타내는 로그 정도는 남기는 것이 좋을 것 같네요🤔
| public static final int VALUE_INDEX = 1; | ||
| public static final String EMPTY = ""; | ||
| public static final String CONTENT_TYPE_HTML = "text/html;charset=utf-8"; | ||
| public static final String NOT_FOUND_RESOURCE = "/404.html"; |
There was a problem hiding this comment.
- 정적 필드들을 public으로 열어두신 이유가 있으신가요?
단축키로 정적 필드를 빼다보니 접근자까지 미처 확인을 못 했습니다.. 😓
콩하나 말처럼 자식 클래스에서만 활용되고 있기 때문에, 지금 처럼 정적 필드로 가져간다면 default로 변경하는 편이 좋을 것 같습니다.
- 핸들러가 많아지고, 요청 url이나 전달되는 값들이 많아지는 경우에는 정적 필드가 점차 많아질 것 같은데 어떻게 분리해볼 수 있을까요?
정적 필드가 너무 많아진다면 공통 관심사를 가진 필드들을 enum으로 따로 빼서 관리하는 방법을 가져갈 것 같습니다. 현재 상황에서 대표적으로 생각해볼 수 있는 예는 정적 리소스에 대한 URI (/login, /login.html 등)을 별도로 빼보는 경우를 고려해볼 수 있겠네요!
| @Override | ||
| public String toString() { | ||
| return String.format("%d %s", code, type); | ||
| } |
There was a problem hiding this comment.
맞습니다.
일반적으로 toString() 은 디버깅 용도로 사용됩니다.
처음에 response를 위한 메서드를 따로 생성할지 toString을 오버라이딩 할지 고민을 많이 했습니다.
작성할 당시에는 해당 포맷이 디버깅 시에도 충분히 사용될 수 있기 때문에 toString을 사용해도 좋을 것 같다고 생각해서 이와 같이 작성했습니다.
미션을 하면서 좀 더 고민을 해봤는데 아무래도 toString()을 사용하기 보다 view를 위한 메서드를 분리하는게 좋을 것 같습니다..🤔
이유를 한번 정리해볼게요.
1. view에 의존하기 때문에 객체의 정보를 온전히 전달할 수 없다.
toString을 재정의할 때는 1) 객체가 가진 정보를 모두 반환할 수 있어야 하고, 2) 포맷의 의도가 명확해야합니다.
toString을 지금처럼 구현한 상황에서 만약 response의 반환 포맷이 변경 된다면 문제가 발생할 수도 있을 것 같아요.
예를 들어 response에 type은 제외하고 code에 다한 정보만 필요하다면, 구현한 toString에서 type 값을 제외하게 될 거에요.
그럼 toString은 객체의 모든 정보를 충분히 반환하지 않게 되겠네요..!!
이 부분은 디버깅할 때 치명적일 것 같네요.
2. 통일성이 깨져 혼선을 야기할 수 있다.
일부의 객체는 response(view) 포맷으로 toString이 정의되어 있고, 나머지 객체는 디버깅을 위해 일반적으로 사용하는 포맷으로 toString이 정의되어 있다면 통일성 측면에서 좋지 않고, 이로 인해 혼선이 발생할 수 있다고 생각합니다.
현재는 혼자 작업을 하고 있지만 함께 협업하는 사람이 있다면 좋은 선택은 아닌 것 같아요!!
(내일의 나는 다른 개발자라는 말이 있듯이.. 미래의 저를 위해서도 좋은 방법은 아닌 것 같네요🥲)
toString은 여러 시스템 디버깅에 사용되기 때문에 고심해서 작성했어야했는데 생각이 조금 부족했던 것 같아요.
콩하나 리뷰 덕분에 다시 한번 생각하게 되었네요.
감사합니다👍
| package org.apache.coyote.http11.request; | ||
|
|
||
| public class HttpRequestBody { | ||
| private final String body; |
There was a problem hiding this comment.
둘다 급하게 작성하느라 놓친 부분이네요.. 감사합니다 ㅎㅎ 반영하겠습니다!! 🙂
|
|
||
| public class HttpRequest { | ||
| private final HttpRequestStartLine startLine; | ||
| private final HttpRequestHeaders header; |
There was a problem hiding this comment.
저도 사용되지 않는 필드나 메서드는 삭제하는 것을 선호하긴 합니다 ㅎㅎ
그런데 HttpRequest에 Header가 없는 것이 부자연스럽다고 느꼈습니다.
큰 비용이 드는 작업이 아니고 확장성 측면에서도 미리 작성하는 것이 좋을 것 같다고 생각해서 이렇게 작성했어요!
꼼꼼하게 놓치지 않고 언급해주셔서 감사합니다 👍
| if (uri.contains(REGISTER_URI)) { | ||
| return new RegisterHandler(); | ||
| } | ||
| return new ResourceHandler(); |
There was a problem hiding this comment.
콩하나~ 의견 감사드립니다!
이 부분에 대해서 해당 코드를 작성할 때도, 3~4단계를 적용하면서도 고민이 많았는데요!
콩하나 말처럼 조건이 늘어나면 분기도 함께 늘어나는 문제가 있을 것 같아요.
그래서 HandlerAdapter에서 Map으로 Handler들을 관리하는 방법으로 적용하는 것이 좋을 것 같다고 생각했습니다.
어차피 request uri가 고정적이라 범용적으로 관리하지 않고 분리하여 사용하는 handler이기 때문에 key-value(uri-handler) 형태로 가져가면 어떨까 생각해보았습니다!
3, 4단계 코드에 적용하였으니 거기에서 더 이야기 해보아요!
안녕하세요 콩하나🫘~ 조이입니다!
1단계는 구현을 다 했지만.. 2단계는 다 구현하지 못 했습니다.. 😓
2단계의 2번 미션인
POST 방식으로 회원가입까지는 구현을 했고, 나머지는 1차 코드리뷰 이후나 step2에서 추가하겠습니다.코드가 깨끗하지 않아서 미리 사죄드립니다..^-^..
혹시 코드에서 이해가 안 되는 부분 있으시다면, 코멘트 혹은 DM 부탁드립니다.
리뷰 잘 부탁드려요!🙂