[3단계] 에버(손채영) 미션 제출합니다. #648
Conversation
| import org.apache.coyote.http11.controller.RegisterController; | ||
| import org.apache.coyote.http11.controller.WelcomeController; | ||
|
|
||
| public enum ControllerMapper { |
There was a problem hiding this comment.
enum을 사용한 방식 신박하네요! 다만 컨트롤러가 늘어나면 Enum에 관리하기 방대해질 수 있으니 향후 새로운 방식을 고민해봐도 좋을거 같아요!
There was a problem hiding this comment.
지금 떠오르는 방법으로는 각 정보들을 특정 클래스의 List로 관리하는 방법이 있을 것 같아요! 하지만 어떤 방법이든 컨트롤러가 추가될 때마다 관련 mapper는 방대해질 것 같은데, 켈리는 이에 대해 어떻게 생각하시나요? 😲 좋은 인사이트 주시면 감사하겠습니다 🙇♀️
| .filter(value -> value.path.equals(path)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new IllegalArgumentException("해당 경로에 매핑되는 컨트롤러가 없습니다.")) | ||
| .controller; |
There was a problem hiding this comment.
매핑되는 컨트롤러가 없다면 NotFoundController를 넘겨주는 것도 괜찮을거 같습니다! 클라이언트 입장에서는 어짜피 없어도 404 응답을 보내야하니 신경쓸 부분이 적어질거 같아요!
There was a problem hiding this comment.
404.html 파일이 그러한 용도로 존재했던 거군요! 바로 추가했습니다 😁
| @@ -36,146 +31,35 @@ public void run() { | |||
| @Override | |||
| public void process(Socket connection) { | |||
|
|
||
| @Override | ||
| public void service(Request request, Response response) throws Exception { | ||
| if (request.hasGetMethod()) { |
There was a problem hiding this comment.
객체에 책임을 위임한 부분 아주 좋습니다! 이제 테스트도 편하게 작성 가능하겠네요!
| } | ||
|
|
||
| private boolean isLoginActive(Request request) { | ||
| boolean hasLoginCookie = request.hasLoginCookie(); |
There was a problem hiding this comment.
Request에 로그인 쿠키 존재 여부를 먼저 물어보고 값을 가져오는 방식도 괜찮지만, 만약 에버외 다른 개발자가 쿠키 존재 유무를 확인하는 메서드 호출을 누락시키고 사용하면 예외가 발생할 여지가 있겠어요..!
loginSessionId를 Optional로 감싸서 반환하는 방법도 있답니다!
request.getLoginCookie().ifPresident(로직 수행);There was a problem hiding this comment.
맞습니다! 이전에 Optional 방식을 차용했다가, LoginController 클래스가 깔끔하게 구현되지 않아서 hasLoginCookie 메서드를 추출했었는데요. 켈리의 말씀대로 hasLoginCookie와 getLoginCookie가 순서에 의존하는 단점이 있었어요. Optional 방식으로 다시 리팩터링해보니 깔끔한 코드가 작성되어서, 해당 방식을 다시 채택하였습니다! 👍
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| public class RequestHeaders { |
There was a problem hiding this comment.
일급 컬렉션 꼼꼼하게 챙겨주셨네요! 진짜 고생 많으셨습니다... 🥹
| } | ||
|
|
||
| public Optional<RequestHeader> getContentLength() { | ||
| return get("Content-Length"); |
There was a problem hiding this comment.
Content-Length같이 정해진 값들은 Enum으로 분리해보시는거 추천 드립니다!
There was a problem hiding this comment.
좋은 생각이에요! HeaderKey 이름의 enum 클래스로 분리하였습니다 :-)
| public static RequestMethod find(String rawMethod) { | ||
| return Arrays.stream(values()) | ||
| .filter(value -> value.name().equals(rawMethod)) | ||
| .findFirst() |
There was a problem hiding this comment.
findFirst와 findAny는 각각 어떤 상황에서 사용하면 좋을까요? 🤔
There was a problem hiding this comment.
필터링 후 일치하는 항목이 오직 하나이고, 스트림이 병렬로 수행되지 않는 상황에서는 둘 중 어느 방식을 사용하든 큰 차이가 없다고 생각합니다. 다만, 스트림이 병렬로 수행되는 경우 findAny가 얕게나마 성능 측면에서 이점이 있다고 해요! 현 상황에서는 미치는 영향이 적다고 생각되지만, 이후 병렬로 진행할 가능성을 고려해 전체적으로 findAny를 채택해볼까 합니다!
| } catch (IllegalArgumentException e) { | ||
| this.value = EMPTY_BODY; | ||
| } | ||
| } // TODO: 테스트를 위해 getValue 메서드가 필요 -> 메시지 파싱 로직을 외부로 추출? |
There was a problem hiding this comment.
불필요한 getter를 열고싶지 않으시다면 전 아래 buildHttpMessage를 활용해서 간접적으로 테스트 해볼거 같습니다!
There was a problem hiding this comment.
급하게 제출하느라 남겨둔 TODO가 꽤 있었네요.. 하하
맞습니다! 저도 간접적으로 테스트를 진행했는데, 클래스 추출이 더 나은 방법인가 고민하고 있었어요. 켈리의 코멘트를 받으니 제 방법에 더욱 확신이 생기네요 ㅎㅎ
| } | ||
|
|
||
| public void addLocation(String value) { | ||
| add("Location", value); |
There was a problem hiding this comment.
역시 buildHttpMessage를 활용해서 간접 테스트를 진행할거 같습니다!
목적이 addLocation의 기능이 잘 동작하는지 확인하는게 테스트의 목적이라면, addLocation이 잘 동작해야지만 원하는 결과가 나오는 buildHttpMessage로 결과로 확인해볼 수 있을거 같네요!
|
안녕하세요 켈리! 남겨주신 리뷰 반영하여 재요청 드립니다! 이번에도 좋은 리뷰 감사해요 :-) |
|
안녕하세요 에버! 3단계는 충분히 진행해주셨다고 생각해서 머지 드리겠습니다! 다음 단계도 파이팅입니다!! |
안녕하세요 켈리!
3단계 리팩터링 미션 진행 후 돌아왔습니다.
1~2단계를 구현하면서 3단계가 리팩터링이라는 사실을 최대한 모른 척 했더니 이번 단계에서 대공사를 진행했어요 ㅎㅎ
이번 리뷰도 잘 부탁드립니다 :)
테스트 커버리지는 아직 많이 낮은 상태예요. 리뷰 먼저 받고 싶어서 급하게 요청드렸는데, 후딱 추가하도록 하겠습니다!