[톰캣 구현하기 1 & 2단계] 홍고(홍여진) 미션 제출합니다 #354
Conversation
There was a problem hiding this comment.
홍고 안녕하세요! 우선 코드 보며 많이 배웠습니다! 감사합니다! 스프링의 HandlerMapping이 어떤식으로 작동하는지 자세히 모르고 있었는데 홍고덕에 공부해볼 수 있었네요!
리뷰는 홍고가 작성해주신 리팩토링 계획에 있는 부분은 제외하고 남겼습니다. 어쩌다보니 질문이 많아졌는데 커멘트에 대한 홍고의 의견을 편하게 공유해주시면 함께 성장하는 시간이 되지 않을까 합니다!
(클래스의 역할에 대해 함께 고민해보고 싶은게 많아서 이 부분을 주로 남기게 됐네요!)
- 글 쓰는 재주가 없어 문장이 읽기 힘드실 수도 있을 것 같다는 생각이 드네요.. 혹시 이해 안되시는 부분 있으시면 편하게 디엠 부탁드립니다
++ favicon이 너무 탐나네요. 저도 가져가도 될까요?
| public Object getAttribute(final String name) { | ||
| return values.get(name); | ||
| } | ||
|
|
||
| public void setAttribute(final String name, final Object value) { | ||
| values.put(name, value); | ||
| } | ||
|
|
There was a problem hiding this comment.
포이에게 getter와 setter의 범위는 어디까지인가요?
저는 필드인 Map<String, Object> values 자체를 get하거나 set하는 것을 getter와 setter라고 생각했는데, 포이는 필드가 컬렉션orMap일 때 그 내부의 값을 조회, 변경하는 것도 getter와 setter라고 생각하시는 건가요?
그럼 removeAttribute메소드도 포이의 기준에서는 setter인가요? 궁금합니다!
public void removeAttribute(final String name) {
values.remove(name);
}There was a problem hiding this comment.
- 필드에 컬렉션을 들고있는 일급 컬렉션을 생각해보면 get, set은 단순히 디미터의 법칙을 위해 get set 해준 것이지 메서드의 본질은 getter setter 라고 생각합니다.
- removeAttribute는 컬렉션에서도 remove(index) or remove(element) 로 관리되는 별도의 메서드이니 getter setter가 아니라고 생각합니다. 또, 일반 클래스로 치환하면 값을 수정하는 updateName 같은 역할이므로 getter setter라고 보긴 어려울 것 같네요!
위와 같은 제 기준으로 getter setter 라고 생각해, 위치를 깜빡하고 안 바꾸신 줄 알고 남긴 커멘트이니 홍고의 기준에서 getter setter가 아니라면 그대로 두셔도 좋을 것 같아요!🫡
tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapper.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapper.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/response/ResponseHeaders.java
Outdated
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapper.java
Outdated
Show resolved
Hide resolved
| private static final Map<HandlerStatus, Function<Request, Response>> HANDLERS = new HashMap<>(); | ||
|
|
||
| public HandlerMapper() { | ||
| init(); |
There was a problem hiding this comment.
결국 HANDLERS를 static field 로 관리한다면, Maps.of() 로 선언과 동시에 초기화 해줄 수도 있을 것 같은데 init()이라는 인스턴스 메서드를 만들어주신 이유가 있을까요?
There was a problem hiding this comment.
앗...ㅎㅎ
HANDLERS 변수를 만들 때, HandlerStatus랑 Handler는 객체를 new해서 매번 새로 만들 필요없이 같은 객체를 계속 사용하면 좋겠다~ 싶어서 static field로 생성했습니다.
근데 생각해보니, Handler가 static이면 Handler포함 호출되는 메소드들도 전부 static이어야하는데... 이후에 Handler 인터페이스를 생성한 뒤 구현하는 방식으로 리팩토링하려고 했어서 static메소드는 사용하지 못할 것 같더라고요!
그래서 일단은 전부 new를 사용해서 생성하는 방식으로 변경했습니다 HANDLERS 필드는... static을 지우는 걸 잊었네요...ㅎㅎ
+ HANDLERS를 static이 아니게 변경하고, Tomcat에 HandlerMapper를 주입해 매 커넥션마다 같은 HandlerMapper를 사용하도록 변경했습니다.
refactor: handlerMapper static 제거
refactor: HandlerMapper를 Tomcat에 주입하게 변경
tomcat/src/main/java/org/apache/coyote/http11/handler/HandlerMapper.java
Show resolved
Hide resolved
tomcat/src/main/java/org/apache/coyote/http11/response/ContentType.java
Outdated
Show resolved
Hide resolved
|
Kudos, SonarCloud Quality Gate passed!
|
안녕하세요, 포이!
리뷰 잘 부탁드립니다. 감사합니다!
리팩토링 계획
HanlderMapper클래스 내부의 Handler 메소드 분리RequestLine의httpVersion으로Response의httpVersion을 생성하게 변경