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단계] 호이(이건호) 미션 제출합니다. #493

Merged
merged 11 commits into from
Sep 12, 2023

Conversation

This2sho
Copy link

안녕하세요. 엔델
3단계 요구사항 까지 구현은 했는데
4단계는 아직 구현을 다 못했습니다..

4단계는 3단계 요구사항과 같이 반영해도 될까요?

@This2sho This2sho self-assigned this Sep 11, 2023
Copy link

@SproutMJ SproutMJ left a comment

Choose a reason for hiding this comment

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

안녕하세요 호이 올려주신것 잘 봤습니다.
잘 작성해주셔서 크게 남길 의견이 없었던 것 같아요.

다만, final을 전반적으로 보강해주시면 어떻까 하는 추가적인 의견이 있고요, 비밀번호를 틀렸을 때 나와야하는 401.html 페이지가 안나오는거 같은데 확인해주시면 좋을 것 같습니다.

Comment on lines 9 to 16
@Override
public void service(HttpRequest request, HttpResponse response) throws Exception {
if (request.isGetMethod()) {
doGet(request, response);
return;
}
doPost(request, response);
}

Choose a reason for hiding this comment

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

여기서 중복 코드가 생기지 않게 한 거 좋은 것 같아요!

import org.apache.coyote.http11.request.HttpRequest;
import org.apache.coyote.http11.response.HttpResponse;

public class ErrorController implements Controller {
public class ErrorController extends AbstractController {

Choose a reason for hiding this comment

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

에러 컨트롤러를 만들어서 핸들링하는 것도 좋은 것 같습니다.

@@ -14,43 +14,20 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

public class LoginController implements Controller {
public class LoginController extends AbstractController {

private static final Logger log = LoggerFactory.getLogger(LoginController.class);

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.

로깅은 사용하지 않고 있습니다. 기존 뼈대 코드에 있던 거라 나뒀는데 삭제하도록 하겠습니다!

Comment on lines +43 to 46
public HttpResponse statusCode(final StatusCode statusCode) {
this.statusCode = statusCode;
return this;
}

Choose a reason for hiding this comment

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

빌더 패턴을 이용해서 객체를 생성하게 한건가요? 좋은 것 같습니다.
다만, 이를 이용해 생성할 때 기본값이 있을까요? 만약 없으면 세팅을 하기 않으면 문제가 발생할까요?

Copy link

@SproutMJ SproutMJ left a comment

Choose a reason for hiding this comment

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

안녕하세요 호이 올려주신것 잘 봤습니다.
잘 작성해주셔서 크게 남길 의견이 없었던 것 같아요.

다만, final을 전반적으로 보강해주시면 어떻까 하는 추가적인 의견이 있고요, 비밀번호를 틀렸을 때 나와야하는 401.html 페이지가 안나오는거 같은데 확인해주시면 좋을 것 같습니다.

Copy link

@SproutMJ SproutMJ left a comment

Choose a reason for hiding this comment

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

안녕하세요 호이 올려주신것 잘 봤습니다.
잘 작성해주셔서 크게 남길 의견이 없었던 것 같아요.

다만, final을 전반적으로 보강해주시면 어떻까 하는 추가적인 의견이 있고요, 비밀번호를 틀렸을 때 나와야하는 401.html 페이지가 안나오는거 같은데 확인해주시면 좋을 것 같습니다.

@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

@SproutMJ SproutMJ left a comment

Choose a reason for hiding this comment

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

미션 고생하셨습니다!

@SproutMJ SproutMJ merged commit 63aafa8 into this2sho Sep 12, 2023
2 checks passed
@SproutMJ SproutMJ deleted the step34 branch September 12, 2023 11:09
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