-
Notifications
You must be signed in to change notification settings - Fork 309
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단계] 에코(조은찬) 미션 제출합니다. #482
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
안녕하세요 에코
촉박한 미션일정 + 주말에 이사였는데도 요구사항에 맞게 꼼꼼하게 구현해주셨네요 👏
코드를 보면서 궁금했던 사항 + 이렇게 하면 더 나을거같다는 개선사항들을 의견 리뷰로 달아놨어요!
확인해보시구 더 얘기나눠보면 좋을거같습니다!!
@@ -1,5 +1,5 @@ | |||
distributionBase=GRADLE_USER_HOME | |||
distributionPath=wrapper/dists | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.1-bin.zip | |||
distributionUrl=https\://services.gradle.org/distributions/gradle-7.2-bin.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gradle 버전을 바꾸신 이유가 있을까욥 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앗 이번에 record를 쓰고 싶다는 마음에 버전업을 해봤었는데 생각해보니 레벨1때 요구사항이랑 등등을 생각해보면 맘대로 버전업을 하면 안되겠더라구요.! 그래서 다시 내렸습니다!
setSum(getSum() + 1); | ||
} | ||
|
||
public int getSum() { | ||
public synchronized int getSum() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calculate() 함수에 synchronized 적용이 되면 setSum(), getSum() 메서드에는 동시 접근이 불가능하지 않을까욥 ? 👀
synchronized 가 성능에 많이 영향을 준다고 알고 있어서 최대한 키워드를 적게 쓰는게 좋을거같습니다!
tomcat/build.gradle
Outdated
@@ -3,8 +3,8 @@ plugins { | |||
id "jacoco" | |||
} | |||
|
|||
sourceCompatibility = JavaVersion.VERSION_11 | |||
targetCompatibility = JavaVersion.VERSION_11 | |||
sourceCompatibility = JavaVersion.VERSION_17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거때문에 깃허브 액션 빌드가 실패하는 거 같아요!
(깃허브 액션이 JDK11 쓰는거같아요)
|
||
|
||
@Override | ||
public void service(HttpRequest request, HttpResponse response) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AbstractController 를 상속받는 각각의 controller 에서 service() 를 재정의해주는 것 보다
AbstractController 의 service() 에서 request 가 GET인지, POST 인지에 따라 doPost(), doGet() 을 호출하는 방법은 어떨까요??
그러면 상속받는 Controller 들에서는 doGet(), doPost() 만 재정의해줘도 될거같아요!
doGet(request, response); | ||
return; | ||
} | ||
throw new UnsupportedMethodException(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거를 doPost() 를 호출했을 때 Exception 을 던지는 걸로!
headers.put("Content-Type", type); | ||
return this; | ||
} | ||
|
||
public HttpResponse addLocation(final String location) { | ||
headers.put("Location", location); | ||
return this; | ||
} | ||
|
||
public String build() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response 의 내용을 string 으로 반환하는 메서드인데 네이밍이 더 나은게 있지 않을까욥? ㅋㅋ
build() 는 빌더패턴에서 마지막에 객체 생성해줄때 쓰는 네이밍 같아서..
public class FormUrlEncodedBody implements Body { | ||
private static final String QUERY_DELIMITER = "&"; | ||
private static final String QUERY_SEPARATOR = "="; | ||
private final Map<String, String> parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static 변수랑 다른 변수 사이 개행 확인해주세요!
import java.io.BufferedReader; | ||
import java.io.IOException; | ||
|
||
public interface Body { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EmptyBody, FormUrlEncodedBody 로 Body 를 구분해주신 이유가 있을까요 ??
} | ||
|
||
public HttpResponse addContentLength(final int length) { | ||
headers.put("Content-Length", String.valueOf(length)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HeaderType 이 enum 에 적용되어 있는데 활용해주면 좋을거같아요!
@@ -41,153 +31,22 @@ public void run() { | |||
@Override | |||
public void process(final Socket connection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Http11Processor 객체가 엄청깔끔해졌네요 ! 👍
Kudos, SonarCloud Quality Gate passed! 0 Bugs 0.0% Coverage 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
에코 안녕하세요~
리뷰 반영해주신거 확인했습니다 👏
커밋 하나에 여러 변경이 합쳐져 있어서 작은 단위로 커밋 이력을 남기면 더 좋을 거 같아요!!
요구사항 모두 충족되어 머지할게요!
이번 미션 너무 수고하셨고 mvc 미션도 파이팅입니다👍
public abstract class AbstractController implements Controller { | ||
|
||
@Override | ||
public void service(HttpRequest request, HttpResponse response) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
안녕하세요 제나!
주말에 이사를 하느라 미션에 시간을 많이 쓰지 못했어요.. 그래서 혹시 코드가 더럽더라도 이해 부탁드립니다 ㅜ
전체적으로
감사합니다~!