[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610

greeng00se · 2023-10-24T01:08:13Z

코드의 변경사항이 조금 많습니다. ㅠㅠ
최대한 서비스 로직을 깔끔하게 만드려고 노력한 것 같습니다.

외래키를 적절히 끊어야할까에 대한 고민이 많이 듭니다.
테스트 로직을 보시면 다른 repository에 대한 의존이 너무 많아지기 때문입니다.

저번에 남겨주신 ID 생성자에 대한 부분은 3단계때 반영하려고 했는데, 리팩터링 도중 다음과 같은 생각이 들었습니다.
테오는 어떻게 생각하시나요?

ID를 받는 생성자를 제거하는 과정에서 생긴 고민

1. 모든 필드를 지정해줄 수 있는 주생성자를 만드는 건 구현 기술에 침투적인 생성자가 생기는 것인가?

id 생성자를 제거한다면, 결국 도메인을 식별하기 위한 값을 넣으려면 jpa와 같은 기술에 의존적인 생성을 해야하는데, 오히려 제거하는 것이 구현 기술에 침투적인 생성자를 만드는 것이 아닌가?
결국 해당 도메인을 식별하기 위한 값으로 본다면, jpa를 사용하지 않았을 때 값을 지정해줘야 할텐데, 해당 부분을 위해 남겨둬야하지 않을까?

3단계 미션도 잘 부탁드립니다!

woosung1223

안녕하세요 허브 반갑습니다

저는 허브가 리뷰이라서 경험치 두 배 이벤트 하고 있는 거 같습니다.

이번에는 도메인 이벤트랑 도메인 서비스를 적절히 사용해주셔서 의존을 깔끔하게 끊어주셨네요! 역시 기대를 저버리지 않는군요

그런데 저는 도메인 이벤트, 도메인 서비스는 사용해 본 경험이 없어서 궁금한 점이 많아 의도치 않게 코멘트를 많이 드리게 되었네요 😅

답변 기다리고 있겠습니다.. 고생하셨습니다!

woosung1223 · 2023-10-24T14:32:25Z

README.md

+1. 모든 필드를 지정해줄 수 있는 주생성자를 만드는 건 구현 기술에 침투적인 생성자가 생기는 것인가?
+
+id 생성자를 제거한다면, 결국 도메인을 식별하기 위한 값을 넣으려면 jpa와 같은 기술에 의존적인 생성을 해야하는데, 오히려 제거하는 것이 구현 기술에 침투적인 생성자를 만드는 것이 아닌가?
+결국 해당 도메인을 식별하기 위한 값으로 본다면, jpa를 사용하지 않았을 때 값을 지정해줘야 할텐데, 해당 부분을 위해 남겨둬야하지 않을까?


허브의 고민을 쉽게 볼 수 있어서 좋네요 💯

확실히 말씀해주신 부분도 납득이 되는군요!
ID를 포함하지 않는 생성자를 지정해주는 경우, 오히려 JPA 구현기술에 의존적으로 ID를 생성할 수 밖에 없다. ID를 도메인을 식별하기 위한 하나의 값으로 보는게 타당하다라고 이해를 했어요.

그런데 궁금한 점이 생겼습니다!

식별자를 생성자에 드러내지 않아서 생기는 문제는 어떤게 있을까요?
반대로 이야기하면, 식별자를 직접 지정해줘야 하는 경우는 언제가 있나요?

저는 아직 도메인 객체에 식별자를 지정해줘야 하는 케이스가 잘 떠오르지 않는 것 같아서요 🤔

생성자에 드러내지 않아서 생기는 문제는 크게 없을 것 같긴한데, 사용하는 입장에서 어떤 식별자를 사용하는지 알기 힘들 것 같다는 생각이 들어요. 만약 식별자로 UUID를 사용한다면 애플리케이션에서 직접 지정해줘서 생성할 수 있을 것 같아요! #610 (comment) 로 이야기 이어나가보면 좋을 것 같아요!

woosung1223 · 2023-10-24T14:35:23Z

src/main/java/kitchenpos/domain/menu/MenuProduct.java

+        this.productId = productId;
+        this.name = name;
+        this.price = price;
+        this.quantity = quantity;


체이닝으로 처리할 수 있을 것 같아요 😄

woosung1223 · 2023-10-24T14:40:31Z

src/main/java/kitchenpos/domain/menu/Menu.java

-            throw new IllegalArgumentException("메뉴의 가격은 메뉴 상품들의 금액의 합보다 클 수 없습니다.");
-        }
+    public void register(MenuValidator menuValidator) {
+        menuValidator.validate(this);


오 도메인 서비스를 활용해주셨네요 👏

서로 다른 도메인 그룹끼리의 협력을 도메인 서비스로 몰아주신 것 처럼 보이는데,
이렇게 작성하면 어떤 이점이 있나요?

MenuService에서 menuValidator의 책임을 수행하는 것과 대비해서요!

생성시 일관성을 보장할 때 validator를 이용해서 검증하니 복잡한 로직을 한눈에 볼 수 있다는 장점이 있는 것 같아요! 👍 👍

src/main/java/kitchenpos/application/menu/MenuService.java

woosung1223 · 2023-10-24T14:49:37Z

src/main/java/kitchenpos/application/menu/MenuMapper.java

+import org.springframework.stereotype.Component;
+
+@Component
+public class MenuMapper {


Mapper를 구현해주셨네요!

Mapper가 생긴김에 제안드리는 것인데, Mapper들을 표현 계층으로 올리고 Service에서는 도메인 객체를 받게 할 순 없을까요?

Mapper를 구현해주셨으니 간단하게 레이어 간의 의존성도 단방향으로 흐르게 할 수 있지 않을까 싶어 말씀드립니다!

그런데 추가로 Response를 매핑하는 작업도 필요하긴 하겠네요! 작업이 커질 것 같다면 의견만 남겨주셔도 좋습니다~

저도 테오 의견을 너무 반영하고 싶지만 작업이 너무 커질 것 같아요 살려주세요.. 😢

현재 MenuMapper에 대해 코멘트 남겨주셨지만 아래의 질문은 OrderMapper 기준으로 봐주시면 좋을 것 같아요!

이 부분에 대해서 테오의 생각을 듣고싶어요!
테오는 controller용 request를 받고 service는 도메인 객체를 받도록 구현하시는 편인가요?
만약 해당 방식으로 구현한다면, 현재 구조에서(Order가 Menu의 정보를 복사해야하는 경우) 어떤 방식으로 Order를 생성하여 서비스로 전달하는 것이 좋을까요?

그리고 코드를 작성하면서도 Mapper가 과연 Repository를 의존해도 되나 라는 생각이 들었어요.
테오라면 어떻게 작성하실껀가요?

해당 부분을 Order와 함께 저장하지 않는다면 Menu의 정보를 저장하는 좋은 방법은 어떤게 있을까요?
(저는 시간이 좀 널널했다면 Mapper가 Repository 의존하지 않고 <- 이 부분에서 어색함을 많이 느꼈고 이게 맞나 �싶습니다., ID를 UUID로 전부 변경하고, 생성시 이벤트를 발행해서, 해당 이벤트에 있는 id값 가지고 Menu를 복제해서 저장하는 방법도 좋을 것 같다는 생각이 들어요. 현재 구조랑 비교해서 봐주시면 정말 좋을 것 같습니다!)

👍👍

저는 서비스가 도메인 객체를 받을 필요는 없다고 생각합니다(서비스 재사용성이 필요한 상황이라면 이야기가 다르겠지만요!)

저도 일단은 DTO 를 서비스에서 받는 걸 선호하는데요!

만약 응용 계층과 표현 계층의 양방향 의존을 제거하고자 한다면, Mapper는 표현 계층에 위치하는게 맞겠죠!

그런데 이 경우 Repository에 의존하는 Mapper는 어떻게 처리할 것인가에 대한 문제가 생기게 되는데, 허브가 말씀해주신 대로 ID 값을 가지고 복제해서 저장하는 방법을 사용할 수도 있을 것 같아요.

Mapper가 Repository를 의존하는 건에 대해서는 저도 확신이 크게 서지는 않네요. 도메인 객체를 조립하기 위해서는 Repository가 필요한게 맞는데, 저라면 Mapper에서 쿼리가 나갈 것으로는 전혀 예측하지 못할 것 같아서요 🥲

근거만 있다면 Mapper에서 Repository를 끌고와도 다 괜찮다고 생각합니다!

좋은 답변 감사합니다 👍

woosung1223 · 2023-10-24T14:55:52Z

src/main/java/kitchenpos/domain/order/Order.java

        this.id = id;
-        this.orderTable = orderTable;
+        this.orderTableId = orderTableId;
        this.orderStatus = orderStatus;
        this.orderedTime = orderedTime;
        this.orderLineItems = new OrderLineItems(orderLineItems);


아까 연어와 잠깐 이야기 나누었던 주제인데,

이렇게 도메인 서비스를 이용해 검증하는 경우 생성자를 통해 단순 생성을 했을 때 비즈니스 규칙을 위반하는 것에 대해서는 어떻게 생각하시나요?

place 메소드를 호출하지 않으면 Order 객체는 비즈니스적으로 의미를 갖지 못할 것 같아요. 그러면 두 로직은 결합되는게 맞지 않을까요? (정적 팩토리 메소드를 사용한다든가..)

현재 Mapper를 사용하고 있어서, Mapper를 사용하여 Order를 만들어주는 부분, 그리고 검증하는 부분을 나눠서 사용하는게 가독성이 더 좋아보인다는 생각이 듭니다.

테오가 언급해주신대로 생성 했을 때 즉시 일관성을 보장하도록 작성한다면 정적 팩터리 메서드에 validator를 같이 넘겨주는 방법도 좋을 것 같습니다! 그렇게 된다면 Mapper가 Validator를 받는 방향으로 작성하면 좋을 것 같아요.

현재 OrderService.create 메서드에서만 해당 도메인을 생성을 해서, 두 방법 모두 사용에는 크게 문제가 된다고 생각하지 않아요!
테오라면 Mapper가 Validator를 받아 조금 더 일관성을 보장하도록 하는 것과, 분리하는 것 둘 중 어떤게 더 좋아보이시나요?

오 좋은 의견 감사합니다 ㅎㅎ

저라면 Mapper가 Validator를 받게 한 다음 정적 팩토리 메소드를 사용할 것 같습니다!

저는 개인적으로 비즈니스 규칙을 담은 객체라면 생성 시점에 무결하다는 보장을 받는 편이 좋지 않을까 싶어요.

도메인 서비스를 응용 서비스에서 주입해주는 게 깔끔하긴 하지만, 그에 따른 리스크가 꽤 큰 것 같다는 생각이 들어서요!

객체를 사용하는 입장에서는 매번 Validator를 호출해야 하는데, 사용하는 쪽에서 설정을 매번 해줘야 한다는게 설정과 사용의 분리가 적절하게 이루어졌나? 라는 고민이 들더라고요..

그런데 분리하는 방법도 응용 서비스에서 흐름을 바로 확인할 수 있어서 좋은 것 같긴 하네요! 정팩메를 사용하는 경우에는 Mapper에 검증 설정 로직이 감춰질 것 같긴 합니다 🤔

생성할 때 Validator를 받게 한다면 조금 더 안정감이 들 것 같다는 생각이 드네요.
어떤게 좋을지 지속적으로 고민을 해봐야겠습니다 크크..

woosung1223 · 2023-10-24T15:06:04Z

src/main/java/kitchenpos/domain/tablegroup/TableGroup.java


    @Id
    @GeneratedValue(strategy = IDENTITY)
    private Long id;
    private LocalDateTime createdDate;

-    @OneToMany(mappedBy = "tableGroup", cascade = CascadeType.ALL)
+    @OneToMany
+    @JoinColumn(name = "table_group_id")
    private List<OrderTable> orderTables = new ArrayList<>();


루트끼리 직접 참조를 수행해주셨네요!

혹시 어떤 이유에서 ID를 통한 간접 참조가 아닌 직접 참조로 해주셨는지 알 수 있을까요?

여기도 group, ungroup 메소드가 호출될 때 도메인 서비스를 주입하면 직접 참조를 할 필요는 없을 것 같아서요!

테오 코멘트 보고 해당 부분 TableGroupClient라는 도메인 서비스를 사용해서 group, ungroup을 수행하도록 변경해봤는데, 이전의 저장 API가 tableGroup을 같이 반환하다보니 어색함이 많이 느껴지더라고요!
조회하는 부분을 분리하도록 일관성 있게 작성하지 않는다면 현재 코드 기준에서 어색함이 느껴지는데 이 부분에 대해서 어떻게 생각하시나요?

역시 다 계획이 있으셨군요 👍

저도 이 부분 DTO 때문에 패키지 의존성이 끊어지지 않아서 고민이었는데요!

저는 그냥 응용 서비스에서 Repository를 의존해서 DTO를 조합해줬습니다.

이 상황이 결국은 뷰가 도메인 객체의 협력관계까지 영향을 미친 상황이라는 생각이 들거든요.

API와 비즈니스의 괴리는 있을 수 밖에 없는 것 같은데, 이를 어떻게 해결할지에 대한 해답은 찾기 어려운 것 같습니다.

제가 생각하는 방법으로는 2계층 서비스를 도입해서 DTO의 조합을 담당하는 서비스를 하나 만드는 것인데(그러면 도메인이 뷰에 의존하는 부분은 완전 제거가 되겠죠) 이 방법 역시도 트레이드 오프가 너무나도 크기 때문에 확신은 서지 않네요..🥲🥲

계획은 없었고.. 테오 코멘트 보고 진행 해보았습니다!
저도 결국 이러한 부분이 뷰가 도메인 객체의 협력 관계에 영향을 미쳤다고 생각합니다.
이 부분에 대해서 다시 한 번 생각해봤는데 테오가 제안해주신대로 id로 참조하고, repository로 tableGroup을 조회해서 반환하는게 깔끔할 것 같아요 👍

woosung1223 · 2023-10-24T15:11:56Z

src/main/resources/db/migration/V4__add_menu_product_field.sql

+ALTER TABLE menu_product
+    ADD COLUMN price DECIMAL(19, 2) DEFAULT NULL;
+
+ALTER TABLE menu_product
+    ADD COLUMN name VARCHAR(255) DEFAULT NULL;


메뉴의 이름과 가격이 변경되면 주문 항목도 함께 변경된다. 메뉴 정보가 변경되더라도 주문 항목이 변경되지 않게 구현한다.

👍

해당 요구사항에 대해서 빠진 부분이 있어서 추가했습니다 😄

woosung1223 · 2023-10-24T15:14:34Z

src/test/java/kitchenpos/application/table/TableServiceTest.java


 @SuppressWarnings("NonAsciiCharacters")
+@RecordApplicationEvents


덕분에 이번에 야무지게 써먹었습니다

좋습니다!

woosung1223 · 2023-10-24T15:17:44Z

src/test/java/kitchenpos/application/order/OrderTableChangedEventHandlerTest.java

+    @EnumSource(value = OrderStatus.class, names = {"COOKING", "MEAL"})
+    @ParameterizedTest(name = "주문 상태가 {0}인 경우 예외를 던진다")
+    void 테이블의_상태_변경_시_해당_테이블을_사용하는_주문의_상태가_조리중이거나_식사중인_경우_예외를_던진다(OrderStatus orderStatus) {
+        // given
+        OrderTable orderTable = orderTableRepository.save(테이블(false));
+        OrderLineItem orderLineItem = 주문_항목(menu.getId(), 2L);
+        orderRepository.save(주문(orderTable, orderStatus, List.of(orderLineItem)));
+
+        // expect
+        assertThatThrownBy(() -> eventPublisher.publishEvent(new OrderTableChangedEvent(orderTable.getId())))
+                .isInstanceOf(IllegalArgumentException.class)
+                .hasMessage("테이블의 주문 상태가 조리중이거나 식사중인 경우 테이블의 상태를 변경할 수 없습니다.");
+    }


도메인 이벤트로 체크하지 않고 직접 이벤트를 발생시켜서 테스트를 작성해주신 이유가 있을까요?

order.changeEmpty를 호출하고 save 하는 시점에 발생하는 이벤트를 검증하는게 더 안정적인 테스트라고 생각이 드는 것 같아요(사실 AbstractAggregateRoot를 처음봐서 아닐 수도 있습니다..) 의견 남겨주세용

오 저도 코멘트 남겨주신 방법이 좋은 방법일 것 같다고 해서 변경해봤는데, 검증부에 orderTableRepository.save를 넣으니 내부적으로 IAE가 InvalidDataAccessApiUsageException로 감싸서 예외를 던지네요!
어떻게 하면 더 좋은 테스트를 작성할 수 있을까요?

흠.. 당연히 예외가 감싸지지 않을 줄 알았는데, 테스트 환경에서만 InvalidDataAccessApiUsageException으로 감싸서 예외가 반환되네요 🥲

이를 해결하기 위해선 아래와 같이 테스트를 작성할 수도 있을 것 같아요.

// expect orderTable.changeEmpty(false); assertThatThrownBy(() -> orderTableRepository.save(orderTable)) .getCause() .isInstanceOf(IllegalArgumentException.class) .hasMessage("테이블의 주문 상태가 조리중이거나 식사중인 경우 테이블의 상태를 변경할 수 없습니다.");

그런데 굳이 이렇게 하기보다는 허브처럼 직접 이벤트를 publish하는 방법이 나을 것 같아요. 해당 방식의 경우 getCause로 접근해야 한다는게 마음에 걸리네요..

현재 방식이 더 나은 것 같긴 하네요! 👍

woosung1223

안녕하세요 허브씨

코멘트 남겨주신 거 다 확인했습니다! 💯

정말 빈틈없는 논리로 무장하셔서 흠 잡을 데가 없네요.. 기계가 코드를 친 것 같습니다..

여러 고민들을 대신 해주셔서 저도 많이 배웠습니다 (굿)

다음 단계로 가주시죠! 고생하셨습니다 🚀

greeng00se added 19 commits October 17, 2023 03:28

docs: ID 생성자에 대한 생각 추가

59581d0

refactor: 테이블의 상태 변경시 테이블의 주문 상태를 확인하는 로직 이벤트로 분리

04e0190

refactor: 테이블의 그룹 해제시 테이블의 주문 상태를 확인하는 로직 이벤트로 분리

a4609be

refactor: 핸들러 테스트 통합 테스트로 변경

45a79de

refactor: 공통 패키지 분리

865695e

refactor: product 패키지 분리

69187e4

refactor: table 패키지 분리

865bb3a

refactor: tablegroup 패키지 분리

afe5d07

refactor: menugroup 패키지 분리

47a334d

refactor: menu 패키지 분리

becc941

refactor: order 패키지 분리

b0c5a13

refactor: OrderTable, TableGroup 양방향 관계 제거

bddd2ed

refactor: tableGroup 메서드명 변경

7a08a36

refactor: 단체 지정 해제 기능 추가

8b03397

refactor: OrderTable 도메인 이벤트 사용하도록 수정

6fa09f5

refactor: default 메서드 사용하도록 수정

ee1a248

refactor: OrderService default 메서드 사용하도록 수정

f0a84c5

refactor: 검증부 도메인 서비스로 분리

229178f

refactor: 메뉴 생성 로직 개선

f52aed9

woosung1223 self-requested a review October 24, 2023 14:27

woosung1223 requested changes Oct 24, 2023

View reviewed changes

greeng00se added 3 commits October 26, 2023 00:35

refactor: 주문 메뉴 저장 로직 수정

df3bca5

test: 정상적으로 작동하지 않는 주문 메뉴 테스트 수정

6b25729

refactor: style 수정

15a9df5

woosung1223 approved these changes Oct 26, 2023

View reviewed changes

woosung1223 merged commit 2ed6c34 into woowacourse:greeng00se Oct 26, 2023

greeng00se deleted the step03 branch October 26, 2023 14:07

greeng00se restored the step03 branch October 26, 2023 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610

[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610

greeng00se commented Oct 24, 2023

woosung1223 left a comment

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 24, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 26, 2023

greeng00se Oct 30, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 26, 2023

greeng00se Oct 30, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023 •

edited

Loading

woosung1223 Oct 26, 2023

greeng00se Oct 30, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 24, 2023

greeng00se Oct 25, 2023

woosung1223 Oct 26, 2023

woosung1223 left a comment


		@SuppressWarnings("NonAsciiCharacters")
		@RecordApplicationEvents

[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610

[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610

Conversation

greeng00se commented Oct 24, 2023

woosung1223 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

greeng00se Oct 25, 2023 • edited Loading

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

woosung1223 left a comment

Choose a reason for hiding this comment

greeng00se Oct 25, 2023 •

edited

Loading