[1 -2단계 - Tomcat 구현하기] 리비(이근희) 미션 제출합니다. #522
Conversation
There was a problem hiding this comment.
리비 안녕하세요~ 감자입니다🥔
저녁에 일정이 있어서 조금 늦은 시간에 리뷰를 남기게 되었네요 👀
코드 잘 봤습니다! 책임 분리 잘해주셔서 리뷰 과정에서 코드 너무 읽기 편했어요 ㅎㅎ
간단한 코멘트 몇 개 남겼으니 시간 날 때 확인해 주세요
[Comment]로 시작하는 리뷰들은 선택적으로 반영해 주시면 됩니다! 변경 요청 사항이 많지 않아서, 다시 리뷰 요청하시면 바로 Approve 하려고 합니다. 혹시 다음 단계 미션 빠르게 진행하고 싶으시면 RC 사항만 반영하고 테스트 3-4단계에서 추가하셔도 괜찮습니다 :)
tomcat/src/main/java/com/techcourse/controller/LoginController.java
Outdated
Show resolved
Hide resolved
| public String getFormData(final String name) { | ||
| String[] pairs = body.split("&"); | ||
| for (String pair : pairs) { | ||
| String[] keyValue = pair.split("="); | ||
| if (keyValue.length != 2) { | ||
| throw new UncheckedServletException("요청의 form-data 형식이 올바르지 않습니다"); | ||
| } | ||
| if (keyValue[0].equals(name)) { | ||
| return keyValue[1]; | ||
| } | ||
| } | ||
| throw new UncheckedServletException(name + "에 해당하는 form-data를 찾지 못했습니다"); | ||
| } |
There was a problem hiding this comment.
폼 데이터에서 필요한 정보가 없거나 본문 형식이 잘못된 경우 예외 처리가 잘 되어 있네요 👍👍
다만 password=11&account=account&email= 처럼 value가 빈 문자열로 들어오는 상황에 대한 처리도 추가되면 더 좋을 것 같아요~
There was a problem hiding this comment.
빈 값이 들어올 수 없다는 것은 저희 서비스의 비즈니스 로직인 것 같아요.
Mdn 문서에서는 html input element에서 공백을 허용합니다. 지금 만들고 있는 것은 Http표준에 맞는 요청을 처리할 수 있는 Tomcat이니 현행 유지하려고 해요.
후에 서비스 로직으로 걷어낼 필요가 있을 때 작업하려고 하는데 하는데 감자가 생각하기에 제가 내린 결론이 어색하면 한 번 더 코멘트 주세요!
There was a problem hiding this comment.
해당 부분이 서비스 비즈니스 로직이라는 의견에 동의합니다 👍 Tomcat 내에서는 Http 표준에 맞게 공백을 허용하고, 서비스 로직 별도로 분리할 경우 추가해 주시면 될 것 같아요
tomcat/src/main/java/com/techcourse/controller/StaticResourceController.java
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public HttpResponse handle(HttpRequest request) throws IOException { |
There was a problem hiding this comment.
| public HttpResponse handle(HttpRequest request) throws IOException { | |
| public HttpResponse handle(final HttpRequest request) throws IOException { |
There was a problem hiding this comment.
final과 var 키워드 사용에 일관성이 없었네요..! 일관성 지키도록 개선했습니다.
|
|
||
| public class LoginPageController implements HttpRequestHandler { | ||
|
|
||
| private static final String LOGIN_SUCCESS_REDIRECT_URI = "http://localhost:8080/index.html"; |
There was a problem hiding this comment.
절대 경로를 사용하신 특별한 이유가 있으실까요? 상대 경로 "/index.html"로 작성하면 호스트나 포트가 바뀌어도 코드 변경 없이 동작하니 더 유연할 것 같습니다!
There was a problem hiding this comment.
@Override
public HttpResponse handle(HttpRequest request) throws IOException {
if (sessionManager.hasSession(request.getSessionId())) {
return HttpResponse.redirect(LOGIN_SUCCESS_REDIRECT_URI);
}
String fileContent = FileUtils.readFile(PAGE_RESOURCE_PATH);
return HttpResponse.ok(fileContent, "html");
}현재 작성된 코드는 위와 같습니다.
/index.html로 작성하면 누군가는 BaseUri를 알아야겠네요.
LoginPageController가 알거나 HttpResponse가 알거나 둘 중 하나인 것 같습니다.
HttpResponse가 아는 것은 제게는 위화감이 있어요. 응답 관련된 객체인데 서버 정보를 아는 것이 책임에 맞지 않다고 생각합니다.
그러면 LoginPageController에서 BaseUri를 알도록 구현할 수 있을 것 같은데요, 이마저도 불필요한 분리인 것 같아 적용하지 않았습니다.
baseUri + redirectUri 로 조합되어야 하는데 한 메서드에서만 사용되는 상수를 굳이 두개로 갈라놓는 느낌이에요.
감자의 의견도 충분히 합리적인 듯 한데요 현재 작성된 코드의 추상화 수준이 높지 않음과 Login이 성공하면 다음으로 redirect할거야라는 정보를 Controller가 알기에 위화감이 없다고 생각하여 현행 유지합니다.
There was a problem hiding this comment.
HTTP/1.1 문서에 Location의 값을 절대 경로로 지정해야 한다고 명시되어 있군요, 리비 덕분에 새로운 정보를 알게 됐네요 ㅎㅎ
baseUri의 경우 RegisterController의 REGISTER_SUCCESS_REDIRECT_URI라는 상수에서도 사용되고 있으니 여러 컨트롤러에서 사용할 수 있도록 따로 분리해 보는 건 어떨까요?
실행되는 애플리케이션의 baseUri인 http://localhost:8080을 아는 책임이 HttpResponse보다 LoginPageController에 더 적합하다는 점은 저도 동의합니다. 그러나 조금 더 유연한 동작을 위해, Controller에서는 헤더에 상대 경로를 설정하고, 절대 경로로의 반환을 WAS에서 해주도록 구현하는 방법도 좋을 것 같아요. HttpResponse Location 헤더가 상대 경로인 경우, HttpRequest Host 헤더를 통해 baseUri를 추출하여 절대 경로로 변경해 주는 거죠. 이 부분은 제안이라 리비가 선택적으로 반영해 주시면 될 것 같아요.
|
안녕하세요 감자 🥔 다만 코멘트 중에는 제 생각만을 남겨놓고 반영하지 않은 부분도 있습니다. 코멘트 내용들 중에는 제가 모르고 있던 것도 있었고 희미하게 잊어가고 있던 것도 있었습니다. 이번 리뷰도 잘 부탁드립니다. |
|
|
||
| public class LoginPageController implements HttpRequestHandler { | ||
|
|
||
| private static final String LOGIN_SUCCESS_REDIRECT_URI = "http://localhost:8080/index.html"; |
There was a problem hiding this comment.
HTTP/1.1 문서에 Location의 값을 절대 경로로 지정해야 한다고 명시되어 있군요, 리비 덕분에 새로운 정보를 알게 됐네요 ㅎㅎ
baseUri의 경우 RegisterController의 REGISTER_SUCCESS_REDIRECT_URI라는 상수에서도 사용되고 있으니 여러 컨트롤러에서 사용할 수 있도록 따로 분리해 보는 건 어떨까요?
실행되는 애플리케이션의 baseUri인 http://localhost:8080을 아는 책임이 HttpResponse보다 LoginPageController에 더 적합하다는 점은 저도 동의합니다. 그러나 조금 더 유연한 동작을 위해, Controller에서는 헤더에 상대 경로를 설정하고, 절대 경로로의 반환을 WAS에서 해주도록 구현하는 방법도 좋을 것 같아요. HttpResponse Location 헤더가 상대 경로인 경우, HttpRequest Host 헤더를 통해 baseUri를 추출하여 절대 경로로 변경해 주는 거죠. 이 부분은 제안이라 리비가 선택적으로 반영해 주시면 될 것 같아요.
| return request.methodEquals(SUPPORTING_METHOD) && | ||
| request.protocolEquals(SUPPORTING_PROTOCOL) && | ||
| request.uriEquals(SUPPORTING_URI); |
There was a problem hiding this comment.
여러 컨트롤러의 supports 메서드에서 코드가 중복되고 있네요, 3단계 리팩토링에서 개선해 주세요~
안녕하세요 감자 🥔
웹 백엔드 6기 리비입니다.
저는 감자랑 많은 이야기 해볼 수 있는 이번 기회 기대하고 있습니다~
부담없이 리뷰주세요!
잘 부탁드립니다 🙇🏻♂️
ps. 시간에 쫓겨서 테스트를 작성하지 못했습니다..! 죄송해요 ㅠ 필요하다고 생각되는 테스트는 다음 리뷰요청까지 채워놓겠습니다 👍