Skip to content

2단계 - HTTP웹 서버 구현 - 리뷰 요청#303

Merged
yongdae merged 24 commits intonext-step:headf1rstfrom
headF1rst:step2
Jul 31, 2022
Merged

2단계 - HTTP웹 서버 구현 - 리뷰 요청#303
yongdae merged 24 commits intonext-step:headf1rstfrom
headF1rst:step2

Conversation

@headF1rst
Copy link

안녕하세요. Step 2 리뷰 부탁드립니다!

기능 구현을 하면서 몇가지 궁금한점이 생겨 리뷰어님의 의견을 들어보고 싶습니다.

  1. Util 클래스는 객체 지향적 설계를 위반하나요?
    첫번째 요구사항에 맞춰서 requestLine의 URL을 추출하는 Util 클래스를 구현하였습니다.
    Util 클래스에 대해 좀 더 알아보니, Util 클래스가 객체 지향적 설계를 위반한다 라는 포스팅을 접하게 되었고 리뷰어님께서는 어떤 의견을 갖고 계시는지 여쭤보고 싶습니다.

  2. static 변수를 사용하면서 프로그램의 성능 이슈도 고려해야 하나요?
    상수값과 정적 펙토리 메서드를 사용하다보니 static 변수, 메서드의 사용 빈도가 많아졌습니다.
    정적 멤버를 많이 만들수록 메모리 누수, 성능 이슈가 생길것 같은데 고려를 해봐야 하는지 여쭤보고 싶습니다.

  3. 어떠한 경우에 커스텀 예외를 던져야 하나요?
    프로젝트를 진행하면서 커스텀 에러를 관리하기 위한 문서를 유지보수하는데 많은 어려움을 겪은 경험이 있습니다.
    그럼에도 커스텀 예외의 경우 오류에 대한 세분화가 가능하다는 장점이 있기 때문에 적절히 사용하면 도움이 될것이라고 생각하는데 그 적절한 경우를 감 잡지 못하겠습니다. 리뷰어님께서는 어떻게 예외를 관리하는지 궁금합니다.
    (추가로 에러 메세지 또한 상수화 해야할지 여쭤보고 싶습니다.)

@yongdae
Copy link

yongdae commented Jul 30, 2022

리뷰에 앞서 먼저 질의주신 부분에 대해 답변 드립니다.

Q. Util 클래스는 객체 지향적 설계를 위반하나요?
첫번째 요구사항에 맞춰서 requestLine의 URL을 추출하는 Util 클래스를 구현하였습니다.
Util 클래스에 대해 좀 더 알아보니, Util 클래스가 객체 지향적 설계를 위반한다 라는 포스팅을 접하게 되었고 리뷰어님께서는 어떤 의견을 갖고 계시는지 여쭤보고 싶습니다.

A. Util 클래스가 무족건 객체 지향적 설계를 위반한다고 생각하지는 않습니다.
다만, Util 클래스가 객체 지향적인 설계를 방해하는 것 또한 사실이라고 생각합니다.

무분별한 getter, setter가 역할에 따라 객체가 해야하는 일을 못하게 방해하는 코드를 양산하는 것처럼
Util 클래스도 충분한 고민없이 사용하게되면 객체 지향 설계를 방해하는 원인이 됩니다.

이번 단계에서 사용하신 QueryUrlParser 클래스는 문제가 없다고 생각합니다.

Q. static 변수를 사용하면서 프로그램의 성능 이슈도 고려해야 하나요?
상수값과 정적 펙토리 메서드를 사용하다보니 static 변수, 메서드의 사용 빈도가 많아졌습니다.
정적 멤버를 많이 만들수록 메모리 누수, 성능 이슈가 생길것 같은데 고려를 해봐야 하는지 여쭤보고 싶습니다.

A. 네 고려해야합니다.
지금까지의 경험상 성능에 대한 이슈보다는 스레드와 관련한 문제가 많았지만 결론적으로는 고민하시는게 맞습니다.

한 가지 의견을 덧붙이자면, 일반적으로 메모리 누수나 성능 이슈는 개발 단계에서는 발견하기 어렵고
운영에 배포 된 이후에 발견되는 경우가 많습니다.

Q. 어떠한 경우에 커스텀 예외를 던져야 하나요?
프로젝트를 진행하면서 커스텀 에러를 관리하기 위한 문서를 유지보수하는데 많은 어려움을 겪은 경험이 있습니다.
그럼에도 커스텀 예외의 경우 오류에 대한 세분화가 가능하다는 장점이 있기 때문에 적절히 사용하면 도움이 될것이라고 생각하는데 그 적절한 경우를 감 잡지 못하겠습니다. 리뷰어님께서는 어떻게 예외를 관리하는지 궁금합니다.
(추가로 에러 메세지 또한 상수화 해야할지 여쭤보고 싶습니다.)

A. 커스텀 예외라고 일반적인 예외랑 다를 것 없이 예외상황을 처리하거나 인지해야하는 경우에 사용하는게 맞다고 생각합니다.

프로젝트를 진행하면서 커스텀 에러를 관리하기 위한 문서를 유지보수하는데 많은 어려움을 겪은 경험이 있습니다.라고 이야기 해주셨는데
어떤 면에서 커스텀 에러와 문서를 유지보수하는데 어려움이 있는지 의견을 주시면 답변이 가능한 부분인 것 같습니다.

저 같은 경우에는 커스텀 예외는 최대한 피하려고 하는 편이며,
커스텀 예외가 필요한 상황에서는 클래스의 이름과 예외 메시지를 통해 예외를 관리합니다.

*에러 메시지의 상수화

  • 유효성 검증처럼 공통적인 텍스트가 있다.
  • 메시지 지역화가 필요하다.

위에 해당하는 경우가 아니라면 이득을 얻는 부분이 없다고 생각해 배제하는 편입니다.

Copy link

@yongdae yongdae left a comment

Choose a reason for hiding this comment

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

안녕하세요. 리뷰이님! 요구사항을 잘 반영해서 2단계 미션을 열심히 해주셨네요. 💯
그럼 바로 다음 단계로 넘어가도록 하겠습니다.

질의 주신 부분을 포함해서 소소하지만 몇 가지 의견을 남겨 놓았습니다.
다음 미션에서 같이 반영 부탁드릴께요. 수고하셨습니다. 🙇

- 클라이언트에서 서버로 전달되는 데이터의 구조는 name1=value1&name2=value2와 같은 구조로 전달된다.
- 파싱하는 로직 구현을 TDD로 구현한다.

## 기능 구현 목록
Copy link

Choose a reason for hiding this comment

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

기능 요구사항과 구현 목록을 추가해주셨네요. 👍


private QueryParameters(Map<String, String> pairs) {
this.pairs = pairs;
this.pairs = Collections.unmodifiableMap(pairs);
Copy link

@yongdae yongdae Jul 31, 2022

Choose a reason for hiding this comment

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

불변한 객체를 사용하게 수정해주셨네요. 👍


import java.util.Map;

public class UserCreateController implements Controller {
Copy link

Choose a reason for hiding this comment

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

요구사항을 잘 구현해주셨네요. 💯

추가로 HttpRequestTest의 request_resttemplate() 처럼
미션 요구사항에 따라 만든 기능을 검증 할 수 있는 테스트를 추가해보면 좋을 것 같아요.

@@ -0,0 +1,22 @@
package webserver.response;

public enum HttpStatusCode {
Copy link

Choose a reason for hiding this comment

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

Http 응답 코드를 활용해서 HttpStatusCode Enum을 잘 만들어주셨네요. 💯

@yongdae yongdae merged commit 866fb4d into next-step:headf1rst Jul 31, 2022
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