step3 (매장 식사 주문) #246

yuhwanwoo · 2023-09-29T02:23:37Z

리뷰어님 안녕하세요.

배달 주문과 매장 주문에 대한 리팩토링을 진행했습니다.

질문이 일단 있는데요!

배달과 매장 주문 컨텍스트에서 menu client를 각각 작성했는데 사실 같은 기능인데

공통적으로 주문에 대한 패키지를 만드는 것이 맞을까요?

감사합니다! 🙇

wlroh · 2023-10-01T08:58:59Z

환우님 안녕하세요 :)

배달과 매장 주문 컨텍스트에서 menu client를 각각 작성했는데 사실 같은 기능인데
공통적으로 주문에 대한 패키지를 만드는 것이 맞을까요?

이부분은 정답이 없는 것 같아요. 팀 개발문화와 몇분이서 개발을 하는지에 따라 많이 달라질 것 같아요.
현재는 같은 기능이라서 공통 기능으로 만들어도 괜찮다고 생각해요. 다만, 공통으로 기능이 들어갈 경우 꼼꼼하게 관리할 수 있는 환경인지 고민을 먼저 해보시는 것을 추천드려요!
힘들다고 판단되시면 지금처럼 각각 구현하는 것이 더 좋지 않을 까 생각이 들어요 😄

저는 공통기능은 양날의 검이라고 생각해요. 잘 사용하면 유지보수, 생산성 모두 좋아지지만 잘못사용하면 각각 분리해서 사용하는 것보다 더 힘든 상황이 올수 있기 때문이죠 😢

wlroh

환우님 안녕하세요 :)
미션 고생하셨습니다 👍
코멘트 달아놓은 것이 있는데 확인 부탁드려요!

wlroh · 2023-10-01T08:29:45Z

src/main/java/kitchenpos/deliveryorders/domain/DeliveryOrder.java

+    @Column(name = "type", nullable = false)
+    @Enumerated(EnumType.STRING)
+    private OrderType type;


각각의 주문별로 바운디드 컨텍스트로 나누고 각각의 Table 도 분리해주신 것으로 보여져요. 😄
그렇다면 OrderType 을 제거해보는 것은 어떤가요?

wlroh · 2023-10-01T08:34:06Z

src/main/java/kitchenpos/deliveryorders/domain/MenuClient.java

+    Optional<Menu> findById(UUID id);
+    List<Menu> findAllByIdIn(List<UUID> ids);


현재 주문 -> 메뉴로 한방향으로만 결합되어 있지만, 주문에서 메뉴 엔티티에 관심이 있기 보다는 메뉴의 가격과 노출여부, 등록되어 있는 메뉴여부에 더 관심이 있지 않을까 생각해요.
반환할 때 주문에서 필요한 정보만 VO를 새로 만들어서 리턴하는 것은 어떤가요?

wlroh · 2023-10-01T08:46:31Z

src/main/java/kitchenpos/eatinorders/application/EatInOrderService.java

+    @Transactional
+    public EatInOrderDto complete(UUID orderId) {
+        EatInOrder eatInOrder = eatInOrderRepository.findById(orderId)
+                .orElseThrow(NoSuchElementException::new);
+        eatInOrder.complete();
+
+        OrderTable orderTable = orderTableRepository.findById(eatInOrder.getOrderTable().getId())
+                .orElseThrow(NoSuchElementException::new);
+        if (!eatInOrderRepository.existsByOrderTableAndStatusNot(orderTable, OrderStatus.COMPLETED)) {
+            orderTable.clear();
+        }
+        return ConvertUtil.convert(eatInOrder, EatInOrderDto.class);
+    }


매장주문이 완료되었을 때 도메인 로직이 어떻게 수행되어야한다고 생각하실까요? 🤔

전 주문 상태 Served로 수정했습니다! 🙇

말씀드리고 싶었던 것은 현재 로직 중에 도메인로직으로 보이는 부분이 있어서 도메인 계층으로 위임하면 어떨까 의견을 드려본 것이였어요 😄
주문완료하는 로직에서 주문테이블과 매장주문 애그리거트간의 협력도 필요하다고 봐요.
그렇기 때문에 도메인 계층으로 위임할 때 여러 고민도 해보시면 좋을 것 같아서요

OrderTable의 추가 Order는 OrderTable에 계속 쌓이며 모든 EatInOrder가 완료되면 EmptyTable이 된다.

Completed 가 되려면 전 주문은 Served 상태여야 한다.

모델링 해주신 것을 봤을 때는 지금처럼 구현한 것도 좋지만, 응용계층에서 도메인에 대한 제어가 더 많아질 것 같아요

말씀해주신 부분을 생각해보았는데요!

제 생각에는 사진 처럼
public void complete(EatInOrderRepository eatInOrderRepository)
메소드에 EatInOrderRepository를 파라미터로 받아야할 것 같은데 제가 이해한 게 맞을까요...?

스크린샷에 올려주신 코드가 매장 주문 도메인인가요? 🤔
도메인에 레포지토리를 주입하거나 파라미터로 넘겨주는 행위는 저는 지양하고 있어요
루트 애그리거트의 핵심역할이 애그리거트의 일관성이 개지지 않다록 하는 것인데 레포지토리를 주입받거나 파라미터로 넘겨주게되면
타 애그리거트의 일관성이 깨질 우려가 있어요

네 매장주문 도메인인데요!

말씀해주신 방향으로 고민을 해봤는데, 도메인 로직으로 옮기려면

이런식으로 작성해야 하지 않나해서 repository에 대해 질문드렸었습니다!

그리고 제가 현재 고민하고 있던 부분도 이에 대한 것중 하나인데
구현하는 로직중 운영정책 등록 시 제목이 없다면 해당 정책의 Category명 + 숫자(숫자는 등록 시 마다 증가)
라는 요구사항이 존재하는데요. 해당 부분을 구현하기 위해서 고민을 해본 결과

Terms의 정적 메소드에 termsRepository 를 파라미터로 받도록 할 수 밖에 없을 것 같은데
말씀하신 방향대로라면 어떻게 진행하는 것이 맞는 방향인걸까요...?

wlroh · 2023-10-01T08:50:55Z

src/main/java/kitchenpos/eatinorders/tobe/domain/order/EatInOrder.java

+    @ManyToOne
+    @JoinColumn(
+            name = "order_table_id",
+            columnDefinition = "binary(16)",
+            foreignKey = @ForeignKey(name = "fk_orders_to_order_table")
+    )
+    private OrderTable orderTable;


현재 구현해주신 구성을 보면 매장 주문 컨텍스트에서는 크게, 매장주문 애그리거트와 주문테이블 애그리거트로 구성되어 있는 것으로 보여져요 😄
매장주문 애그리거트에서 주문테이블 애그리거트를 직접참조하고 있도록 구현해주셨는데요, 혹시 어떤 의도로 직접참조로 구성해주신 것일까요? 🤔
직접참조와 간접참조에 대해서도 같이 고민해보시면 좋을 것 같아요

사실 직접 참조를 제거 할까 고민을 했었는데요, 오더 테이블 같은 경우에 오더와 생성과 소멸을 같이 한다고 보고 직접 참조로 작성했었습니다!

저는 환우님이 매장주문 컨텍스트에서 루트 애그리거트를 매장주문과 주문테이블 2개로 구성해주신것으로 저는 생각했어요~
저는 직접참조와 간접참조에 대해 루트애그리거트 내에서는 직접참조를 하고 루트 애그리거트 간은 간접참조하는 것이 좋다고 생각하고 있어요
루트 애그리거트의 핵심역할 중 하나가 애그리거트의 일관성은 깨지지 않도록 하는 것이기 때문에 타 애그리거트를 참조하게 되면 타 애그리거트를 수정하는 이슈가 발생할 수 있지 않을까해요.

제가 현재 구성하고 있는 구조가 루트 애그리터트가 매장 주문이고 속한 객체중 하나가 주문 테이블이라고 생각하고 있었는데
제가 착각한 부분이 있었던 것 같아요!

order와 orderTable이 각각 루트 애그리거트라고 생각하면 되는걸까요?

이부분은 어떻게 모델링을 하는지에 따라 달라지게 될 것 같아요. 정답이 없는부분이여서 환우님 말씀이 논리적이라고 생각되시면 그대로 유지하는거죠~

다만, 현재 환우님 구현주신 코드로 보게되면 매장주문 루트애그리거트와 주문테이블 애그리거트로 보여져요.
조금 더 쉽게 보면 지금까지 경험하기로는 루트애그리거트 기준으로 레포지토리가 생성되더라구요
루트 애그리거트를 애플리케이션 관점에서 일관성을 유지하기 위해서죠.

그렇다면, 환우님 코드를 보게되었을 때 주문테이블과 매장주문이 각각의 루트애그리거트이지 않을까 생각이들어요.
환우님 말씀처럼 구현하면 주문테이블의 대한 레포지토리 없이 매장주문이 관리해주어야하지 않을까 생각이 들었어요.

아 이해했습니다 감사합니다!

wlroh · 2023-10-01T08:52:12Z

src/main/java/kitchenpos/eatinorders/tobe/domain/ordertable/OrderTable.java

@@ -0,0 +1,71 @@
+package kitchenpos.eatinorders.tobe.domain.ordertable;


매장 주문 애그리거트와 주문테이블 애그리거트를 패키지를 통해 분리해주셨네요 👍 👍

wlroh · 2023-10-01T09:02:31Z

README.md

+| 한글명 | 영문명 | 설명                                             |
+| --- | --- |------------------------------------------------|
+| 배달 | delivering | 배달원이 매장을 방문하여 배달 음식의 픽업을 완료하고 배달을 시작하는 단계      |
+| 배달 대행사 | delivery agency | 준비한 음식을 고객에게 직접 배달하는 서비스                       |
+| 배달 완료 | delivered | 배달원이 주문한 음식을 고객에게 배달 완료한 단계                    |
+| 서빙 | served | 조리가 완료되어 음식이 나갈 수 있는 단계                        |
+| 완료 | completed | 배달이 완료된 후 주문을 마무리 하는 단계                        |
+| 접수 | accepted | 주문을 받고 음식을 조리하는 단계                             |
+| 접수 대기 | waiting | 주문이 생성되어 매장으로 전달된 단계                           |
+| 주문 | order | 집이나 직장 등 고객이 선택한 주소로 음식을 배달한다.                 |
 | 주문 상태 | order status | 주문이 생성되면 매장에서 주문을 접수하고 고객이 음식을 받기까지의 단계를 표시한다. |
-| 주문 항목 | order line item | 주문에 속하는 수량이 있는 메뉴 |
+| 주문 항목 | order line item | 주문에 속하는 수량이 있는 메뉴                              |


지속적인 용어사전 업데이트 👍 👍
모델링도 구현하신 것에 맞게 업데이트도 해보면 어떨까요?

wlroh

환우님 안녕하세요 :)
리뷰가 너무 늦어져서 죄송합니다 🙏
개인적인 일들이 많아지면서 알림을 놓쳤습니다
피드백 잘 반영해주셨고 코멘트 추가한 부분이 있는데요 확인부탁드려요!
즐거운 주말되세요 😄

wlroh · 2023-11-04T08:02:08Z

src/main/java/kitchenpos/deliveryorders/domain/DeliveryOrder.java

+    protected DeliveryOrder() {
+    }
+
+    public DeliveryOrder(UUID id, OrderStatus status, LocalDateTime orderDateTime, DeliveryOrderLineItems orderLineItems, DeliveryOrderAddress deliveryAddress) {


정적 팩토리메서드를 사용하게 되면 외부에서 생성자를 통해 생성하지 못하도록 수정해보는 것은 어떤가요?
배달 주문은 생성하기 위한 로직들이 정적메서드에 구현되어 있어, 정책에 위배되는 배달주문 생성하는 위험이 존재할 것 같아요

이 부분에 대해 고민을 했던 부분이
처음 주문을 생성하면 WAITING이라는 조건 때문에 정적 팩토리메소드로 생성 시 무조건 WAITING 상태로 DeliveryOrder를 생성 하는데요,
이렇게 되면 테스트 코드 작성 시 다른 상태값을 생성하지 못해 public으로 두었습니다.
정적 팩토리 메소드를 통해서 생성시에도 상태를 받도록 변경하는 하고 테스트 코드도 of를 통해 생성하는 쪽이 좋을까요?

저도 많은 고민을 했었는데요~ 테스트코드를 작성하기 위해 프로덕션 코드에 손이 가는 것이 괜찮을까? 고민이 들기는 했어요.

정적 팩토리 메소드를 통해서 생성시에도 상태를 받도록 변경하는 하고 테스트 코드도 of를 통해 생성하는 쪽이 좋을까요?

말씀주신 방법으로 구현해도 좋아요. 다만, 프로덕션 코드에서는 배달주문을 생성할 때 Status 값이 무조건 WAITING 이라는 점때문에 찜찜하실수 있을 것 같아요
이런경우면 of 정적팩토리메서드 대신 ReflectionTestUtils 을 이용하는 방법도 있고, Fixture 를 이용하는 방법도 있을 것 같아요
위의 2가지 방법도 같이 찾아보시면서 고려해보시면 좋을 것 같아요 😄

wlroh · 2023-11-04T08:08:42Z

src/main/java/kitchenpos/eatinorders/tobe/domain/order/EatInOrder.java

+    @ManyToOne
+    @JoinColumn(
+            name = "order_table_id",
+            columnDefinition = "binary(16)",
+            foreignKey = @ForeignKey(name = "fk_orders_to_order_table")
+    )
+    private OrderTable orderTable;


저는 환우님이 매장주문 컨텍스트에서 루트 애그리거트를 매장주문과 주문테이블 2개로 구성해주신것으로 저는 생각했어요~
저는 직접참조와 간접참조에 대해 루트애그리거트 내에서는 직접참조를 하고 루트 애그리거트 간은 간접참조하는 것이 좋다고 생각하고 있어요
루트 애그리거트의 핵심역할 중 하나가 애그리거트의 일관성은 깨지지 않도록 하는 것이기 때문에 타 애그리거트를 참조하게 되면 타 애그리거트를 수정하는 이슈가 발생할 수 있지 않을까해요.

wlroh · 2023-11-04T08:25:11Z

src/main/java/kitchenpos/eatinorders/application/EatInOrderService.java

+    @Transactional
+    public EatInOrderDto complete(UUID orderId) {
+        EatInOrder eatInOrder = eatInOrderRepository.findById(orderId)
+                .orElseThrow(NoSuchElementException::new);
+        eatInOrder.complete();
+
+        OrderTable orderTable = orderTableRepository.findById(eatInOrder.getOrderTable().getId())
+                .orElseThrow(NoSuchElementException::new);
+        if (!eatInOrderRepository.existsByOrderTableAndStatusNot(orderTable, OrderStatus.COMPLETED)) {
+            orderTable.clear();
+        }
+        return ConvertUtil.convert(eatInOrder, EatInOrderDto.class);
+    }


말씀드리고 싶었던 것은 현재 로직 중에 도메인로직으로 보이는 부분이 있어서 도메인 계층으로 위임하면 어떨까 의견을 드려본 것이였어요 😄
주문완료하는 로직에서 주문테이블과 매장주문 애그리거트간의 협력도 필요하다고 봐요.
그렇기 때문에 도메인 계층으로 위임할 때 여러 고민도 해보시면 좋을 것 같아서요

OrderTable의 추가 Order는 OrderTable에 계속 쌓이며 모든 EatInOrder가 완료되면 EmptyTable이 된다.

Completed 가 되려면 전 주문은 Served 상태여야 한다.

모델링 해주신 것을 봤을 때는 지금처럼 구현한 것도 좋지만, 응용계층에서 도메인에 대한 제어가 더 많아질 것 같아요

wlroh · 2023-11-04T08:28:54Z

README.md

+- `EatInOrder`는 식별자와 `OrderStatus`, 주문 시간, `OrderLineItems`를 가진다.
+- `Menu`가 `Display Menu`되고 있으며 판매되는 메뉴 가격과 일치하면 `EatInOrder`가 생성된다.
+- `EatInOrder`는 접수 대기 ➜ 접수 ➜ 서빙 ➜ 계산 완료 순서로 진행된다.
+- `EatInOrder`가 생성되면 `OrderStatus`는 `Waiting` 상태이다.
+- `Accepted` 가 되려면 전 주문은 `Waiting` 상태여야 한다.
+- `Served` 가 되려면 전 주문은 `Accepted` 상태여야 한다.
+- `Completed` 가 되려면 전 주문은 `Served` 상태여야 한다.


모델링 업데이트 👍 👍
전략적 설계를 하면서 구성하신 모델링과 실제 구현한 코드간의 싱크를 맞춰주시는 것에 대해서도 꾸준히 유지보수가 필요하다고 생각해요
모델링과 구현코드간의 싱크가 맞지 않으면, 멘탈모델이 일치하지 않아 결국 DDD 도입 전과 같은 문제들이 계속 발생하게 되는 이슈가 있어요 😢

wlroh · 2023-11-04T08:38:32Z

src/main/java/kitchenpos/deliveryorders/domain/DeliveryOrder.java


-    @Column(name = "type", nullable = false)
-    @Enumerated(EnumType.STRING)
-    private OrderType type;
-


피드백 잘 반영해주셨어요 👍 👍
배달주문뿐만 아니라 다른 주문도 같이 반영해보는 것은 어떤가요?

wlroh · 2023-11-04T08:39:51Z

src/main/java/kitchenpos/eatinorders/tobe/domain/order/EatInOrder.java

-        if (status != OrderStatus.ACCEPTED) {
+        if (status != OrderStatus.SERVED) {


테스트 코드가 있으면 지금과 같은 상황이 없지 않을까 생각이 들어요 😄
도메인에 대한 테스트 코드가 많이 없는 것 같더라구요 😢
테스트 코드도 작성해보는 것은 어떤가요?

hwanwooyu and others added 4 commits September 25, 2023 22:59

feature(order): step2 리뷰 반영(menuProductPrice 영속화)

aa31b61

feature(order): 배달 주문 작성

ce277ce

feature(order): 배달 주문 작성

267e943

feature(order): 매장 주문 작성

a888d59

wlroh requested changes Oct 1, 2023

View reviewed changes

hwanwooyu added 3 commits October 3, 2023 21:36

feature(refactoring): order Type 제거

f4da63b

feature(refactoring): order 완료시 상태 조건 변경

33e3f30

feature(refactoring): 모델링 업데이트

b21163d

wlroh requested changes Nov 4, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

step3 (매장 식사 주문) #246

step3 (매장 식사 주문) #246

yuhwanwoo commented Sep 29, 2023

wlroh commented Oct 1, 2023

wlroh left a comment

wlroh Oct 1, 2023

wlroh Oct 1, 2023

wlroh Oct 1, 2023

yuhwanwoo Oct 3, 2023

wlroh Nov 4, 2023

yuhwanwoo Nov 12, 2023

wlroh Nov 14, 2023

yuhwanwoo Nov 16, 2023

yuhwanwoo Nov 16, 2023

wlroh Oct 1, 2023

yuhwanwoo Oct 3, 2023

wlroh Nov 4, 2023

yuhwanwoo Nov 12, 2023

wlroh Nov 14, 2023

yuhwanwoo Nov 16, 2023

wlroh Oct 1, 2023

wlroh Oct 1, 2023

wlroh left a comment

wlroh Nov 4, 2023

yuhwanwoo Nov 12, 2023

wlroh Nov 14, 2023

wlroh Nov 4, 2023

wlroh Nov 4, 2023

wlroh Nov 4, 2023

wlroh Nov 4, 2023

wlroh Nov 4, 2023

		Optional<Menu> findById(UUID id);
		List<Menu> findAllByIdIn(List<UUID> ids);

		@@ -0,0 +1,71 @@
		package kitchenpos.eatinorders.tobe.domain.ordertable;

		if (status != OrderStatus.ACCEPTED) {
		if (status != OrderStatus.SERVED) {

step3 (매장 식사 주문) #246

Are you sure you want to change the base?

step3 (매장 식사 주문) #246

Conversation

yuhwanwoo commented Sep 29, 2023

wlroh commented Oct 1, 2023

wlroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wlroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment