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단계] 도기(김동호) 미션 제출합니다. #449

Merged
merged 15 commits into from
Sep 12, 2023

Conversation

kdkdhoho
Copy link

안녕하세요 하마드! 3, 4단계 제출합니다!

이번에 3단계를 적용하기 위해 구조를 바꾸다보니 1단계부터 다시 구현하게 되었는데요.
중간에 Request 객체로 파싱하는 과정에서 실수가 있어 삽질을 많이 하게 되었네요 😢
또 3, 4단계를 진행하며 최대한 객체도 잘게 만들고 역할도 나눈다고 나눴지만 못보고 놓친 부분이 있을 것 같아요. 이런 부분 잘 봐주시면 감사하겠습니다 🙇‍♂️

그럼 잘부탁드려요 :)

@kdkdhoho kdkdhoho self-assigned this Sep 10, 2023
Copy link

@rawfishthelgh rawfishthelgh left a comment

Choose a reason for hiding this comment

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

안녕하세요 도기 하마드입니다!
자칫 사소하게 넘어갈 수 있는 부분에 대해서도, 역할 분리를 고민하신 흔적이 짙게 남아있는 것 같습니다ㅎㅎ
다만 지금 로그인 페이지가 렌더링이 안 되고 있는 것 같은데,
이 부분만 변경되면 merge 될 것 같아요!
추가로 몇 개 의견을 묻는 코멘트도 남겨 놨습니다 :)

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

public class LoginService {

Choose a reason for hiding this comment

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

A

단순 질문입니다!
저 같은 경우는 로그인/혹은 회원가입 로직이 비대하지 않다고 판단해 컨트롤러 영역에 위치시켰는데, 서비스 클래스를 도입하신 이유가 있을까요??

Copy link
Author

Choose a reason for hiding this comment

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

물론 하마드 말씀대로 로직 자체는 비대하지 않지만, 습관적으로 분리한 것 같아요 🤣

Comment on lines 16 to 17
Optional<User> foundUser = findByAccount(account);
User user = foundUser.orElseThrow();

Choose a reason for hiding this comment

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

A

현재 상황에서 account에 일치하는 유저가 없다면, orElseThrow();에서 NoSuchElementException 이 터질텐데, 따로 예외 명시를 하지 않은 이유가 있는지 궁금합니다ㅎㅎ
(사실 어떻게 해도 컨트롤러에서 401로 catch 되기에 큰 문제가 없을 것 같긴 하네요)

Copy link
Author

Choose a reason for hiding this comment

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

1, 2단계에서도 마찬가지였지만 예외 처리에 대한 신경을 거의 쓰지 않았습니다. 동시에 말씀하신대로 외부에서 catch로 예외를 처리해주기에 크게 의식하진 않았어요.

Comment on lines 8 to 10
boolean support(HttpRequest request);

void handle(HttpRequest request, HttpResponse response);

Choose a reason for hiding this comment

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

C

뼈대 코드의 service, doPost, doGet 형태를 사용하지 않고,
위와 같은 형태로 사용한 이유가 궁금하네요!

Copy link
Author

Choose a reason for hiding this comment

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

doPost, doGet 메서드를 구현하라는 가이드라인을 보지 못했어요 ㅜㅜ
수정하겠습니다..!

import org.apache.coyote.session.Session;
import org.apache.coyote.session.SessionManager;

public class LoginPageController implements Controller {

Choose a reason for hiding this comment

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

R

스크린샷 2023-09-12 오전 12 40 23 로그인 페이지가 나타나지 않고 있네요ㅜㅜ

Copy link
Author

Choose a reason for hiding this comment

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

image

Copy link
Author

Choose a reason for hiding this comment

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

농담이고 이따 한번 확인하러 갈게요..ㅎ 죄송합니다 🤣

this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
this.executorService = Executors.newFixedThreadPool(maxThreads);

Choose a reason for hiding this comment

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

A

이건 그냥 참고사항인데요!
LMS "생각해보기"에 보시면,

최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?
라는 문장이 있어요!

Executors.newFixedThreadPool(maxThreads);
를 사용하면 작업 큐의 사이즈가 Ìnteger.MAX_VALUE` 로 설정이 되는데,
어떻게 이걸 100으로 할 수 있을지 생각? 정도만 해보셔도 좋을 것 같네요

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다!! 해당 방법에 대해 생각해보고 적용까지 한번 해볼게요 :)

@sonarcloud
Copy link

sonarcloud bot commented Sep 12, 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 6 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

@rawfishthelgh rawfishthelgh left a comment

Choose a reason for hiding this comment

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

리뷰 내용 충분히 반영해주신 것 같아 approve & merge 하겠습니다!
고생하셨어요 도기ㅎㅎ

@rawfishthelgh rawfishthelgh merged commit fb539ad into woowacourse:kdkdhoho Sep 12, 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