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

chr0m3 · 2022-10-24T08:26:53Z

시간이 얼마 남지 않았다보니 마음이 급해 충분히 잘 되었을지 모르겠네요..
그래도 최선을 다해보았습니다.

chr0m3 · 2022-10-24T08:30:06Z

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

+
+    public final UUID productId;
+
+    public final Price pricePerUnit;


단순히 price를 사용하는 경우 메뉴 하나의 가격인지, 메뉴 가격에 수량을 곱한 소계에 해당하는 값인지 혼동되어 명확하게 네이밍했습니다.

chr0m3 · 2022-10-24T08:32:33Z

src/main/java/kitchenpos/menu/tobe/domain/service/MenuDisplayPolicy.java

+import org.springframework.stereotype.Service;
+
+@Service
+public class MenuDisplayPolicy {


이 때는 별도의 클래스로 분리했지만 실제로 Menu aggrigate만을 사용하므로 Menu 내부에 두어도 문제가 없을 것 같다는 생각을 했습니다. 별도의 클래스로 분리하는 것과 내부에 두는 것 서로 장단점이 있는 것 같습니다.

현재 구현해주신 코드는 Menu Aggregate만을 사용하게 되지만, 가격 변경시 메뉴에 속한 상품의 합보다 적거나 같아야한다.와 같은 정책이 지켜지지 않을것 같아요.
메뉴가 가지고 있는 가격이 현시점에 상품 가격이라고 확신할수 없는 구조 입니다.

chr0m3 · 2022-10-24T08:34:12Z

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

@@ -19,28 +19,28 @@ public class Menu {

    private final List<MenuProduct> menuProducts;

-    private Boolean isVisible;
+    private Boolean displayed;


굳이 isVisible로 바꿔야만 하는 이유가 없어서 불필요한 데이터베이스 변경을 만들지 않기 위해 displayed를 그대로 사용하기로 했습니다.

chr0m3 · 2022-10-24T08:37:39Z

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

@@ -6,7 +6,6 @@
 import kitchenpos.common.name.Name;
 import kitchenpos.common.price.Price;
 import kitchenpos.menu.tobe.domain.service.MenuDisplayPolicy;
-import kitchenpos.menu.tobe.domain.vo.MenuGroup;


구현을 하면서 MenuGroup에 id가 필요하다는 것을 알게 되었고, 그렇게 되면 MenuGroup entity와 vo의 차이가 없어져 entity를 그대로 사용하기로 했습니다.
다만 추후에 MenuGroup의 이름을 변경할 수 있게 되거나 하는 요구사항이 생긴다면 MenuGroup entity는 더이상 불변이 아니게 될 것이므로 이 때는 다시 불변인 VO를 추가하게 될 수도 있을 것 같습니다.

chr0m3 · 2022-10-24T08:38:53Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuProductEntityConverter.java

+    @Component
+    public static class MenuProductToMenuProductEntityConverter {
+
+        public MenuProductEntity convert(final MenuProduct menuProduct, final Menu menu) {


기존의 Converter 인터페이스로 커버가 안되는 케이스가 발생해버렸습니다.

chr0m3 · 2022-10-24T08:45:17Z

src/main/java/kitchenpos/menu/tobe/domain/repository/MenuGroupRepository.java

@@ -7,9 +7,9 @@

 public interface MenuGroupRepository {

-    MenuGroup save(MenuGroup menuGroup);
+    MenuGroup save(final MenuGroup menuGroup);


코틀린이 그립네요..

동감합니다 😢

chr0m3 · 2022-10-24T08:45:27Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuEntityConverter.java

+            this.nameFactory = nameFactory;
+        }
+
+        public Menu convert(


여기서도 기존의 Converter 인터페이스로 커버되지 않는 케이스가 발생했습니다.

chr0m3 · 2022-10-24T08:45:57Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuProductEntityConverter.java

@@ -34,5 +37,11 @@ public MenuProduct convert(final MenuProductEntity menuProductEntity) {
                new MenuProductQuantity(menuProductEntity.quantity)
            );
        }
+
+        public List<MenuProduct> convert(final Collection<MenuProductEntity> menuProductEntities) {


데이터베이스에서 조회하는 경우 Collection에 포함된 엔티티를 모두 변환해야 할 필요가 있어 추가했습니다.

chr0m3 · 2022-10-24T08:47:18Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/JpaMenuRepository.java

+        final List<MenuProductEntity> menuProducts = this.menuProductToMenuProductEntityConverter.convert(
+            menu
+        );
+        if (menuEntity.id != null) {


식별자로 사용할 수 있는 seq가 있기 때문에 더 나은 방법으로도 구현이 가능하겠지만 일단은 @ElementCollection처럼 모두 삭제하고 새로 만드는 방식을 택했습니다.

chr0m3 · 2022-10-24T08:49:45Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/JpaMenuRepository.java

+    }
+
+    @Override
+    public List<Menu> findAll() {


현재 구현은 조회해온 menu의 수 만큼 추가로 쿼리하게 되므로 n+1 쿼리 문제가 있긴 합니다.

chr0m3 · 2022-10-24T08:51:14Z

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

@@ -63,7 +63,7 @@ public void hide() {

    public void setPrice(final Price price) {
        if (!MenuDisplayPolicy.isDisplayable(this)) {
-            this.hide();
+            throw new IllegalArgumentException("메뉴 노출 정책에 따라 가격을 변경할 수 없습니다");


이전 과제에서 모델링할 당시 가격 변경시 정책을 위반하게 되면 메뉴를 숨기도록 해서 이를 따라 구현했는데, 정책을 위반하도록 가격을 변경할 수는 없는 것이 맞는 것으로 판단되어(기존 구현이 그렇게 되어 있으므로) 그렇게 변경했습니다.

chr0m3 · 2022-10-24T08:53:58Z

src/main/java/kitchenpos/menu/api/MenuGroupRestController.java

@@ -21,7 +22,7 @@ public MenuGroupRestController(final MenuGroupService menuGroupService) {
    }

    @PostMapping
-    public ResponseEntity<MenuGroup> create(@RequestBody final MenuGroup request) {
+    public ResponseEntity<MenuGroup> create(@RequestBody final CreateMenuGroupCommand request) {


도메인 모델을 그대로 받는 대신 요청하는 행위에 맞는 DTO를 도입해야 할 필요성이 느껴져 DTO를 추가했습니다.

DTO 도입 좋습니다 ㅎㅎ 😄

chr0m3 · 2022-10-24T08:55:02Z

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

-            throw new IllegalArgumentException();
-        }
+    public MenuGroup create(final CreateMenuGroupCommand request) {
+        final Name name = this.nameFactory.create(request.name);


이제 DTO를 받은 뒤 제대로 NameFactory를 사용해 Name을 생성하게 됩니다.

chr0m3 · 2022-10-24T08:56:30Z

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

@@ -36,6 +36,9 @@ public Menu(
        this.price = price;
        this.menuGroup = menuGroup;
        this.menuProducts = menuProducts;
+        if (!MenuDisplayPolicy.isDisplayable(this)) {


테스트코드를 보니 메뉴 노출 정책에 부합하지 않으면 메뉴 생성 자체가 불가능해야 하는 것으로 보여 Menu의 생성 시점에 이를 검사하도록 했습니다.

MenuDisplayPolicy를 static 메소드의 호출로 검증하는것과 파라미터 인자로 받아 검증하는것은 어떤 차이가 있을까요 ?

chr0m3 · 2022-10-24T08:57:57Z

src/test/java/kitchenpos/menu/application/MenuServiceTest.java

@@ -172,6 +173,7 @@ void display() {
        assertThat(actual.displayed()).isTrue();
    }

+    @Disabled


그런데 이 경우 메뉴의 가격이 상품 가격의 합보다 높으면 메뉴 생성 자체가 불가능하므로 일부 케이스에 대한 테스트가 불가능해져서 어떻게 해결하면 좋을지 고민입니다.

chr0m3 · 2022-10-24T08:59:05Z

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

            throw new IllegalArgumentException();
        }
+        final List<MenuProduct> menuProducts = menuProductRequests.stream()


기존 구현에는 command에 포함된 가격(아마 클라이언트에서 요청한 가격)을 그대로 사용한다는 문제점이 있었습니다.
생성 시점의 상품 가격을 직접 조회하여 반영하도록 개선했습니다.

chr0m3 · 2022-10-24T08:59:50Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuGroupEntityConverter.java

@@ -12,10 +11,8 @@ private MenuGroupEntityConverter() {
    }

    @Component
-    public static class MenuGroupToMenuGroupEntityConverter
-        implements Converter<MenuGroup, MenuGroupEntity> {
+    public static class MenuGroupToMenuGroupEntityConverter {


sah3122

메뉴 리팩터링 구현 잘해주셨습니다 👍
몇가지 코멘트 남겨두었는데 확인 부탁드려요 !
추가로 남아있는 미션을 조급하게 진행하지 않아도 됩니다 ㅎㅎ
강의 기간이 끝나도 리뷰는 진행할수 있습니다 😄

sah3122 · 2022-10-25T00:00:00Z

src/main/java/kitchenpos/menu/api/MenuGroupRestController.java

@@ -21,7 +22,7 @@ public MenuGroupRestController(final MenuGroupService menuGroupService) {
    }

    @PostMapping
-    public ResponseEntity<MenuGroup> create(@RequestBody final MenuGroup request) {
+    public ResponseEntity<MenuGroup> create(@RequestBody final CreateMenuGroupCommand request) {


DTO 도입 좋습니다 ㅎㅎ 😄

sah3122 · 2022-10-25T00:06:27Z

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

        if (Objects.isNull(menuProductRequests) || menuProductRequests.isEmpty()) {
            throw new IllegalArgumentException();
        }
-        final List<Product> products = productRepository.findAllByIdIn(
+        final Map<UUID, Price> productPrices = productRepository.findAllByIdIn(


Menu Application Layer에서 Product Context와의 결합을 제거할수 있는 방법을 고민해보세요 😄

sah3122 · 2022-10-25T00:08:13Z

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

-        final MenuGroup menuGroup = menuGroupRepository.findById(request.getMenuGroupId())
+    public Menu create(final CreateMenuCommand command) {
+        final Price price = new Price(command.price);
+        final MenuGroup menuGroup = menuGroupRepository.findById(command.menuGroupId)


동일한 Context에서 Root Aggregate간 결합을 허용할건지, 결합을 제거할건지 고민해보시는것도 권장 드립니다 😄

sah3122 · 2022-10-25T00:11:01Z

src/main/java/kitchenpos/menu/tobe/domain/repository/MenuGroupRepository.java

@@ -7,9 +7,9 @@

 public interface MenuGroupRepository {

-    MenuGroup save(MenuGroup menuGroup);
+    MenuGroup save(final MenuGroup menuGroup);


동감합니다 😢

sah3122 · 2022-10-25T00:16:03Z

src/main/java/kitchenpos/menu/tobe/domain/service/MenuDisplayPolicy.java

+import org.springframework.stereotype.Service;
+
+@Service
+public class MenuDisplayPolicy {


현재 구현해주신 코드는 Menu Aggregate만을 사용하게 되지만, 가격 변경시 메뉴에 속한 상품의 합보다 적거나 같아야한다.와 같은 정책이 지켜지지 않을것 같아요.
메뉴가 가지고 있는 가격이 현시점에 상품 가격이라고 확신할수 없는 구조 입니다.

sah3122 · 2022-10-25T00:16:42Z

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

+        this.displayed = false;
+    }
+
+    public void setPrice(final Price price) {


setter 네이밍을 사용하기 보다는 의미있는 네이밍을 고민해보세요 😄

sah3122 · 2022-10-25T23:49:44Z

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

@@ -36,6 +36,9 @@ public Menu(
        this.price = price;
        this.menuGroup = menuGroup;
        this.menuProducts = menuProducts;
+        if (!MenuDisplayPolicy.isDisplayable(this)) {


MenuDisplayPolicy를 static 메소드의 호출로 검증하는것과 파라미터 인자로 받아 검증하는것은 어떤 차이가 있을까요 ?

sah3122 · 2022-10-25T23:50:32Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuEntityConverter.java

+import org.springframework.beans.factory.annotation.Autowired;
+import org.springframework.stereotype.Component;
+
+public class MenuEntityConverter {


entity domain 간 분리를 위한 converter 도입 좋습니다 ㅎㅎ

sah3122 · 2022-10-25T23:50:42Z

src/main/java/kitchenpos/menu/tobe/infra/jpa/MenuEntityConverter.java

+
+        public MenuEntity convert(final Menu menu) {
+            final MenuEntity menuEntity = new MenuEntity();
+            menuEntity.id = menu.id;


public 인자로 정의한 이유가 있을까요 ?

chr0m3 added 30 commits October 23, 2022 19:25

Price에 multiply() 추가

bde7ea3

MenuGroup entity 추가

86b5760

MenuGroupEntity 추가

4022276

MenuGroupEntityConverter 추가

8382a9b

MenuGroupRepository 추가

a1644b4

JpaMenuGroupRepository, JpaMenuGroupDao 추가

b84cfe6

Price.multiply() 테스트 추가

eb121a6

Price에 add() 추가

4e44a8f

Price가 Comparable을 구현하도록 수정

2c52931

MenuProductQuantity VO 추가

0146904

MenuProduct VO 추가

a968bc6

MenuGroup VO 추가

7ff9973

Menu entity 추가

3621adb

MenuDisplayPolicy 추가

5aab011

MenuRepository 추가

58bd527

Menu의 isVisible을 displayed로 변경

0701e17

MenuEntity 추가

bf9bfb1

MenuGroup VO 삭제

44adb96

MenuProductEntity 추가

2d16e74

MenuProductEntityConverter 추가

218f580

누락된 final 추가

c346395

MenuEntityConverter 추가

26aa5e7

JpaMenuRepository 추가

ac684f6

JpaMenuRepository 리팩터링 - 중복 코드 제거

748ee71

메뉴 노출 정책을 위반하는 가격 변경이 불가능하도록 수정

96fe49a

새로운 도메인 모델을 사용하도록 Menu 컨텍스트 수정

f795b5c

불필요해진 과거 코드 제거

2c7035e

CreateMenuGroupCommand DTO 추가

a662282

ChangeMenuPriceCommand DTO 추가

a753755

CreateMenuCommand DTO 추가

86dc9e6

chr0m3 added 4 commits October 24, 2022 17:05

메뉴 노출 정책을 위반하는 메뉴 생성이 불가능하도록 수정

81afbbc

메뉴 생성시 정확한 상품 가격을 반영하도록 개선

bd08ccb

DB 마이그레이션

71f200a

도메인 모델 수정

139c6f8

chr0m3 commented Oct 24, 2022

View reviewed changes

Converter 인터페이스 제거

1c460a0

chr0m3 commented Oct 24, 2022

View reviewed changes

sah3122 requested changes Oct 25, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chr0m3 commented Oct 24, 2022 •

edited

Loading

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

sah3122 Oct 25, 2022

chr0m3 Oct 24, 2022 •

edited

Loading

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

sah3122 Oct 25, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022 •

edited

Loading

chr0m3 Oct 24, 2022

sah3122 Oct 25, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

sah3122 Oct 25, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

chr0m3 Oct 24, 2022

sah3122 left a comment

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022

sah3122 Oct 25, 2022


		public final UUID productId;

		public final Price pricePerUnit;

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

Are you sure you want to change the base?

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

Conversation

chr0m3 commented Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chr0m3 Oct 24, 2022 • 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

chr0m3 Oct 24, 2022 • 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

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

chr0m3 commented Oct 24, 2022 •

edited

Loading

chr0m3 Oct 24, 2022 •

edited

Loading

chr0m3 Oct 24, 2022 •

edited

Loading