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

[보스독] 4단계 - HTTP 웹 서버 리팩토링 미션 제출합니다. #187

Open
wants to merge 10 commits into
base: yeonnseok
Choose a base branch
from

Conversation

yeonnseok
Copy link

킹뽀대 안녕하세요!
RequestMapping 부분에서 controller 구현체를 사용하고있다보니 분리가 고민이었는데, enum을 사용해보았습니다.
또한 web-server쪽의 handlerbar, view 테스트에서 User모델을 사용하고 있기 때문에 도메인 모델은 web-common이라는 모듈에 넣는게 어떨까 싶어서 web-server/web-common/service-app 이렇게 총 3개의 모듈로 분리해보았습니다.
이번에도 잘 부탁드리겠습니다! 감사합니다 :)

@yeonnseok yeonnseok changed the base branch from master to yeonnseok November 4, 2020 18:01
Copy link

@kingbbode kingbbode left a comment

Choose a reason for hiding this comment

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

안녕하세요!
코멘트 작성해두었습니다.
확인 부탁드려요 :)

- [x] 세션의 고유 아이디는 JDK에서 제공하는 UUID 클래스를 사용해 고유한 아이디를 활용한다.
- [x] 세션 관리를 위한 자료구조는 Map을 사용하며, Map<String, HttpSession>와 같은 구조가 된다. 이 때, 키는 UUID이다.
- [x] service application과 server/servlet부분을 멀티 모듈로 분리한다.
- [x] web-server, web-common, service-app 세개의 모듈로 분리

Choose a reason for hiding this comment

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

모듈 잘 분리해주셨네요! 피드백은 이쪽에 작성하겠습니다 :)

  1. 전체 프로젝트에 적용되어야 할 의존성을 common 모듈을 사용하셨네요. root level 에 build.gradle 을 정의하여 전체 프로젝트에 적용되어야 할 설정을 작성해보는 것은 어떨까요?

  2. web-server 모듈을 완전한 라이브러리로 분리해보시는 것은 어떨까요? 그동안 구현하셨던 웹서버를 어떤 어플리케이션 비지니스에도 녹일 수 있도록 작성하셨을 것이라고 생각합니다. 그러나 현재의 web-server 모듈은 비지니스를 포함하고 있는 것 처럼 보이네요 :) 어떤 어플리케이션에서도 해당 라이브러리를 사용할 수 있도록 조금 더 분리해보시길 바랍니다.

  3. common 이란 네이밍은 다소 위험함이 있습니다. common 이라는 추상적인 네이밍은 네이밍이 가진 추상적인 의미 때문에 어떠한 코드도 들어갈 수 있습니다. 그래서 되도록이면 명시적인 네이밍을 작성할 것을 추천합니다. 실제로 추상적인 네이밍으로 어마무시한 슈퍼 모듈이 만들어지고 이 슈퍼 모듈이 스파게티 의존을 만들어내는 경우들을 많이 보아왔습니다.

  4. 각 모듈은 본인이 작성한 의도가 있긴 할텐데요. 그것을 다른 사람이 보았을 때도 명확히 이해하기 힘들 수 있습니다. 아무리 명시적으로 하려고 해도 추상적인 의미를 갖는 네이밍을 갖기도 할텐데요. README.md 를 각 모듈별로 작성하여, 해당 모듈의 책임과 역할이 어느 범위인지를 명시해주면 좋을 것 같습니다 :)

Copy link
Author

Choose a reason for hiding this comment

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

킹뽀대님! 이번에도 섬세한 리뷰 감사합니다.!
다른 부분은 어느정도 제가 이해를 한 것 같아 반영해보았습니다.
다만 2번에서 web-server모듈을 완전한 라이브러리로 분리하는 피드백을 반영하려다보니, 비즈니스 로직을 포함하고 있는 부분을 캐치하는게 쉽지 않았습니다. ㅠㅠ 그나마 Session에 관한 내용이 요구사항으로 인해 적용되었던 점을 감안해서 Session부분을 다른 모듈로 분리해보았는데요. 의도하신 부분이 이게 맞는지는 잘 모르겠습니다.
또한 Session을 분리하고 있다고 해도 ReuqestHandler등에서 이미 HttpSession을 사용하고 있던터라, 완전한 라이브러리화는 안되고 있는 것 같은데요, 이 부분에 대해서 조금만 더 힌트를 주시면 감사드리겠습니다!

@kingbbode
Copy link

  1. web-server 에 작성된 resource 들에 대한 정리가 필요해보입니다.
  2. web-domain애플리케이션에 사용되는 도메인 객체들을 모아 놓은 모듈 이라고 정의하셨는데요. web-server 는 애플리케이션을 알 필요가 없어보여요.

Copy link

@kingbbode kingbbode left a comment

Choose a reason for hiding this comment

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

코멘트 작성해두었습니다 :)
확인 부탁드려요.

build.gradle Outdated
Comment on lines 45 to 49
project(':web-server') {
dependencies {
compile project(':web-common')
}
}

Choose a reason for hiding this comment

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

각 모듈의 설정까지 뺄 필요는 없을 것 같아요 :)

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