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 & 4단계] 이레(이다형) 미션 제출합니다 #484

Merged
merged 13 commits into from
Sep 15, 2023

Conversation

zillionme
Copy link

@zillionme zillionme commented Sep 11, 2023

테코톡 -75

객체간 의존성은 위와 같구요,
coyote-> catalina -> jwp 순으로 코드 흐름이 이어가도록 했습니다.

세션은 웹브라우저에서 처음 요청이 왔을 때 생성되는 것이라고 알고 있기 때문에, catalina에서 세션을 생성하고 생명주기를 유지하도록 했습니다

Copy link
Member

@e-astsea e-astsea left a comment

Choose a reason for hiding this comment

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

안녕하세요 이레!

구성도도 그려주셔서 코드를 이해하는데 도움이 되었습니다. 패키지간 흐름을 고려해서 작성하신것도 인상깊었어요 !

몇가지 궁금한 점들에 대해서 질문 남겼습니다. 미션, 팀프로젝트 등등 바쁠텐데 3&4단계 구현하느라 고생하셨어요!

RequestMapping requestMapping = new RequestMapping().addHandler("/", new RootController())
.addHandler("/login", new LoginController())
.addHandler("/register", new RegisterController());
tomcat.start(new DispatcherServlet(requestMapping));
Copy link
Member

@e-astsea e-astsea Sep 11, 2023

Choose a reason for hiding this comment

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

톰캣의 인자로 DispatcherServlet을 전달한 이유가 궁금합니다!

톰캣은 서블릿을 관리하고 실행한다고 저는 생각하는데 내부에서 처리가 되어야 하는게 아닌가 싶어서요!

여기서 받은 값을 커넥터에 servlet을 인자로 가지고 가는 것에대한 이유가 궁금합니당

Copy link
Author

Choose a reason for hiding this comment

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

RequestMapping 정보는 애플리케이션 실행시에 넘겨주는 xml파일 같은 거라고 생각했어요.웹 애플리케이션이 시작될 때 서블릿 컨테이너(톰캣)은 web.xml 파일 또는 자바 기반 설정을 읽어 웹 애플리케이션의 구성을 초기화하는데, 이때 톰캣에 관련 정보를 넘겨주는 것이 맞다고 생각해서 이와 같이 구성하게 되었습니다.

public class StaticFileHandler implements Handler {

@Override
public void handle(final HttpRequest request, final HttpResponse response) throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

개인적인 의견으로는 정적 파일을 핸들링 하는 것은 뭔가 util성에 더 가깝고 정적 파일을 조회, 전달 의 역할만 수행되어야한다고 생각합니다. 그래서 null 일경우에도 예외를 던져서 다른 쪽에서 핸들링을 하는게 더 적합하다고 생각하는데 이레 생각이 궁금합니다~

Copy link
Author

Choose a reason for hiding this comment

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

오션이 주신 좋은 의견에 대해서 생각을 못했던 거라서, 예외를 모아서 다른쪽에서 핸들링 하라는 구조를 제가 잘 이해를 못하는 것 같습니다.
스프링을 쓰는 게 아니니 글로벌 핸들러를 사용할 수도 없고, 어느쪽에서 핸들링 하라는 지 모르겠습니다ㅠㅠ

public HttpResponse(final HttpStatus httpStatus, final List<String> headers, final String body) {
private Session session;

public HttpResponse httpStatus(HttpStatus httpStatus) {
Copy link
Member

Choose a reason for hiding this comment

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

메소드체이닝으로 response를 생성하는 방법 너무 깔끔하고 좋은 것 같아요 ! 배워갑니다~~

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다...ㅜ

String email = requestBody.getContentValue("email");
String password = requestBody.getContentValue("password");
InMemoryUserRepository.save(new User(account, password, email));
response.httpStatus(OK)
Copy link
Member

Choose a reason for hiding this comment

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

성공 시 index.html로 리다이렉트 되는 코드 같아요!

302 상태코드에 location 만 넣어줘도 되지 않을까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

변경했습니다. 감사합ㄴ디ㅏ

.redirectUri("/index.html");
return;
}
response.httpStatus(UNAUTHORIZED)
Copy link
Member

Choose a reason for hiding this comment

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

401, 404, 500 등등 에러가 발생할 경우 현재는 각각 처리를 하고 있는데 이를 한곳에서 에러 핸들링을 하면 더 처리가 편하지 않을까요 ?!

Copy link
Author

@zillionme zillionme Sep 14, 2023

Choose a reason for hiding this comment

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

오션의 의견이 맞는것 같습니다. 예외 처리를 한다면 상위 객체로 예외를 몰아 처리를 해야할 것 같습니다.


public class Constants {

public static final String EMPTY = "";
Copy link
Member

Choose a reason for hiding this comment

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

단순하게 상수만을 위한 클래스라면 enum 타입은 어떠신가요 ?

EMPTY만 따로 상수화를 진행한 이유도 궁금합니다!

Copy link
Author

Choose a reason for hiding this comment

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

Enum 타입은 하나의 클래스인데 문자열 상수를 쓰기 위해서 하나의 클래스 객체로만들 필요가 있을까요? (질문입니다!)
사실, 다른 문자열도 상수화를 진행하려고 했었는데 시간 문제로 위와같이 제출하게 되었습니다.

Copy link
Member

Choose a reason for hiding this comment

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

enum에 상수를 통해 값을 나타내니까 좀 더 응집성 높을 것 같아 의견 남겨봤습니다!

Comment on lines +10 to +11
SVG(".svg", "text/html; "),
;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
SVG(".svg", "text/html; "),
;
SVG(".svg", "text/html; ");

HttpResponse httpResponse = new HttpResponse();
httpResponse.httpStatus(INTERNAL_SERVER_ERROR)
.redirectUri("/500.html");
writer.write(httpResponseParser.generate(httpResponse));
Copy link
Member

Choose a reason for hiding this comment

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

중복코드도 발생하고 serverError만 여기서 처리하는 것도 조금어색한것 같아요! 발생하는 에러들을 응집도 있게 처리하면 더 좋을 것 같습니다!!

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 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 10 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

Copy link
Member

@e-astsea e-astsea left a comment

Choose a reason for hiding this comment

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

3&4단걔도 고생하셨어요 이레!

톰캣 미션은 여기서 approve하겠습니다~

@e-astsea e-astsea merged commit 0b717a4 into woowacourse:zillionme Sep 15, 2023
2 checks passed
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