Skip to content
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주차 미션 / 서버 3조 - 함형주 #1

Open
wants to merge 61 commits into
base: main
Choose a base branch
from

Conversation

hamhyeongju
Copy link
Member

@hamhyeongju hamhyeongju commented Apr 3, 2023

FrontHandler - RequestHandler에서 실행되어 각 요청에 맞는 핸들러를 실행시킴
CustomHandler - 인테페이스, 프론트핸들러에서 기능에 따라 적절한 구현체가 호출되어 실행됨

  • 각 구현체는 html을 응답하거나 요청 처리 후 redirect 시키는 역할을 가짐

HttpRequest - http request message에서 uri, content-length, parameter, header 파싱
HttpResponse - http response message 생성, status 코드, header 동적 생성

hamhyeongju and others added 30 commits April 2, 2023 17:51
상세 요청 처리를 위한 인터페이스
index.html을 읽고 반환하는 핸들러
get 메서드로 회원 가입 처리 및 index.html 반환
get 메서드로 회원 가입 처리 및 index.html 반환
body의 쿼리스트링 파싱 -> Post 요청 처리
signup 요청 시 302 리다이렉트
login_failed.html 응답
로그인 요청 처리
Request HTTP Message 파싱
Response HTTP Message 생성
프론트 컨트롤러
startLine, header, body 작성
HttpRequest, HttpResponse 사용
@hamhyeongju hamhyeongju changed the title 3주차 미션 / 1조 함형주 3주차 미션 / 서버 1조 - 함형주 Apr 3, 2023
Copy link
Contributor

@jung-woo-kim jung-woo-kim left a comment

Choose a reason for hiding this comment

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

안녕하세요!

코드 잘봤습니다. 어렵지 않을까 생각했었는데, 생각보다 빠르게 pr 해주셔서 깜짝 놀랐어요!
코드도 엄청 잘짜주셔서 놀랐습니다 😮😮

다만, 좀 더 객체지향적인 고민과 테스트코드가 하나도 없다는 점이 조금은 아쉬웠어요!
제 코드랑 비교해보시는 시간 가지시면 더욱 도움될 것 같습니다!!
고생하셨어요🙂


response.createStartLine();
response.createHeader();
response.responseBody(bytes);
Copy link
Contributor

Choose a reason for hiding this comment

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

이런식으로 3가지 메서드를 호출하게 하면, 다른 개발자가 HttpResponse 객체를 사용한다고 했을 때 하나만 빼먹을 수 있을 것 같지 않나요?🧐
HttpResponse 객체에 forward(String url) 라는 메서드를 만들어서 이 3가지 메서드를 캡슐화해보는 것은 어떤가요?
이렇게 한다면 각 컨트롤러에서 파일의 내용을 읽어 getBytes해야하는 중복도 없앨 수 있어요!

또 redirect 처리도 보다 자유롭게 할 수 있습니다.


@Override
public byte[] process(HttpRequest request, HttpResponse response) throws IOException {
System.out.println("진입");
Copy link
Contributor

Choose a reason for hiding this comment

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

sysout보단 Logger를 활용해보세요!

public byte[] process(HttpRequest request, HttpResponse response) throws IOException {
System.out.println("진입");
Map<String, String> headerMap = request.getHeaderMap();
String cookieValue = headerMap.get(COOKIE.getValue());
Copy link
Contributor

Choose a reason for hiding this comment

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

headerMap을 getter로 꺼내와서 값을 읽어오는 것보단 request.getHeader(COOKIE.getValue()) 이런 식으로 하는 처리하는 것이 좋아요.
디미터 규칙을 지키려고 노력해보세요!

Map<String, String> headerMap = request.getHeaderMap();
String cookieValue = headerMap.get(COOKIE.getValue());
String[] split = cookieValue.split(";");
System.out.println("split[0] = " + split[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

cookieValue를 parsing하는 일은 util 클래스를 하나 만들어도 더욱 좋을 것 같습니다!🧐

if (split[0].equals(LOGINED_TRUE.getValue())) {
BufferedReader fr = new BufferedReader(new FileReader(file));
String fileData = IOUtils.readData(fr, (int) file.length());
return fileData.getBytes();
Copy link
Contributor

Choose a reason for hiding this comment

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

파일로부터 데이터를 읽어와서 ~ 해주는 작업을 HttpResponse에 캡슐화해보세요!
다른 핸들러들에서도 계속 중복이 일어나는 것 같아요🧐

private static void setStatusCodeAndLocation(String location, HttpResponse response) {
response.setStatusCode(FOUND.getValue());
response.setHeader(LOCATION.getValue(), location);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

다른 곳에서도 redirect가 필요하다면, 이 코드가 똑같이 또 필요하지 않을까요?
HttpResponse에 sendRedirect() 메서드를 만들어보세요!

handlerMappingMap.put(LOGIN.getValue(), new LoginHandler());
handlerMappingMap.put(USER_LIST.getValue(), new UserListHandler());
handlerMappingMap.put(CSS.getValue(), new CssHandler());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

단순히 파일 내용을 읽어오는 작업만 있는 핸들러들은 FowardHandler를 만들어서 FowardHandler(String url)로 하나로 묶을 수 있을 것 같아요!

public static HttpRequest from(BufferedReader br) throws IOException {
HttpRequest httpRequest = new HttpRequest();

String startLine = getStartLine(br);
Copy link
Contributor

Choose a reason for hiding this comment

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

StartLine에는 HttpVersion, url, HttpMethod 정보등이 들어가 있어 하나의 클래스 HttpStartLine으로 분리해보는 것이 어떨까요?🧐

} catch (IOException e) {
log.log(Level.SEVERE, e.getMessage());
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

HttpHeader 클래스를 따로 만들고 그 클래스에 ToString을 override하면 깔끔해질 것 같아요!
또 HttpHeader 클래스는 HttpRequest클래스에서도 사용할 수 있을 것 같아요!

Copy link
Contributor

Choose a reason for hiding this comment

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

Enum 클래스는 관련된 상수끼리 분리해서 작성하는 것이 좋습니다!
지금은 너무 여러가지의 상수를 가지고 있어 무거운 것 같아요

@hamhyeongju
Copy link
Member Author

앗 테스트 생각을 못했네요.. 코드 리뷰 감사합니다!

@hamhyeongju hamhyeongju changed the title 3주차 미션 / 서버 1조 - 함형주 3주차 미션 / 서버 3조 - 함형주 Apr 7, 2023
FrontController 에서 하던 작업 HttpResponse로 이동
FrontController 에서 하던 작업 HttpResponse로 이동
FileIO 작업 HttpResponse로 이동
httpResponseStartLine 추가, redirect() 메서드 생성. body 필드 추가
httpResponseStartLine 추가, redirect() 메서드 생성. body 필드 추가
정적 리소스 관련 요청은 ForwardController 사용하여 범용적으로 응답 가능
ForwardController 생성 위치 변경
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