3단계 - 리팩터링(매장 식사 주문) #321
3단계 - 리팩터링(매장 식사 주문) #321
Conversation
- ProductName의 규약 검증을 위해서 ProfanityValidator를 생성시 추가 - DIP를 위해서 Interface를 domain에 추가
- 규약에 적힌 규칙들에 대해 테스트
…리고 menu 와 menu group으로 aggregate을 나눴습니다. 1. AGGREGATE - MENU a. menu product b. menu name c. menu price 2. AGGREGATE - MENU GROUP a. menu group b. menu group name 마지막으로, 서로 간접 참조로 관게를 생성하였습니다. - 여기서 중요한 점은, menu product가 product에 간접 참조하게 되고 price 값이 수동으로 update되어야하는 점입니다. (이벤트를 받거나, 수동으로 table update 필요)
# Conflicts: # src/test/java/kitchenpos/menus/application/MenuServiceTest.java
1. 메뉴 가격 동일 검증 2. 숨겨진 메뉴 검증
…유지하였습니다. 1. 변경 + 조회 사이클이 식당내 주문과 동일함
…유지하였습니다. 1. 변경 + 조회 사이클이 식당내 주문과 동일함
…유지하였습니다. 1. 변경 + 조회 사이클이 식당내 주문과 동일함
…유지하였습니다. 1. 변경 + 조회 사이클이 식당내 주문과 동일함
- ORDERFACTORY로 생성 로직을 생성하였습니다. -
- ORDERFACTORY로 생성 로직을 생성하였습니다. - DomainService로 외부시스템 연동이나 여러개의 aggregate을 들고와야할때 사용하려고 합니다.
- ORDERFACTORY로 생성 로직을 생성하였습니다. - DomainService로 외부시스템 연동이나 여러개의 Root Aggregate을 들고와야할때 사용하려고 합니다.
…rService 생성해서 외부연동과 생성로직을 다른 클래스로 분리하였습니다.
…keoutOrder로 세분화하였습니다. 하지만 공통의 패키지를 공유하게끔 만들었습니다.
1. 생성로직 2. 도메인 상태 검증 로직 3. 상태 변화 로직 4. 외부 연동 로직(배달 요청)
혹시 어떤 질문이실까요? 🤔
1 : N 관계에서 N 이 많아지면 어떤점이 걱정되어서 고민이신 것일까요? 🤔
타입이 많아지면 디자인패턴 중에 팩토리 패턴을 활용하기도 해요. 다만, 위의 미션의 경우 타입이 많아질 때 꼭 타입으로 구분하는 것이 맞을지 객체를 통해 구분하는 것이 맞을지 고민해보시면 좋을 것 같아요.
공통로직을 공유커널로 두고 활용하시려는 의도실 것 같은데 공통로직이 맞나 우선 확인해볼 것 같아요!
내부이벤트와 외부이벤트로 나눌수 있죠! 구분하는 방법에 대해 말씀하시는 것일까요? 이부분은 팀 컨벤션에 따라 결정될 것 같아서 정답은 없는 것 같아요 😄 제가 잘못 이해한 것이면 다시 말씀주세요~
아! 이부분은 menu 에게 더 많은 로직을 위임해도 괜찮다고 생각되어서 말씀드린거였어요!
어떤 테스트코드를 작성하시려는지에 따라 달라질 것 같아요! 추가로 시창님께 여쭤보고 싶은 것이 있는데 테스트 커버리지는 높으면 높을수록 좋다고 생각하실까요? 시창님 안녕하세요! 미션 고생하셨어요! 모델링과도 싱크가 안맞는 것 같아서 우선, 매장주문를 집중적으로 설명해주실수있을까요? 혹은 매장주문에서 애그리거트를 어떻게 생각하고 계실가요? 핑퐁이 길어질 것 같으면 DM 이나 게더에서 이야기해도 좋아요 |
| @ManyToOne | ||
| @JoinColumn( | ||
| name = "order_table_id", | ||
| columnDefinition = "binary(16)", | ||
| foreignKey = @ForeignKey(name = "fk_orders_to_order_table") | ||
| ) | ||
| private OrderTable orderTable; |
There was a problem hiding this comment.
주문테이블을 직접참조하신 의도가 있을까요?
추가로, Order 속성으로 구성해주신 의도가 있을까요?
There was a problem hiding this comment.
네, 주문 테이블의 상태 변화는 주문에 생성, 삭제를 제외하고 주문 상태 변화에 의존성이 걸려져있습니다.
트랜잭션 설계시에도 같이 조회되고, 같이 수정되는 것으로 보입니다.
그렇다면, 굳이 간접 참조로 뺄 이유가 없어보여서 직접 참조로 남겼습니다.
주문 테이블의 상태변화가 아닐때에도, 같이 조회되는 점도 LAZY LOAD 하면 되지 않을까 생각했습니다.
나중에 필요할때, 간접참조로 빼도 상관 없어 보였습니다.
상태 변화 케이스들
- 주문 테이블 생성
- 주문 테이블 삭제
--- 주문 테이블 상태 변경 - 주문 테이블 점유
- 주문 테이블 점유 해제
| @@ -0,0 +1,79 @@ | |||
| package kitchenpos.eatinorders.domain.eatinorder; | |||
There was a problem hiding this comment.
매장주문뿐만 아니라 배달주문, 포장주문도 작업해주신 것으로 보여지네요!
eatinorder 패키지에 구성해주신 의도가 있을까요?
There was a problem hiding this comment.
-
패키지 변경시 코드리뷰시에 불편함때문에 변경하지 않았습니다. -
만약 변경했다면 다음과 같이 변경했을 것으로 보입니다.
Order 상위 패키지
|__ EatInOrder
|__ DeliveryOrder
|__ TakeoutOrder
- 동일 로직을 모두 가지고 있으며 event를 통해서 각각 유니크 로직들을 처리할 수 있다고 생각했습니다.
- 하지만 실제 구현은 힘들었는데
OrderService내에서
- 공통 로직 구현시에는
배달 주문만 실행하는 delivering에 대한 로직을 어떻게 수정할지 고민했습니다. - Null Object 패턴으로 만약 실행된다면 아무것도 실행되지 않는 개념으로 구현했습니다.
| public class DefaultOrderFactoryTest { | ||
| } |
There was a problem hiding this comment.
테스트코드가 부족한 것 같아요 😢
테스트코드도 모두 작성해보시는 것은 어떤가요?
There was a problem hiding this comment.
이 부분은 먼저 코드리뷰받고 진행하려고 했습니다!
- 테스트 작성하겠습니다!
| public void clear(final Order order) { | ||
| if (order.isNotEatIn()) { | ||
| return; | ||
| } | ||
| if (orderTableRepository.findById(order.getOrderTableId()).isEmpty()) { | ||
| throw new IllegalArgumentException("주문 테이블이 존재 하지 않습니다."); | ||
| } | ||
|
|
||
| if (orderRepository.existsByOrderTableAndStatusNot(order.getOrderTable(), OrderStatus.COMPLETED)) { | ||
| throw new IllegalStateException("주문이 완료되지 않은 테이블이 있을때, 주문 테이블을 사용안함으로 변경할 수 없습니다."); | ||
| } | ||
|
|
||
| order.clearOrderTable(); | ||
| } |
There was a problem hiding this comment.
주문이 완료되지 않는 테이블의 경우 예외를 던지고 있는데 혹시 그런 의도로 구성해주신 것일까요?
매장 테이블 정리에 대한 정책으로 보여지고, 매장주문이 완료될 때마다 호출되는 것 같은데 만약 해당테이블에 여러 매장주문이 등록되어 있을때 완료처리가 제대로 안될 것 같아요 😢
| @@ -1,4 +1,6 @@ | |||
| package kitchenpos.eatinorders.domain; | |||
| package kitchenpos.eatinorders.domain.eatinorder; | |||
There was a problem hiding this comment.
현재 주문테이블은 루트애그리거트로 생각중이실까요?
모델링과 현재 구현해주신 코드랑 싱크가 안맞아서 확인부탁드릴게요
There was a problem hiding this comment.
주문테이블을 AGGREGATE에 속한 ENTITY로 생각을 하고 있습니다!
다음과 같은 모델링을 어떻게 변경해야 명시적으로 ORDER AGGREGATE에 속한 ENTITY로 표현할 수 있을까요?
주문 테이블
| 한글명 | 영문명 | 설명 |
|---|---|---|
| 주문 테이블 | Order Table | 식당 내에서 주문에 대한 메뉴가 대접받는 장소 |
| 고객 인원 | Customer Headcount | 주문 테이블에서 대접받고 있는 손님의 수 |
| 주문 테이블 상태 | Order Table Status | 주문 테이블이 주문을 받을 수 있는지에 대한 정보 |
| @Target(ElementType.TYPE) | ||
| @Inherited | ||
| @Documented | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Component | ||
| public @interface DomainService { |
| if(Objects.isNull(orderTable)){ | ||
| return Optional.ofNullable(null); | ||
| } | ||
|
|
||
| return Optional.of(orderTable); |
There was a problem hiding this comment.
return Optional.ofNullable(orderTable);
으로 커버가 가능할 것 같은데 어떠신가요?
| public Order startDelivery(final UUID orderId) { | ||
| final Order orderRequest = orderRepository.findById(orderId) | ||
| .orElseThrow(NoSuchElementException::new); | ||
| if (order.getType() != OrderType.DELIVERY) { | ||
| throw new IllegalStateException(); | ||
| } | ||
| if (order.getStatus() != OrderStatus.SERVED) { | ||
| throw new IllegalStateException(); | ||
| } | ||
| order.setStatus(OrderStatus.DELIVERING); | ||
| return order; | ||
| } | ||
|
|
||
| @Transactional | ||
| public Order completeDelivery(final UUID orderId) { | ||
| final Order order = orderRepository.findById(orderId) | ||
| orderRequest.delivering(); | ||
| return orderRequest; | ||
| } |
There was a problem hiding this comment.
제가 배달컨텍스트도 해당 패키지안에 구현하였습니다!
| public OrderTable clear(final UUID orderTableId) { | ||
| final OrderTable orderTable = orderTableRepository.findById(orderTableId) | ||
| .orElseThrow(NoSuchElementException::new); | ||
| if (orderRepository.existsByOrderTableAndStatusNot(orderTable, OrderStatus.COMPLETED)) { | ||
| throw new IllegalStateException(); | ||
| } | ||
| orderTable.setNumberOfGuests(0); | ||
| orderTable.setOccupied(false); | ||
| return orderTable; |
There was a problem hiding this comment.
만약에, OrderTable의 상태변화가 Order에 의존성이 있다면,
Order -> OrderTable.clear 로만 변경될 수 있게 변경하려고 했습니다.
| - 행동 | ||
| 1. `주문 종류`: 공통 (해당 조건은 공통으로 지켜져야 한다.) | ||
| - `주문`을 생성할 때, `대기중(WAITING)` 으로 `주문 상태`를 변경한다. | ||
| - `메뉴` 내의 `주문상품`들이 삭제된 `상품`이면 안된다. | ||
| - `메뉴` 내의 `주문 메뉴`들이 삭제된 `메뉴`이면 안된다. | ||
| - `메뉴`가 생성된 상태이고 `공개 상태(VISIBLE)` 되어있어야한다. | ||
| - `메뉴 가격` 총액과 `주문`에 속한 `상품 가격` 총액이 동일해야한다. | ||
| - `주문 가격` 총액과 `주문`에 속한 `메뉴 가격` 총액이 동일해야한다. | ||
| - `주문`은 `메뉴` 1개 이상으로 구성되어야 한다. | ||
| 2. `주문 종류`: `포장주문` | ||
| - 주문을 생성할 때, 주문상품들의 갯수를 감소시킬 수 없다. |
There was a problem hiding this comment.
꾸준한 모델링 업데이트 👍 👍
다만, 모델링에 그림이 혹시 어떤 것을 뜻하는 것일까요? 🤔
There was a problem hiding this comment.
그림은 원래 있었는데 추가해두지 않았습니다. 해당부분도 삭제하겠습니다!
|
노션으로 질문 답변에대해서 정리해둬서 슬랙으로 메시지 드렸습니다!! 따로 게더에서 이야기해보고 정리해서 해당 PR에 올리도록 하겠습니다! |
안녕하세요! 웅래님, 이번에는 마음을 편하게먹고 즐기면서 진행을 했습니다.
도메인 주도 개발 시작하기를 읽으면서 하고 있어서 조금 늦었네요!이상한 부분들이 많을텐데 언제든지 말씀주세요!
A. 연관관계 설정 및 VO 및 도메인 로직 구현
ORDER (바운디드 컨텍스트는 넓게 잡기가 선호되서 하나의 패키지로 구현하였습니다.)
ORDER ITEMS
ORDERTABLE
ORDER SERVICE
B. @Domain SERVICE 와 @Domain FACTORY에 대한 에노테이션 생성으로 명시적 표기
질문들
책에 따라 DOMAIN SERVICE를 METHOD 호출시 전달하게끔 변경하였습니다. '외부 시스템 연동이 필요한 도메인 로직일때 혹은 계산로직일때'
1-N(주문, 주문항목들) 의 경우 N이 많아질 때가 있는데 해당부분은 어떻게 구현하실지 궁금합니다.
주문 타입의 경우가 많아지면,
어떤 타입 -> 팩토리에 대한 맵핑을 지원하는 디자인 패턴을 사용하시나요?지금 상태에서 실무에 돌입하게 되면 ORDER 와 EATIN ORDER의 관계에서 상속이 객체지향적이지 않아 보여요! (EJ 합성 > 상속)
마지막으로, 공유커널(COMMON)에 있는 이벤트들은 내부 이벤트(내부 서비스) VS 외부 이벤트(외부 서비스를 위함) 들이 있을것 같아요
저번에 말씀하셨던 application에서 event를 발행하는 것(productService)를 domain 으로 넣으라고 가이드를 주셨던것이죠??
🚀 2단계 - 리팩터링(메뉴) #304 (comment)
마지막으로, @async 와 @TransactionalEventListener(AfterCommit) 옵션에 대해서는 혹시 테스트를 진행해보신적 있다면 노하우가 있으실까요??
테스트하기가 너무 까다롭네요!