Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84
[HTTP 서버 구현하기 - 2, 3단계] 배럴(김나은) 미션 제출합니다. #84
Changes from 6 commits
2f7ae19
b5e58df
82bbf52
f632ba3
557c4da
abe590b
5a5b175
9fd55e4
80d9537
3aa7310
a2b9540
93970f6
aba6397
34c667c
d656a2b
fd3a119
a804af5
4366da5
9946153
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
단순히 생성자의 인자로 들어가기때문에, 이전의 방식이 더 좋아보여요. 변수 인스턴스화를 하게된 이유가 궁금해요!
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.
사용되지 않아서 불필요하다고 생각했던 것 같아요!ㅎㅎㅎ 지금은 HttpRequest 라는 객체에 모아두었는데, 프로퍼티로 설정하는 것과 변수 인스턴스로 만드는 검프만의 기준은 어떤 것인지 궁금하네요!ㅎㅎㅎ
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.
저는 인스턴스화 된 변수가 다른곳에서 쓰이고, 할당된다면 인스턴스화를 진행하는 거 같아요!
저기서는 생성자의 인자로만 들어가고 있어 남긴 피드백이었습니다. 😊
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.
위와 같은 네이밍은 어떤가요?!
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, value 가 더 명확해 보이네요:)
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.
Http라는 네이밍은 조금 Http 자체를 뜻하는 거 같아요. 위와같은 네이밍은 어떨까요?!
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.
HttpTerms 라고 하였어요 :)
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.
HttpRequest를 만들어 RequestHeader, RequestBody를 넣어주면, 전체 컨트롤러에 공통 적용 가능하고, 역할을 분리 할 수 있을 거 같아요!
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.
좋은 생각입니다! 해당 내용으로 변경할게요 :)
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.
HttpRequest등에 HttpSession을 넣으면, 위와 같이 활용할 수 있을거라 생각해요!
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.
저는 HttpCookie에 이와 같은 로직을 넣었어요
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.
해당 로직 이동해 보았습니다 :)
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.
이부분도 HttpRequest를 만들면 자연스레 사라질 것이라 생각해요.
우선 HttpRequest, HttpResponse를 만들어 현재 로직에서의 책임을 분리하고(ResponseEntity가 HttpResponse의 역할을 한다고 생각하는데, 이 부분을 리팩터링 해보는건 어떨까요?? 😊),
정상 작동을 한다면, HttpSession, HttpCookie에 책임을 할당 해주는 식으로 좋을 것 같아요!
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.
네이밍에서의 Index라는 의미가 어떤 의민지 알 수 있을까요~~?
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.
index 페이지를 의미했었는데 해당 내용도 인자로 받을 수 있도록 변경하였어요 :-D
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.
이렇게 하면, 한군데서만 쓰이는 인스턴스 생성을 막을 수 있을거라 생각해요!
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.
👍 좋은 생각입니다!!
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.
Handler 인터페이스의 메소드니까 handle이라 둬도 의미상 변함없을 거같아요 :)
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.
👍 동의합니다!ㅎㅎㅎㅎㅎ
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.
GetHandler와 같은 피드백입니다!
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.
👍 반영했어요:)
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.
2 만약. equal가 사용되지 않는다면, 재정의를 해야할 필요성은 무엇이 있을까요?
이펙티브 자바 규칙 8 - equals 재정의 / 오버라이드 방법 참고해봐도 좋을 거같아요 :)
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.
구현 중간에 같은 유저가 맞는지 확인하는 로직을 짜느라 넣었는데 해당 로직을 삭제하면서 해당 내용까지 삭제하지 못한 것 같아요! 삭제 처리 하였습니다! :) 예리검프! 👍
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.
Map 자료구조에 값들이 생성될 때만 저장되고 있어요.
Cookie에 값을 넣어주고, Response시에 HttpResponse(코드에서 ResponseEntity.class)에 넣어주는 방식은 어떨까요?
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.
해당부분은 Request 에서 사용되는 것 같은데.. 혹시 어떤 부분을 의도한 건지 좀 더 자세히 설명해주실 수 있을까요? 🙏
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.
HttpCookie가 HttpRequest에서만 있어야 할까? 라는 의문점에서 나온 피드백이었어요!
HttpResponse에도 Cookie를 가지고 있으면 더 자연스러워 보일 거 같았어요 😊
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.
여러 상황을 고려해서 인자로 HttpSession을 넘겨 주는 건 어떨까요?
HttpSession.setAttribute 이후 HttpSessions에 넣어줄 상황도 생길 수 있다고 생각이 들어요
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.
오! 좋은 생각입니다! :)