Repository navigation
Conversation
| parseUrl(parts[1]); | ||
| readHeaders(in); | ||
| } | ||
| else { |
There was a problem hiding this comment.
| else { | |
| throw new 다른 예외; |
[제안] Request 생성자의 로직을 보면 else문이 없어도 똑같이 동작하고 있어요.
위 상황에서 if문이 끝나고 바로 예외가 터지도록 작성해보는게 어떨까요?
NullPointerException 또한 명시적으로 적어주지 않아도 requestLine이 null이라면 NullPointerException이 터지는 것 같아요.
[질문] 위 제안에 이어서 널포인터 예외를 터뜨리지 않는다면 어떤 예외를 터뜨리는게 좋을까요?
[질문] 생성자에서 여러 로직들이 실행되고 있어요. 생성자에서 여러 로직을 실행하는게 맞을까요?
There was a problem hiding this comment.
메서드에 잘못된 인자를 전달하면 발생하는 예외로 IllegalArgumentException을 사용하는게 맞는 것 같네요. 그리고 생성자에서는 초기화의 역할을 하는 게 맞는 것 같은데 한 번 간단하게 바꿔보겠습니다.
| String[] parts = requestLine.split(" "); | ||
| this.method = parts[0]; | ||
| this.url = parts[1]; | ||
| this.header = new HashMap<String, String>(); |
There was a problem hiding this comment.
[질문] 헤더를 해시맵으로 초기화 시킬 때 생성자에서 초기화 시키고 있어요.
1번 케이스 필드에서 직접 초기화 private Map<String, String> header = new HashMap<>();
2번 케이스 생성자에서 초기화 this.header = new HashMap<>();
지금은 2번 케이스인데 1번 케이스는 어떻게 생각하시나요?
There was a problem hiding this comment.
생성자에서 초기화하는 것은 무언가 매개변수를 전달받아 초기화할 때 유용할 것 같은데 지금같은 경우는 필드에서 바로 초기화하는 게 낫겠네요 좋은 지적 감사합니다.
| private String url; | ||
| private String body; | ||
| private Map<String, String> cookies; | ||
| private boolean isLogined; |
There was a problem hiding this comment.
[질문] 클래스명을 보면 Request로 우리가 작성할 미션은 http를 다루니 http요청이 들어올 것이라고 예측할 수 있는데요. Request 클래스는 isLogined라는 필드를 가지고 있어요. 로그인에 대한 상태가 이 클래스에게 어울리는 지는 잘 모르겠어요. 이부분에 대해서 어떻게 어떻게 생각하시나요?
|
|
||
| } else { | ||
| this.url = url; | ||
| this.parameters = new HashMap<>(); |
There was a problem hiding this comment.
[질문] 위에 질문에서 필드 초기화를 어디서 해야할지에 대한 질문과 연관되어 있는 코드인 것 같아요.
private 메서드 내부에서 Map<String, String> parameters 필드를 초기화 하고 있어요. 이 코드를 읽지 못했더라면, 이 클래스의 필드와 메서드명만 보고는 필드가 초기화 제대로 되고 있는건지 긴가민가했을 것 같아요.
그렇다면 필드는 어디서 초기화 해야할까요? (질문이 필드에서 초기화해! 라는 느낌을 담고 있긴한데 저도 매번 고민하는데 정답은 없습니다ㅎㅎ)
|
|
||
| private String readBody(BufferedReader in, int contentLength) throws IOException { | ||
| StringBuilder bodyBuilder = new StringBuilder(); | ||
| char[] buffer = new char[1024]; |
There was a problem hiding this comment.
어디선가 1024가 적당하다는 말은 들었는데 이유는 저도 잘 모르겠어서 찾아봤습니다. 버퍼 사이즈에 따라 큰 영향은 있을 것 같진 않아요.
https://www.joinc.co.kr/w/Site/system_programing/File/buffer_size_perf
| public String getHeader(String fieldName) { | ||
| return header.get(fieldName); | ||
| } | ||
| public String getParameter(String parameterName) { |
There was a problem hiding this comment.
[제안] getParameter말고 getQueryString, getQueryparameter라는 이름은 어떠신가요? http를 알고보면 쿼리파라미터일거라고 추측을 할 수 있을 것 같은데, 한번더 코드를 확인하는 이름인 것 같아요.
저는 이렇게도 생각했어요. Json의 value값 일수도 있지 않을까?
| } | ||
| } | ||
|
|
||
| private String getMimeType(String filename) { |
| import webserver.Request; | ||
| import webserver.Response; | ||
|
|
||
| public abstract class AbstractController implements Controller{ |
There was a problem hiding this comment.
[칭찬] Controller를 다형성 활용해서 처리할 생각은 전혀 못했었네요.
추상화 과정을 통해 이후에 작성하신 CreateUser, ListUser, Login 같은 Controller 클래스들도 손쉽게 추가할 수 있는게 좋습니다!
| import webserver.Request; | ||
| import webserver.Response; | ||
|
|
||
| public interface Controller { |
There was a problem hiding this comment.
[질문] AbstractController 추상클래스 외에 추가로 인터페이스 Controller 도 함꼐 구현해주셨는데,어떤 이유로 해당 Interface를 구현한것인지 궁금합니다!! 이유는, 제가 느꼈을때는 AbstractController 추상클래스만으로도 충분하지 않나? 라는 생각도 들었어요. (단순 의견입니다)
3 participants
No description provided.