🚀 2단계 - 리팩터링(메뉴) #101

dacapolife87 · 2022-04-13T15:34:27Z

안녕하세요 리뷰어님
2단계 메뉴 리팩토링 진행하였습니다.
일단 전체적으로 리팩토링이 진행이 아니라
메뉴 부분만 리팩토링을 진행하다보니 서비스레이어가 정리가 덜 된 기분이 드네요
최대한 메뉴관련된 부분 도메인 내부로 옮겨서 처리가 되도록 수정해보았습니다.
이번에도 많은 지적과 피드백 부탁드리겠습니다
감사합니다.

- MenuName, MenuPrice, MenuGroupName 추출 - 패키지 이동

- 값 객체 테스트코드 작성

- Dto 객체 사용으로 Entity분리

sah3122

메뉴 리팩토링 잘해주셨습니다 👍
몇가지 생각거리를 남겨두었는데, 질문이 있다면 편하게 DM 주세요 🙏

sah3122 · 2022-04-14T13:58:57Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+            columnDefinition = "varbinary(16)",
+            foreignKey = @ForeignKey(name = "fk_menu_to_menu_group")
+    )
+    private MenuGroup menuGroup;


MenuGroup Aggregate를 직접 참조로 구현한 이유가 있을까요 ?

sah3122 · 2022-04-14T13:59:19Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+            columnDefinition = "varbinary(16)",
+            foreignKey = @ForeignKey(name = "fk_menu_product_to_menu")
+    )
+    private List<MenuProduct> menuProducts;


MenuProduct 일급 컬렉션을 만들어 사용해보세요 😄

sah3122 · 2022-04-14T14:00:01Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+    )
+    private List<MenuProduct> menuProducts;
+
+    public Menu() {


JPA Entity의 기본 생성자는 protected 접근제어자를 선언하는게 어떨까요 ?

https://github.com/next-step/ddd-tactical-design/pull/101/files#r850475902 를 확인해주세요 :)

sah3122 · 2022-04-14T14:02:37Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+    public Menu() {
+    }
+
+    public Menu(PurgomalumClient purgomalumClient, String name, BigDecimal price, MenuGroup menuGroup, boolean displayed, List<MenuProduct> menuProducts) {


Menu라는 Aggregate 는 정의된 불변식(메뉴 가격은 속해있는 상품의 가격보다 작거나 같다. 등)을 모두 만족해야 합니다.
어떤 불변식들이 존재하고, 도메인 로직에 어떤 방법으로 녹일수 있는지 고민해보세요.

sah3122 · 2022-04-14T14:05:18Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+
+    }
+
+    public void show() {


메뉴를 노출하기 위한 요구사항을 만족시키지 못할거 같아요. 😄

sah3122 · 2022-04-14T14:06:39Z

src/main/java/kitchenpos/menus/tobe/domain/MenuProduct.java

-        foreignKey = @ForeignKey(name = "fk_menu_product_to_product")
+            name = "product_id",
+            columnDefinition = "varbinary(16)",
+            foreignKey = @ForeignKey(name = "fk_menu_product_to_product")
    )
    private Product product;


Product 도메인을 직접참조로 구현한 이유가 있을까요 ?
서로 다른 컨텍스트의 Aggregate간 결합도를 낮춘다면 어떤 장점이 있을지 고민해보세요 😄

sah3122 · 2022-04-14T14:12:05Z

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

-        if (Objects.isNull(price) || price.compareTo(BigDecimal.ZERO) < 0) {
-            throw new IllegalArgumentException();
-        }
+    public MenuResponse create(final MenuRequest request) {


아직 중요한 비즈니스 로직이 서비스 레이어에 노출되어 있는것 같아요.
만약 현재 구조에서 다른 개발자분이 서비스 레이어에 있는 로직을 보지못하고 Menu 도메인 객체를 생성한다면 Menu 객체는 완전한 객체라고 할수 없을것 같아요 😢
노출되어 있는 비즈니스 로직을 도메인 내부로 옮겨보세요 😄

sah3122 · 2022-04-14T14:14:58Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+
+@Table(name = "menu")
+@Entity
+public class Menu {


새롭게 정의한 클래스들의 테스트 코드가 필요해보여요.
domain 패키지에 존재하는 클래스들은 단위테스트를 필수로 작성해보세요 😄

dacapolife87 · 2022-04-21T15:07:34Z

안녕하세요 리뷰어님
오랜만에 리뷰요청드리게되었습니다.
오랜기간을 고민하다가 도저히 안되겠어서...
과제 초기로 돌아가서 다시 시작하게되었습니다.
첫 단추를 잘못시작해서 리팩토링을 진행하려고 보니 계속해서 더 잘안되게 되네요
그래도 다시 미션 처음부터 진행해보니 조금씩 미션진행을 할수 있었습니다.
많은 피드백 및 지적 부탁드리겠습니다
감사합니다.

sah3122

피드백 반영 잘해주셨네요 👍
몇가지 생각거리를 추가로 남겨두었어요.
질문이 있으면 언제든지 DM 주세요 😄

sah3122 · 2022-04-22T08:29:52Z

src/main/java/kitchenpos/common/domain/Price.java

+import java.util.Objects;
+
+@Embeddable
+public class Price {


Value Ojbect의 불변성, 값 동등성을 위해 Equals & HashCode 를 추가해보는건 어떨까요 ?

sah3122 · 2022-04-22T08:31:24Z

src/main/java/kitchenpos/common/domain/Price.java

+    public static Price add(Price totalPrice, Price addPrice) {
+        return new Price(totalPrice.price.add(addPrice.price));
+    }


Suggested change

public static Price add(Price totalPrice, Price addPrice) {

return new Price(totalPrice.price.add(addPrice.price));

}

public Price add(Price addPrice) {

return new Price(this.price.add(addPrice.price));

}

static 메소드를 제공하기 보다는 객체에 메시지를 던질수 있도록 상태값을 이용하는것이 좋아보여요 😄

sah3122 · 2022-04-22T08:33:42Z

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

-        }
-        menu.setPrice(price);
+
+        List<Product> findProducts = productRepository.findAllByIdIn(menu.getProductIds());


Product 도메인이 MenuContext에 존재하지 않도록 도메인 서비스를 구현해보는건 어떨까요 ?

sah3122 · 2022-04-22T08:34:41Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+    )
+    private List<MenuProduct> menuProducts;
+
+    public Menu() {


https://github.com/next-step/ddd-tactical-design/pull/101/files#r850475902 를 확인해주세요 :)

sah3122 · 2022-04-22T08:35:39Z

src/main/java/kitchenpos/menus/tobe/domain/Menu.java

+    public Menu() {
+    }
+
+    public Menu(PurgomalumClient purgomalumClient, UUID id, String name, Price price, UUID menuGroupId, boolean displayed, MenuProducts menuProducts) {


생성자가 다양해지는경우 주 / 부 생성자 개념을 도입하여 보는건 어떨까요 ?
https://jackjeong.tistory.com/148

sah3122 · 2022-04-22T08:37:15Z

src/main/java/kitchenpos/menus/tobe/domain/MenuGroup.java

+    @Embedded
+    private MenuGroupName name;
+
+    public MenuGroup() {


https://github.com/next-step/ddd-tactical-design/pull/101/files#r850475902
위 코멘트를 확인부탁드려요

sah3122 · 2022-04-22T08:38:00Z

src/main/java/kitchenpos/menus/tobe/domain/MenuGroup.java

+    private void validation(String name) {
+        if (Objects.isNull(name) || name.isEmpty()) {
+            throw new IllegalArgumentException("메뉴그룹의 이름은 Null또는 공백이 될수 없습니다.");
+        }
+    }


메뉴 그룹 이름과 같은 비즈니스 요구사항은 MenuGroupName VO가 담당하는건 어떨까요 ?

sah3122 · 2022-04-22T08:52:37Z

src/main/java/kitchenpos/menus/tobe/domain/MenuProduct.java

-    )
-    private Product product;
+    @Column(name = "product_id", columnDefinition = "varbinary(16)", nullable = false)
+    private UUID productId;


메뉴 가격과 관련된 불변성을 지키기 위하여 크게 2가지 정도의 방안이 있을것 같아요.

MenuProduct에서 Product의 가격 정보를 가지고 있는다.

MenuProduct에서 도메인 서비스를 통하여 Product의 가격 Validation을 진행한다.

2가지 방법 모두 많이 사용되고, 장단점이 있어요.
장단점에 대하여 고민해보시고 미션 진행 해보세요 😄

이번 미션에서 가장 중요한 부분이라고 생각하여 조금은 두루뭉실하게 코멘트를 남겨드릴게요.
구현에 대하여 궁금증이 있으실때는 편하게 Slack DM 부탁드릴게요 !

sah3122 · 2022-04-22T08:53:23Z

src/main/java/kitchenpos/menus/tobe/domain/MenuProducts.java

+import kitchenpos.common.domain.Price;
+import kitchenpos.menus.ui.dto.MenuProductRequest;
+import kitchenpos.products.tobe.domain.Product;


도메인 코드에서는 외부 Context간 결합도가 없으면 좋을것 같아요. 😄

sah3122 · 2022-04-22T08:54:32Z

src/main/java/kitchenpos/menus/tobe/domain/MenuProducts.java

+import kitchenpos.menus.ui.dto.MenuProductRequest;
+import kitchenpos.products.tobe.domain.Product;
+
+import javax.persistence.*;


패키지 내에 모든 클래스를 import할 필요가 있을까요 ?
https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports

dacapolife87 · 2022-05-03T12:09:34Z

안녕하세요 리뷰어님
생각을 좀 많이 하느랴 리뷰요청이 늦었습니다.
저번에 리뷰주신내용중에 아직 하나가 좀 이해가 되지않아서 생각이 많았는데요
'Product 도메인이 MenuContext에 존재하지 않도록 도메인 서비스를 구현해보는건 어떨까요 ?' 이부분 입니다.
DM으로도 질문을 드렸었는데요
'Menu와 Product도메인이 공존하는 도메인 서비스를 구현해보는것을 권장 드린것입니다. ' 이렇게 말씀을 해주셨는데
2개의 도메인이 공존하는 도메인서비스 라는건
Menu에서 Product가 직접참조를 하게되는걸까 라는 생각도 해보았는데
Menu를 생성하는부분에서는 직접참조를 할 객체도 존재하지않아서 product를 조회해서 가격비교를 해야하는 상황이고...
도저히 답이 안나오는듯하여 일단 리뷰요청을 드리면서 다시 질문을 드리게되었습니다.
나머지 피드백 주신부분은 최대한 수정을 해보았는데요

부족한 부분이나 잘못된점있으면 많은 지적 부탁드리겠습니다
감사합니다!

sah3122

리뷰가 많이 늦어져 죄송합니다. 🙏
질문주신 내용에 대한 답변을 남겨두었는데 확인 부탁드려요 !

sah3122 · 2022-05-09T03:37:21Z

src/main/java/kitchenpos/menus/tobe/domain/MenuProducts.java

+        this.menuProducts = menuProducts;
+    }
+
+    public MenuProducts(List<MenuProductDto> menuProductDtos, List<Product> products, Price menuPrice) {


도메인 서비스 구현에 관해 어려움을 겪고 있는것 같아 제가 구현하는 방법을 소개 시켜드릴게요.
우선 앞서 이야기 드린것처럼 Aggreagte간 간접참조를 이용하면서 서로 다른 Context간의 결합을 낮추기위해 도메인서비스를 사용하곤 해요.

아래 코드는 예시이니 참고만 부탁드려요 🙏
MenuProducts 를 생성할떄 상품이 존재하는지, 가격이 유효한지 판단하기 위해선 아래와 같이 구현할 수 있을것 같아요.

public MenuProducts(List<MenuProductDto> menuProductDtos, MenuCreateService menuCreateService, Price menuPrice) { menuCreateService.validate(menuProductDtos); ... } public class MenuCreateService { private final ProductRepository productRepository; public void validate(List<MenuProductDto> menuProductDtos) { menuProductDtos.forEach(dto -> { Product product = productRepository.findById(dto.getProductId()); // 메뉴 가격과 관련된 유효성 검사 실행 }) } }

개인적으로는 위와 같이 생성자 또는 메소드 파라미터로 도메인 서비스를 전달 받아 도메인 서비스 내부에서 외부 Context의 도메인과 협력하여 요구사항을 구현하는 방법을 선호합니다.

위 코드에서는 Repository 를 주입받았지만 이와 같은 형태는 동일한 프로젝트에서 패키지 단위로 컨텍스트가 구분되어 있을때 사용할 수 있는 방법이에요.

마이크로 서비스로 구현되어 있는 프로젝트에서는 각 프로젝트가 물리적으로 분리되어 있기 때문에 Repository가 아닌 외부 Client를 사용하게 되겠죠.

위처럼 구현하는것에 대한 장점은 Aggregate / Service 레이어는 다른 Context와 결합이 없어지며 구조가 변경되는 경우 (모놀리식아키텍처에서 마이크로서비스로 분리되는 경우)에는 도메인서비스에서 주입받는 Repository의 구현체를 변경해주는것으로 마이크로 서비스로의 분리가 가능하다는것이에요.

우선 위처럼 구현하는것을 도전해보세요 !

dacapolife87 added 8 commits April 14, 2022 00:18

refactor : Request, Response 객체 생성

54fc7db

refactor : domain 리팩토링

23fd6cd

- MenuName, MenuPrice, MenuGroupName 추출 - 패키지 이동

test : 테스트 코드 작성

5aba3a1

- 값 객체 테스트코드 작성

refactor : Package 변경 및 불필요 임포트 제거

f5f8269

refactor : Controller에서 Entity 분리

67170c2

- Dto 객체 사용으로 Entity분리

test : Dto 사용에 따른 테스트코드 수정

64dfd9c

test : setter 제거후 생성자 사용

5c159c0

refactor : 값객체내에서 처리가능한 로직 이동

1de0bdb

sah3122 requested changes Apr 14, 2022

View reviewed changes

dacapolife87 added 13 commits April 17, 2022 21:33

refactor : Price 공통 도메인으로 추출

23058a2

refactor : MenuProduct setter 제거

5279b6f

refactor : MenuProduct 가격체크 로직 상위로 이동

1ce2bf9

refactor : MenuProducts 일급컬렉션생성

79a7593

refactor : MenuProductsRequest생성 및 Entity분리

ec823ac

refactor : formatting 및 패키지 변경

19a1ed1

refactor : Price 공통 도메인으로 이동 및 MenuPrice & ProductPrice 통합

97ada58

feat : request 객체 생성 및 컨트롤러 분리

742c136

feat : 패키지 이동 및 도메인 기능 구현

cd7e080

feat : 간접참조로 변경

f32cc8a

refactor : 도메인으로 로직 이동

134c1c4

test : 테스트로직 수정 및 신규작성

fc0ffb0

refactor : 응답규격 확인불가로 response 객체는 일단 제거

4408754

sah3122 requested changes Apr 22, 2022

View reviewed changes

dacapolife87 added 5 commits May 1, 2022 13:42

refactor : validation 제거

eec9f36

refactor : request 객체와 dto객체 분리

743a4da

refactor : equals, hashcode추가

075b342

test : request객체 dto객체로 변경

0f60d64

refactor : 가격비교로직 MenuProducts로 이동

489c1c2

sah3122 requested changes May 9, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚀 2단계 - 리팩터링(메뉴) #101

🚀 2단계 - 리팩터링(메뉴) #101

dacapolife87 commented Apr 13, 2022

sah3122 left a comment

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

sah3122 Apr 22, 2022

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

sah3122 Apr 14, 2022

dacapolife87 commented Apr 21, 2022

sah3122 left a comment

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

sah3122 Apr 22, 2022

dacapolife87 commented May 3, 2022

sah3122 left a comment

sah3122 May 9, 2022

🚀 2단계 - 리팩터링(메뉴) #101

Are you sure you want to change the base?

🚀 2단계 - 리팩터링(메뉴) #101

Conversation

dacapolife87 commented Apr 13, 2022

sah3122 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

dacapolife87 commented Apr 21, 2022

sah3122 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

dacapolife87 commented May 3, 2022

sah3122 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment