-
Notifications
You must be signed in to change notification settings - Fork 309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[톰캣 구현하기 - 3, 4단계] 달리 미션 제출합니다 #442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요! 달리
먼저 3, 4단계도 고생하셨습니다!👏
3, 4단계를 진행하면서 코드의 전체적인 구조와 흐름에서 가독성이 좋아져서 코드 리뷰를 하는데 술술 읽혀서 즐거웠습니다.
코드 리뷰를 하면서 달리의 의견을 듣고 싶은 부분에 대해서 피드백을 남겼습니다! 한번 확인부탁드립니다!
final String uri = request.getPath(); | ||
final var cookie = request.getCookie(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 자료형을 나타낼 때 var를 사용하는 경우와 그렇지 않은 경우는 어떤 기준이신가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 원래는 var
를 안쓸려고 했는데 여기에 추가한 것을 깜빡했네요;;;;
var
를 안써봐서 확실한 기준이 아직은 없네요;;
만약 사용한다면 메서드나 기능적인면 혹은 역할이 바뀌지 않는 객체이지만 후에 type이 변경될 수 있을때 사용할 것 같습니다.
0ad5111
FilterChainManager filterChainManager = new FilterChainManager(); | ||
filterChainManager.add(new LoginFilter()); | ||
filterChainManager.getInitialChain().doFilter(request, response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FilterChainManager는 요청이 들어올 때마다 생성이 되는데 이 부분을 싱글톤으로 관리해보는 것은 어떤가요?🤔 그리고 회원가입 페이지도 필터에 추가해도 될거 같아요! 제 생각에는 로그인이 된 상태로 회원가입에 접근할 필요가 없다고 생각합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class DefaultFilter implements Filter { | ||
@Override | ||
public void doFilter(Request request, Response response, FilterChain filterChain) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 DefaultFilter는 무슨 역할을 가지고 있는 필터인가요?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultFilter가 아무런 필터를 추가하지 않았을때 npe를 피하기 위해 설정해둔 것인데 이를 initialChain과 lastChain에 초기 생성시 정의하는 것을 잊었네요;;;;;
0fca1b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가하신 것 확인했습니다!
public FilterChainManager() { | ||
this.defaultChain = new Chain(new DefaultFilter()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 FilterChainManager가 생성되고 바로 아무런 filter가 등록되지 않고 dofilter가 실행되게 되면 initialChain이 null이어서 에러가 발생할 것 같습니다. 초기 생성될 때 initalChain과 lastChain을 따로 정의하지 않은 이유는 무엇인가요?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultFilter가 아무런 필터를 추가하지 않았을때 npe를 피하기 위해 설정해둔 것인데 이를 initialChain과 lastChain에 초기 생성시 정의하는 것을 잊었네요;;;;;
0fca1b5
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가하신 것 확인했습니다!
public Controller mapping(Request request) { | ||
RequestMapper requestInfo | ||
= new RequestMapper(request.getMethod(), request.getPath()); | ||
if (request.getPath().contains(".")) { | ||
return new ViewController(); | ||
} | ||
return controllers.entrySet().stream() | ||
.filter(entry -> entry.getKey().equals(requestInfo)) | ||
.findFirst() | ||
.map(Entry::getValue) | ||
.orElseThrow(NoSuchApiException::new); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 HandlerAdapter와 RequestMapper를 서로분리를 해주셨는데요. HandlerAdapter의 mapping 메소드는 RequestMapper에 더 적합한 기능이라고 생각이 듭니다.
제가 이해한 RequestMapper와 HandlerAdapter는 다음과 같습니다.
- RequestMapper: 사용자의 요청(path, method 등)정보를 이를 해결할 수 있는 컨트롤러를 찾는 역할
- HandlerAdapter: 정해진 컨트롤러를 실행할 수 있는 어뎁터를 찾는 역할
즉, mapping메소드는 현재 요청의 path와 method를 통해 실행할 컨트롤러를 찾는 것이기에 이부분은 RequestMapper에서 처리를 하는 것이 좋지 않을까요?
이 부분에 대한 달리의 생각을 듣고 싶습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequestMapper
는 각 컨트롤러가 담당하는 method와 url을 저장하는 값객체인데 제가 클래스명을 제대로 정하지 않아서 헤깔릴수도 있겠다 생각이 드네요. 정확한 역할은
- RequestMapper: path와 method를 통해 equals와 hash를 정의하고 가지고 있음
- HandlerAdapter: Map<RequestMapper,Controller>로 사용자의 요청과 requestMapper를 매칭하고 해당하는 컨트롤러를 찾아줌
입니다. 때문에 requestMapper의 클래스명을 바꾸는 것이 어떤가 생각합니다.
def103c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
클래스 명칭을 수정하니 보다 명확하게 이해가 됩니다. 👍 개인적으로 궁금한 부분은 path만이 아닌 method까지 포함해서 controller를 찾는것 인데요. 그 이유는 controller 내부적으로 doPost와 doGet로 나누어져서 처리가 되기 때문에 path로만 controller를 찾는 것도 괜찮지 않을까 생각합니다.
public enum HttpMethod { | ||
GET("GET"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 개행 한줄 추가해 주세요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 개행이 두줄 입니다!😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public class InMemorySession { | ||
private static final Map<User, UUID> session = new HashMap<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한줄 개행 추가부탁합니다!
혹시 UUID가 키 값이 아니고 User가 키 값인 이유가 있을까요? 저라면 UUID를 키값으로 가지고 저희가 필요한 정보인 User를 값으로 했을 것 같습니다. 이 부분에 대해서 달리의 의견이 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 아코 말이 맞는 것 같네요! 단순히 user가 key를 가지는 것이라 생각했는데 생각해보니 uuid를 통해 user를 찾는 것이니 이런형식이 맞겠군요.
3bd9ea4
public class UnauthorizedException extends RuntimeException { | ||
public UnauthorizedException(String message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 개행 추가 부탁드립니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 개행이 두줄입니다!😱
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kudos, SonarCloud Quality Gate passed! 0 Bugs 0.0% Coverage The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17. |
public static FilterChainManager getInstance(){ | ||
if(instance == null){ | ||
synchronized (FilterChainManager.class){ | ||
instance = new FilterChainManager(); | ||
} | ||
} | ||
return instance; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
싱글톤을 잘 구현해주셨네요👍
코드 컨벤션은 잘 안 지켜졌네요..😥
public static FilterChainManager getInstance(){ | |
if(instance == null){ | |
synchronized (FilterChainManager.class){ | |
instance = new FilterChainManager(); | |
} | |
} | |
return instance; | |
} | |
public static FilterChainManager getInstance() { | |
if (instance == null) { | |
synchronized (FilterChainManager.class) { | |
instance = new FilterChainManager(); | |
} | |
} | |
return instance; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
@@ -11,14 +11,19 @@ public class FilterChainManager { | |||
|
|||
public FilterChainManager() { | |||
this.defaultChain = new Chain(new DefaultFilter()); | |||
initialChain = lastChain = defaultChain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 두줄로 분리하는 것이 가독성이 좋을 것 같습니다!🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
if(initialChain.equals(defaultChain)){ | ||
initialChain = chain; | ||
initialChain.next = defaultChain; | ||
return; | ||
} | ||
if(lastChain.equals(defaultChain)){ | ||
initialChain.next = chain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
컨벤션이 지켜지지 않았네요..
if(initialChain.equals(defaultChain)){ | |
initialChain = chain; | |
initialChain.next = defaultChain; | |
return; | |
} | |
if(lastChain.equals(defaultChain)){ | |
initialChain.next = chain; | |
if (initialChain.equals(defaultChain)) { | |
initialChain = chain; | |
initialChain.next = defaultChain; | |
return; | |
} | |
if (lastChain.equals(defaultChain)) { | |
initialChain.next = chain; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
public class DefaultFilter implements Filter { | ||
@Override | ||
public void doFilter(Request request, Response response, FilterChain filterChain) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가하신 것 확인했습니다!
if(!response.isFiltered()){ | ||
filter.doFilter(request, response, next); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기도 코드 컨벤션이 지켜지지 않았네요..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅
public FilterChainManager() { | ||
this.defaultChain = new Chain(new DefaultFilter()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
추가하신 것 확인했습니다!
public void service(Request request, Response response) { | ||
if (request.getMethod().equals(HttpMethod.POST)) { | ||
doPost(request, response); | ||
return; | ||
} | ||
doGet(request, response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기서 만약에 put이나 delete 요청이 들어오게 되면 어떻게 동작하게 될까요?🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handler adaptor에 현재 RequestInfo에 put이나 delete 메서드에 해당되는 RequestInfo가 없어 NoSuchApiException 처리가 될것 같네요. 많약 요청에 대한 controller가 필요하다면 AbstractController에 doPut, doDelete 메서드를 추가하여 구현할 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 달리!
제 리뷰를 빠르게 반영해주셨군요!👍 감사합니다!
제가 달리에게 도움이 되는 리뷰어가 되기위해 리뷰를 할 때 최대한 사소한 것도 보다도 달리의 생각을 물어보면서 메타인지를 시켜주는 것을 목표로 했었는데 도움이 되었는지는 모르겠습니다..!
저도 달리 코드를 보면서 많이 배울 수 있는 시간이었습니다
현재 요구사항에 대해서는 충분히 만족한다고 생각해서 approve merge하겠습니다.
다음 미션도 화이팅입니다!💪
p.s 제가 처음에 미처 발견하지 못한 사항에 대해 리뷰 남겨보았는데 그 부분에 대해 확인해보면 좋을 것 같습니다!
안녕하세요 아코!
이번 단계에 리펙토링을 통해 전체적으로 바꿔 보았어요.
request가 들어오면
잘부탁드립니다.