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

Open
wants to merge 43 commits into
base: dongock
Choose a base branch
from

Conversation

Dongock
Copy link

@Dongock Dongock commented Feb 16, 2025

#개요
안녕하세요 리뷰어님 2단계를 바탕으로 리팩토링 완성했습니다.

  • 궁금한점이 저는 2단계에서 주문유형별로 배달, 포장, 매장 주문을 각각 바운디드 컨텍스트로 세팅했습니다. 그런데 이제 코드에서 바운디드 컨텍스트를 기반으로 패키지를 나눈다고 하면 orderservice안에 이 모든것이 들어가있어서 구분이 안되고 그냥 order 도메인을 기반으로 패키지를 구성하게 되더라구요. 그러면 이럴때는 orderservice도 혹시 리팩토링으로 바운디드컨텍스트를 기반으로 분리를 해야하나요?(ex:deliveryOrderService)

양이 많아도 좋으니 최대한 자세한 피드백 부탁드릴께요!

Dongock and others added 30 commits February 4, 2025 22:59
Copy link

@wlroh wlroh left a comment

Choose a reason for hiding this comment

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

동옥님 안녕하세요 😄
미션 고생하셨습니다 👍
코멘트 드린것이 있는데 확인부탁드려요
궁금한 내용이 있으면 편한 채널로 연락주세요!

@wlroh
Copy link

wlroh commented Feb 19, 2025

저는 모델링과 용어사전은 컨텍스트안에 있어야 진정한 힘을 발휘할 수 있다고 생각해요.

용어사전과 글로써 작성한 모델링을 컨텍스트로 묶어보시거나, 각각의 컨텍스트패키지에 Readme.md 를 만들어서 분리하기도 해요.
그림으로 표현한 모델링은 전체적인 모델링이라서 현재 Readme.md 에 위치시키고요

제 개인적인 의견이라서 꼭 반영은 하지 않아도 괜찮아요 😄

추가로, 혹시 서로 다르게 생각하는 것인가해서 그림으로 표현한 모델링에 대해 말씀드리면 상품 도메인은 상품컨텍스트에도 속하지만 메뉴 컨텍스트에도 속해요.
그림으로 표현해주신 모델링은 상품 도메인이 메뉴컨텍스트에 포함된다는 것을 화살표로 표현해주신것인가 생각이 들었어서 말씀을 안드렸는데 다시 보다보니 계속 헷갈리네요 ㅎㅎ

그림에 있는 도메인이 도메인 모델을 의미하는거면 괜찮을것같아요!
그림에서 도형과 색상 그리고 화살표가 무엇을 뜻하는지도 같이 명시해주면 더 좋은 모델링이 될 것 같아요

image

Copy link

@wlroh wlroh left a comment

Choose a reason for hiding this comment

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

동옥님 안녕하세요 😄
피드백 잘 반영해주셨습니다 👍
코멘트 드린것이 있는데 확인부탁드려요!
궁금하신 점 있다면 편하게 연락주세요

Comment on lines +3 to +5
import kitchenpos.menu.domain.model.Menu;
import kitchenpos.menu.domain.model.MenuProduct;
import kitchenpos.menu.domain.repository.MenuRepository;
Copy link

Choose a reason for hiding this comment

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

현재 메뉴에서 상품을 의존하고 있고, 상품에서도 메뉴를 의존하고 있어요.
메뉴컨텍스트와 상품컨텍스트가 양방향으로 의존하고있는데 강결합을 낮출 수 있는 방법에 대해서 고민해보시면 좋을 것 같아요
이번 미션은 패키지를 바운디드 컨텍스트로 구성하는 것이여서 코드를 수정하실 필요는 없고 답변으로 설명주셔도 돼요!

Copy link
Author

Choose a reason for hiding this comment

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

강의때 들었던 거 같은데 직접 참조가 아니라 id로 참조하는 방법이면 될까요??

@@ -1,4 +1,4 @@
package kitchenpos.domain;
package kitchenpos.order.common.domain.model;
Copy link

Choose a reason for hiding this comment

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

공통영역에 주문 엔티티를 구성해주신 의도가 궁금해요
저는 모델링을 이해했을 때 매장, 포장, 배달 컨텍스트에서 주문 도메인도델 공통적으로 메뉴와 주문유형을 가진다는 것으로 생각했는데 주문도메일 모델도 별도로 나와있어서요
이번 미션이 컨텍스트기준으로 리팩터링하는 것이라서 그냥 두신 것일까요?
본격적인 리팩토링은 다음 미션이라서 답변으로 진행해주셔도 돼요!

Copy link
Author

Choose a reason for hiding this comment

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

맨처음에 나누면서는 온전히 생각을 못했어서 공통을 두고 전체적으로 커버할 수 있는 영역을 만들자라고 생각했는데, 다시 생각해보니 제가 분리를 진행하면서 온전히 다 처리할 수 있을거 같네요. 없애도 될 거 같아요. 감사합니다 ㅎㅎ

@@ -0,0 +1,165 @@
package kitchenpos.order.deliveryorder.application;
Copy link

Choose a reason for hiding this comment

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

각각의 주문 유형으로 컨텍스트 분리하셨네요 👍
주문의 경우 유형별로 비슷하지만 다른 부분이 있죠!

@@ -1,25 +1,20 @@
package kitchenpos.application;
package kitchenpos.order.common.application;
Copy link

Choose a reason for hiding this comment

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

OrderService 도 공통에 위치한 의도가 궁금해요 🤔
각각의 컨텍스트로 분리해주셨는데 어떤 역할로 위치해주신 것일까요?

Copy link
Author

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