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단계 - 기능 우선 패키지 구성하기 #340

Open
wants to merge 2 commits into
base: mytechnic
Choose a base branch
from

Conversation

mytechnic
Copy link

코드 리뷰 요청 드립니다.
잘 부탁 드립니다 ^^

석봉운(Bongwoon Seok)/커뮤니케이션기술개발팀/SKP added 2 commits September 13, 2023 18:35
- 패키지 변경
- 컨텍스트 분리
- 주문 컨텍스트는 3개로 분리
Copy link

@sah3122 sah3122 left a comment

Choose a reason for hiding this comment

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

패키지 구성 잘해주셨습니다 👍
Root Aggregate을 기준으로 패키지를 나누어 주셨는데 Context 기반으로 패키지를 나눌수도 있기 때문에 몇가지 코멘트 남겨두었어요 😄
확인 부탁드립니다 !

@@ -0,0 +1,153 @@
package kitchenpos.deliveryorders.application;
Copy link

Choose a reason for hiding this comment

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

주문 컨텍스트를 아래와 같이 분리하여 정의해주셨네요 👍

  • deliveryorders
  • eatinorders
  • takeoutorders

이와 같이 분리한 경우 공통적으로 사용하는 도메인 OrderLineItem 이 중복됩니다.
중복을 피하기 위해 분리하는것도 좋은 방법이라고 생각해요 😄
다른 방법으론 주문이라는 컨텍스트 하위에 배달 주문, 매장 식사, 포장 주문을 포함시킬수도 있을것 같습니다.

  • orders
    • deliveryorders
    • takeoutorders
    • eatinorders
    • shared

주문 이란 컨텍스트에서 공유하는 도메인, VO를 shared 또는 common 이라는 패키지에 정의하고 명시적으로 주문의 유형들을 나타낼수 있는 방법이라고 생각하는데 봉운님은 이와 같은 구조를 어떻게 생각하시나요 >?

Copy link
Author

Choose a reason for hiding this comment

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

DDD 진행을 통해 서비스가 나뉠수도 있다는 생각에 결합도를 줄이기 위해 shared 패키지명 작성을 자제하고 있었는데,
shared 라는 패키지가 생긴다면 보다 많은 일이 가능할 것 같습니다. :)

Copy link
Author

Choose a reason for hiding this comment

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

이와 같이 분리한 경우 공통적으로 사용하는 도메인 OrderLineItem 이 중복됩니다.

orders에 따라 OrderLineItem도 분리하여 작성하였습니다.

Copy link
Author

Choose a reason for hiding this comment

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

궁금한게 있습니다. 주문의 경우 3가지 케이스로 나누다보니 findAll 의 기능을 만들기가 애매해졌습니다.
findAll() 대신 findAllByType() 사용이 가능할까요?

@@ -1,7 +1,7 @@
package kitchenpos.application;
package kitchenpos.menugroups.application;
Copy link

Choose a reason for hiding this comment

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

menugroup Aggregate 를 기준으로 패키지를 분리해주셨네요 👍
Menu Context에 MenuGroup을 포함시키는건 어떨까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

Menu Context에서만 사용하고 있으니 그게 좋겠네요

import kitchenpos.menus.domain.MenuRepository;
import kitchenpos.products.domain.Product;
import kitchenpos.products.domain.ProductRepository;
import kitchenpos.products.infra.ProfanityClient;
Copy link

Choose a reason for hiding this comment

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

products 의 Infra 를 사용한다면 어떤 문제점이 있을까요 ?

Copy link
Author

Choose a reason for hiding this comment

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

코드의 결합이 강해질 것 같아요.
느슨한 결합을 위해 Adapter 클래스와 같은 도메인 서비스를 만들어 사용하는게 좋을 것 같습니다.

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