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단계] 오도(문민혁) 미션 제출합니다. #452

Merged
merged 10 commits into from
Sep 14, 2023

Conversation

odo27
Copy link

@odo27 odo27 commented Sep 10, 2023

안녕하세요 루쿠!
3,4단계 구현해봤습니다~
이번 리뷰도 잘 부탁드립니다 ㅎㅎ

Copy link

@aiaiaiai1 aiaiaiai1 left a comment

Choose a reason for hiding this comment

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

안녕하세요 오도~
리펙터링이랑 구현잘해주셨네요!
예외처리에 대해서 꼼꼼하게 신경을 많이 쓰신거 같습니다
간단하게 의견 남겨드렸는데 확인부탁드려요 고생하셨습니다~~

Comment on lines +17 to +23
public void service(HttpRequest request, HttpResponse response) throws Exception {
try {
Controller controller = requestMapping.getController(request);
controller.service(request, response);
} catch (BaseException e) {
exceptionControllerAdvice.handleException(e, response);
}

Choose a reason for hiding this comment

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

try catch로 컨트롤러에서 발생한 예외를 잡는거 같은데
catch문에서 BaseException대신 ControllerException을 받는건 어떤가요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 controller.service 메서드에서 발생하는 모든 예외들을 해당 try-catch문에서 잡고 있어서 ControllerException만 받으면 LoginController에서 발생하는 AuthException을 못잡을 것 같아요!

Comment on lines +7 to +25
public class FrontController {

private final RequestMapping requestMapping;
private final ExceptionControllerAdvice exceptionControllerAdvice;

public FrontController(RequestMapping requestMapping, ExceptionControllerAdvice exceptionControllerAdvice) {
this.requestMapping = requestMapping;
this.exceptionControllerAdvice = exceptionControllerAdvice;
}

public void service(HttpRequest request, HttpResponse response) throws Exception {
try {
Controller controller = requestMapping.getController(request);
controller.service(request, response);
} catch (BaseException e) {
exceptionControllerAdvice.handleException(e, response);
}
}
}

Choose a reason for hiding this comment

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

프론트 컨트롤러를 만드신 목적이 무엇인가요??
저는 프론트 컨트롤러의 역할이 여러 컨트롤러에서 발생되는 공통부분들을 한번에 처리하는 역할로 생각하고있었는데
여기서는 따로 공통된 부분을 처리하는 로직이 없는거같아서요!

Copy link
Author

Choose a reason for hiding this comment

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

비지니스 로직에서 예외가 발생하더라도 해당 예외 상황을 HttpProcessor까지 내려보내지 않고 싶었습니다. 예외를 처리해주는 기능을 리퀘스트 매핑에서 하는 것이 어색해서 프론트 컨트롤러라는 클래스에 리퀘스트 매핑과 예외 상황 처리라는 2가지 역할을 부여하게 되었습니다. 사실 프론트 컨트롤러라는 이름이 적절한 이름인지는 모르겠습니다 😭

@sonarcloud
Copy link

sonarcloud bot commented Sep 13, 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 5 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

@aiaiaiai1
Copy link

오도 리뷰 반영이랑 답변 확인했습니다~!
리뷰 반영하시느라 고생하셨어요!!
남은 mvc미션도 파이팅입니다!

@aiaiaiai1 aiaiaiai1 merged commit 3238aeb into woowacourse:odo27 Sep 14, 2023
2 checks passed
@kpeel5839
Copy link

루쿠짱 approve 로 작성하시고 merge 해야져어어어엉

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.

3 participants