Skip to content

Conversation

@ehBeak
Copy link
Member

@ehBeak ehBeak commented Sep 11, 2024

안녕하세요. 알파카🦙
리뷰이 배키입니다 :)

3단계 리팩토링 진행해서 제출합니다!
3단계도 잘 부탁드립니다~ 😊

3단계를 진행하면서, 변경된 부분은 다음과 같습니다.

  • HttpRequest, HttpResponse를 조금 더 세분화했습니다.
  • 테스트 코드를 추가했습니다.
  • (이전에 Controller를 따로 분리했기 때문에 구조가 크게 변경되지는 않았습니다.)

@ehBeak ehBeak self-assigned this Sep 11, 2024
@ehBeak ehBeak requested a review from slimsha2dy September 11, 2024 12:36
Copy link
Member

@slimsha2dy slimsha2dy left a comment

Choose a reason for hiding this comment

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

안녕하세요 배키~
기존에도 리팩토링이 아주 잘 되어 있던 코드라 크게 피드백드릴 부분은 크게 없습니다.
다만 controller 패키지는 사용자가 개발하는 부분인데 apache/catalina에 있는 건 어색하다고 생각합니다. 사용자가 구현하는 패키지, 즉 Application 클래스가 포함된 패키지에 있는 편이 좋을 것 같은데 이에 대한 배키의 생각이 궁금합니다.

  • 그런데 또 정적 리소스를 관리하는 DefaultController의 기능은 톰캣의 기능으로 알고 있어 이는 따로 관리가 되어야 할 것 같네요.

그 외에 피드백은 크게 드릴 부분도 없었고 해서 가볍게 봐주시면 좋을 것 같습니다!

public class HttpRequest {

private final String request;
private static final String SP = " ";
Copy link
Member

Choose a reason for hiding this comment

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

SP는 separator를 뜻하는 걸까요?
좀 더 의미가 명확하면 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

SPACE를 뜻하는 것입니다. 반영하겠습니다 :)

import java.util.Arrays;
import java.util.NoSuchElementException;

public enum HttpVersion {
Copy link
Member

Choose a reason for hiding this comment

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

버전도 상수화한 점이 아주 좋습니다! 🐤

Comment on lines 156 to 160
public Session getSession(boolean create) {
Session session = new Session(UUID.randomUUID().toString());
SessionManager sessionManager = SessionManager.getInstance();
sessionManager.add(session);
return session;
Copy link
Member

Choose a reason for hiding this comment

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

저번엔 지나쳤던 부분이긴 한데..
create 변수는 어디 사용되고 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 현재 코드에서 쓰이고 있지 않습니다..😅 없애야 겠다고 생각했는데, 저도 미뤄두다 삭제를 안했네요.. 수정하겠습니다!

Comment on lines 186 to 187


Copy link
Member

Choose a reason for hiding this comment

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

빈틈의 실 발견

Copy link
Member Author

Choose a reason for hiding this comment

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

어이쿠..

Comment on lines 52 to 53


Copy link
Member

Choose a reason for hiding this comment

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

약점 포착

Copy link
Member Author

Choose a reason for hiding this comment

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

세상에...

@ehBeak
Copy link
Member Author

ehBeak commented Sep 12, 2024

안녕하세요 알파카!

알파카의 말대로 controller 패키지가 어색하다고 생각합니다.
사실 현재 구현한 controllerservlet의 역할과 같다고 생각했습니다. 그래서 apache/catalina/servlet 패키지에 ~~Controller 클래스들을 넣어두었습니다. (기존 네이밍은 ~Servlet이었습니다!)

생각 해보니, servletcontroller의 역할을 분리하지 못한 것 같네요.
controller에서 DispatcherServlet을 구현하고 controller와 매핑하는 로직이 필요할 것 같아요.

그런데 현재는 mvc를 구현하는 것보다 톰캣을 구현하는데 더 집중해할 것 같아서 고민이 됐습니다... (mvc는 이후 미션인 것 같아서요!)
일단은 단순하게 각 servlet에서 controller를 호출하는 방식으로 구현해 봤습니다.

확인 부탁드려요!

Copy link
Member

@slimsha2dy slimsha2dy left a comment

Choose a reason for hiding this comment

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

학습 테스트도 모두 잘 돌아가는 것 확인했습니다~
3단계 수고하셨고 4단계까지 화이팅해보아요 😄

@slimsha2dy slimsha2dy merged commit cfd507b into woowacourse:ehbeak Sep 13, 2024
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