Skip to content

[톰캣 구현하기 1,2단계] 홍실(홍혁준) 미션 제출합니다.#318

Merged
hyeonjerry merged 61 commits intowoowacourse:hong-silefrom
hong-sile:step1
Sep 4, 2023
Merged

[톰캣 구현하기 1,2단계] 홍실(홍혁준) 미션 제출합니다.#318
hyeonjerry merged 61 commits intowoowacourse:hong-silefrom
hong-sile:step1

Conversation

@hong-sile
Copy link
Member

@hong-sile hong-sile commented Sep 4, 2023

안녕하세요 제리 홍실입니다.

미션을 step2까지 진행을 한 상황이에요.
step3가 리팩터링인데 미션하다보니까 어느정도 하게 되더라고요?

일단 시간이 모자라서 몇몇 부분은 아직 추상화가 안 되있긴한데, 해당 부분은 step3에서 반영하도록 할게요.

꼼꼼하게 봐주시고, 편하게 지적해주세요.

@hong-sile hong-sile self-assigned this Sep 4, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 4, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning 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.
Read more here

@hong-sile hong-sile requested a review from hyeonjerry September 4, 2023 05:35
* try-with-resources를 사용한다.
* java 9 이상에서는 변수를 try-with-resources로 처리할 수 있다.
*/
try (final OutputStream tryOutputStream = new ByteArrayOutputStream(4)) {

Choose a reason for hiding this comment

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

이렇게 할 경우 위에 선언되어 있는 outputStream이 아닌 새로운 OutputStream 객체가 만들어져 try문이 끝나도 tryOutputStream의 자원은 회수되지만 outputStream의 자원이 회수되지 않아 아래 추가로 outputStream.close()를 해주신 것 같아요.
새로운 OutputStream 객체를 선언하지 않고 다음과 같이 위에 선언한 OutputStream 객체를 try-with-resource문에서 사용할 수 있습니다.

try (outputStream){}

Copy link
Member Author

Choose a reason for hiding this comment

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

try wit resources에 대해선 알고 있었는데 저런식으로 해도 되는 지는 몰랐네요 배웠습니다.

* try-with-resources를 사용한다.
* java 9 이상에서는 변수를 try-with-resources로 처리할 수 있다.
*/
inputStream.close();

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.

요것도 몰랐네요 배웠어요.

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class InternalServerErrorHandler implements ExceptionHandler {

Choose a reason for hiding this comment

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

커스텀 예외까지 구현하시고 대단하시네요👍

log.info(user.toString());
final String body = "";
final HttpHeaders headers = resolveHeader(body);
headers.put(LOCATION.getValue(), LOGIN_SUCCESS_LOCATION);

Choose a reason for hiding this comment

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

enum과 상수화를 적절히 사용하여 코드의 가독성을 크게 높이셨네요!!

public class LoginHandler implements HttpHandler {

private static final Map<HttpMethod, HttpHandle> HANDLE_MAP = Map.of(
HttpMethod.GET, LoginHandler::servingStaticResource,

Choose a reason for hiding this comment

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

미션에서는 로그인 페이지 요청과 실제 로그인 요청의 메서드가 GET/POST로 나누어져 있지 않고 모두 GET으로 들어오게 되어 있었습니다.
홍실은 메서드를 분리하여 깔끔하게 처리하셨는데 만약 둘 다 GET으로 받아야 한다는 제약사항이 있다면 어떻게 하실 것인지도 한 번 생각해 보면 좋을 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

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

아 미션 요구사항에선 POST로 바꾸라 되어 있었어요!

this.cookies = cookies;
}

public static HttpCookie from(final String cookieValue) {

Choose a reason for hiding this comment

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

👍


HTML("text/html;charset=utf-8"),
CSS("text/css;"),
JS("text/css;");

Choose a reason for hiding this comment

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

JS인데 css로 되어있네요ㅎㅎ

Copy link
Member Author

Choose a reason for hiding this comment

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

핳;;; 수정하도록 하겠습니다.

import java.util.HashMap;
import java.util.Map;

public class SessionManager {

Choose a reason for hiding this comment

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

요구사항에는 tomcat/src/main/java/org/apache/catalina에 있는 Manager 인터페이스를 구현하라고 나와있어요. 나중에 리팩터링 할 때 구현해 주시면 될 것 같습니다.

Copy link

@hyeonjerry hyeonjerry left a comment

Choose a reason for hiding this comment

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

수고하셨습니다. TDD로 작성하여 시간이 오래 걸리셨을 텐데 끝까지 TDD로 작성하신 점이 대단하시네요!!
전체적으로 클래스를 잘 분리하여 코드를 읽기 너무 편했습니다. 3단계 때 크게 리팩터링 할 것도 없을 것 같네요ㅋㅋ
다만 몇 가지 놓치신 부분이 있는 것 같아 리뷰 남겼습니다. 생각해 보시고 확인 후 3단계 때 반영해 주시면 될 것 같아요.

@hyeonjerry hyeonjerry merged commit 5a0613b into woowacourse:hong-sile Sep 4, 2023
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