[김대겸] Step3 PR #735

Gyeom · 2022-12-18T12:46:36Z

안녕하세요 리뷰어님,

3단계 의존성 리팩터링 리뷰요청드립니다.

패키지 분리 (Lifecycle이 같은 도메인들은 같은 Aggregate)
서로 다른 Aggregate의 엔티티간의 단방향 참조를 할 수 있도록 간접참조 활용
비즈니스 로직을 보다 명확하고 쉽게 관리할 수 있도록 Validator를 활용한 절차지향 적용
Order <-> OrderTable 간의 양방향 참조 문제를 해결하기 위해 인터페이스 및 구현체 활용 (DIP 원칙 적용)

이번 미션은 위에 작성한 내용을 중심으로 개발하였습니다.

이번 리뷰도 잘 부탁드립니다 🙏 🙏 🙏

…or 추가

limwoobin

안녕하세요 대겸님
3단계 반영 잘 해주셧습니다 ㅎㅎ

몇가지 고민해보면 좋을것같은 부분에 대해 코멘트를 남겨드렸습니다.
확인해보시고 다시 요청부탁드립니다!

limwoobin · 2022-12-18T13:43:02Z

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


+        menuValidator.validateCreateMenu(request, menuProducts);


validator를 메소드 맨 앞에서 처리하는건 어떨까요??

그게 더 나을 것 같네요~! validator 를 앞에서 처리하도록 수정하겠습니다ㅎㅎ

limwoobin · 2022-12-18T13:44:33Z

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

+            final ProductRepository productRepository,
+            final MenuValidator menuValidator


productRepository도 MenuValidator에서 직접 의존하는건 어떨까요??
MenuValidator를 별도로 만드셧는데 해당 서비스코드에 있는 validate 로직은
이동시켜도 좋을것같습니다!

그렇게되면 menuservice 는 정말 menu도메인에 대해서만 의존할 수 있겠네요~!ㅎㅎ

limwoobin · 2022-12-18T13:49:52Z

src/main/java/kitchenpos/menu/validator/MenuValidator.java

+@Component
+public class MenuValidator {
+    private final MenuGroupRepository menuGroupRepository;
+
+    public MenuValidator(MenuGroupRepository menuGroupRepository) {
+        this.menuGroupRepository = menuGroupRepository;
+    }
+
+    public void validateCreateMenu(MenuRequest menuRequest, MenuProducts menuProducts) {
+        validateMenuGroup(menuRequest.getMenuGroupId());
+        validateMenuProducts(menuRequest, menuProducts);
+    }
+
+    private void validateMenuGroup(Long menuGroupId) {
+        if (!menuGroupRepository.existsById(menuGroupId)) {
+            throw new IllegalArgumentException(INVALID_MENU_GROUP.getMessage());
+        }
+    }
+
+    private void validateMenuProducts(MenuRequest menuRequest, MenuProducts menuProducts) {
+        if (menuRequest.getMenuProductsRequest().size() != menuProducts.value().size()) {
+            throw new IllegalArgumentException(INVALID_MENU_PRODUCT.getMessage());
+        }
+
+        Price price = Price.from(menuRequest.getPrice());
+        if (price.compareTo(menuProducts.totalPrice()) > 0) {
+            throw new IllegalArgumentException(MENU_PRICE_LESS_THAN_SUM_OF_PRICE.getMessage());
+        }
+    }
+}


validator로 잘 분리해주셨네요 ㅎㅎ 좋습니다
굳이 필수로 적용하실 필요는 없지만 해당 validator를 SRP, OCP를 준수해 별도의 validator로 분리해보는것에 대해
고민해보셔도 좋을것같아요
그리고 해당 validator들을 관리하는 별도의 객체를 생성하는 방법으로 풀어보면 보다 확장에 용이할 수 있을것같기도 하네요 :)

limwoobin · 2022-12-18T13:50:44Z

src/main/java/kitchenpos/menugroup/domain/MenuGroup.java

@@ -1,4 +1,4 @@
-package kitchenpos.domain;
+package kitchenpos.menugroup.domain;


해당 엔티티에서는 기본생성자의 접근제어자가 public이네요
protected로 제한해서 사용하면 무분별한 객체생성을 막을수 있을것같아요 ㅎㅎ
추가로 jpa에서 기본생성자를 private으로 사용했을때는 어떤 문제점이 있을지에 대해서도 같이 알아보시면 좋을것같네요!

public으로 되어 있는 엔티티가 꽤많네요 😢 수정하도록 하겠습니다~ ㅎㅎ

jpa가 프록시를 사용할 때 jpa hibernate가 객체를 강제로 생성해야되는데, 접근제한자가 private일 경우 생성을 못해서 예외가 발생하는 것으로 알고 있습니다~!ㅎㅎ

limwoobin · 2022-12-18T13:53:22Z

src/main/java/kitchenpos/order/application/OrderService.java

-        Long orderTableId = request.getOrderTableId();
-        OrderTable orderTable = orderTableRepository.findById(orderTableId)
-                .orElseThrow(() -> new IllegalArgumentException(String.format(ErrorMessage.NOT_FOUND_ORDER_TABLE.getMessage(), orderTableId)));
+        orderValidator.validate(request);


validateOrderLineItems 메소드는 orderValidator로 위임 안하신 이유가 있나요??

누락된것 같네요 ㅠㅠ 죄송스럽네요........ 😢

limwoobin · 2022-12-18T13:57:46Z

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

-package kitchenpos.domain;
+package kitchenpos.order.domain;
+
+import kitchenpos.common.domain.Quantity;


메뉴의 이름과 가격이 변경되면 주문 항목도 함께 변경된다. 메뉴 정보가 변경되더라도 주문 항목이 변경되지 않게 구현한다. 와 같은 요구사항이 있습니다.
해당 요구사항이 충족되지 않은걸로 보이네요 :)

요구사항을 빠뜨렸군요 ㅠㅠㅠㅠㅠㅠ
OrderLineItem에 menuName과 price칼럼을 추가하여 요구사항에 충족하도록 수정하겠습니다~!

limwoobin · 2022-12-18T14:07:07Z

src/main/java/kitchenpos/ordertable/application/TableService.java

+    private final OrderRepository orderRepository;
+    private final OrderValidator orderValidator;


Order < -> OrderTable 양방향 의존관계를 맺고있네요
어떻게 의존관계를 끊으면 좋을지 고민해보심 좋을것같아요

추가로 패키지구조가 엔티티단위로 모두 나누어져 있네요 ㅎㅎ
별도의 이유가 있으시면 코멘트로 남겨주심 좋을것같아요
한편으론 같은 성격이라면 패키지를 합치는것도 고민해봐주시면 좋겟습니다 :)

Order < -> OrderTable 양방향 의존관계를 맺고있네요

인터페이스 및 구현체(Validator, ValidatorImpl) 활용하여 DIP 원칙을 적용하여
Order <-> OrderTable 간의 양방향 참조 문제를 해결했다고 생각했는데,, 제가 이해하지 못한 내용이 있을까요...?

추가로 패키지구조가 엔티티단위로 모두 나누어져 있네요 ㅎㅎ
별도의 이유가 있으시면 코멘트로 남겨주심 좋을것같아요
한편으론 같은 성격이라면 패키지를 합치는것도 고민해봐주시면 좋겟습니다 :)

패키지 구조의 경우 Aggregate 단위로 나눴습니다. Aggregate의 기준은 Lifecycle이 같은 도메인들끼리 그룹핑했습니다 :)

refactor(OrderValidator): Order, OrderTable 양방향 의존성 제거를 위한 DIP 적용(abbc0af)
리뷰할때 해당커밋이 없는버전으로 리뷰했던것같았는데 뭔가 혼선이 있었나보네요!
말씀하신대로 인터페이스 이용해 의존성 잘 끊어내주신게 맞네요 👍

limwoobin · 2022-12-18T14:10:33Z

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

+package kitchenpos.menu.domain;
+
+import kitchenpos.common.domain.Price;
+import kitchenpos.product.domain.Product;


여기에선 Product를 직접참조방식으로 유지하셧네요 ㅎㅎ
혹시 직접참조방식을 유지하신 이유가 있을까요??
잘못되었다는건 아니고 의견이 궁금해서 여쭤봅니다!

관계가 단방향(MenuProduct -> Product)이라고 생각되었고, 앞으로도 양방향으로 바뀔일은 없다고 판단하여 직접참조방식을 유지했습니다 :)

양방향.단방향에 대한 질문은 아니었고
라이프 사이클관점에선 두 객체가 다름에도 id로 참조하지 않고 직접참조(객체참조)로 작성하신 이유가 궁금했습니다 ㅎㅎ
정답은 없는문제이기에 라이프사이클이 굳이 같지않더라도 빈번하게 같이 사용된다면 전 직접참조도 괜찮다고 생각들긴 하네요!

Gyeom · 2022-12-20T12:07:56Z

리뷰어님, 피드백 반영하여 리뷰 요청드립니다~!

저번 리뷰요청 때, 실수를 많이해서 죄송스럽네요 ㅠㅠ

이번 리뷰도 잘 부탁드립니다 :)

감사합니다.

limwoobin · 2022-12-20T13:43:14Z

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

+    private Long menuId;
+    private String menuName;
+    @Embedded
+    @AttributeOverride(name = "price", column = @Column(name = "menu_price"))
+    private Price menuPrice;


피드백 잘 반영해주셨네요 ㅎㅎ
menuId, menuName, menuPrice 에 대해 일급컬렉션으로 묶는게
보다 명시적이지 않을까 싶습니다!

그러면 보다 쉽게 관리할 수 있겠네요~~!ㅎㅎ 수정하도록 하겠습니다

그런데 혹시 해당 경우에도 일급컬렉션이라고 지칭하나요~~?
저는 Collection들을 한번 래핑한 컬렉션정도로 생각하고 있었는데 궁금합니다~

@Gyeom 제가 잘못말씀드렸네요 ㅎㅎ
일급컬렉션이아닌 객체로 래핑하는걸 의도해서 말씀드린게 맞습니다!

limwoobin · 2022-12-20T13:48:17Z

src/main/java/kitchenpos/ordertable/application/TableService.java

+    private final OrderRepository orderRepository;
+    private final OrderValidator orderValidator;


refactor(OrderValidator): Order, OrderTable 양방향 의존성 제거를 위한 DIP 적용(abbc0af)
리뷰할때 해당커밋이 없는버전으로 리뷰했던것같았는데 뭔가 혼선이 있었나보네요!
말씀하신대로 인터페이스 이용해 의존성 잘 끊어내주신게 맞네요 👍

limwoobin

대겸님 3단계 피드백 잘 반영해주셨습니다 👍👍
이제 4단계 하나만 남았네요!

코멘트 몇개 남겨드렸으니 다음미션 진행하면서 같이 확인하시면 좋을것같습니다 ㅎㅎ
조금만 더 화이팅입니다!

Gyeom added 14 commits December 16, 2022 17:18

refactor(menuService): 상품조회 메소드 네이밍 변경

9d2577a

refactor(tableGroupService): 상품조회 메소드 네이밍 변경

0b504b2

refactor: yml을 활용하여 batch size 설정

41d7985

refactor: toEntity에서 to도메인 으로 네이밍 변경

2da19fa

fix: 불필요 어노테이션 제거

59b5489

docs(readme): step3 요구사항 작성

981fa93

refactor(package): 패키지 분리

da282cb

refactor(validator): Menu, MenuGroup 의존성 제거를 위해 Validator 사용

91aa044

refactor(validator): Order, OrderTable, TableGroup 의존성 제거를 위한 Validat…

300e21e

…or 추가

refactor: 사용하지 않는 코드 제거

721a20a

refactor: 테스틐 코드 패키지 분리

ada6aff

test(OrderValidator): OrderValidator 테스트 코드 추가

5c447b9

test(MenuValidator): MenuValidator 테스트 코드 추가

7d50b0e

refactor(OrderValidator): Order, OrderTable 양방향 의존성 제거를 위한 DIP 적용

abbc0af

limwoobin requested changes Dec 18, 2022

View reviewed changes

Gyeom added 5 commits December 20, 2022 19:33

refactor(menuService): validator 위치 변경

0725762

refactor: menuValidator에서 productRepository 직접 의존하도록 변경

7455f70

refactor(entity): protected로 접근제한자 변경

47d0fa8

refactor: validateOrderLineItems 함수를 orderValidator에서 사용하도록 변경

362c0b7

refactor(orderLineItem): 메뉴의 이름과 가격이 변경되어도 주문항목이 변경되지 않도록 테이블 컬럼 추가

15465fa

limwoobin reviewed Dec 20, 2022

View reviewed changes

limwoobin approved these changes Dec 20, 2022

View reviewed changes

limwoobin merged commit 105f996 into next-step:gyeom Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[김대겸] Step3 PR #735

[김대겸] Step3 PR #735

Gyeom commented Dec 18, 2022 •

edited

limwoobin left a comment

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

limwoobin Dec 18, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022 •

edited

limwoobin Dec 18, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

Gyeom Dec 20, 2022 •

edited

limwoobin Dec 20, 2022

limwoobin Dec 18, 2022

Gyeom Dec 20, 2022

limwoobin Dec 20, 2022

Gyeom commented Dec 20, 2022

limwoobin Dec 20, 2022

Gyeom Dec 24, 2022

limwoobin Dec 24, 2022

limwoobin Dec 20, 2022

limwoobin left a comment

		final ProductRepository productRepository,
		final MenuValidator menuValidator

		@@ -1,4 +1,4 @@
		package kitchenpos.domain;
		package kitchenpos.menugroup.domain;

		private final OrderRepository orderRepository;
		private final OrderValidator orderValidator;

[김대겸] Step3 PR #735

[김대겸] Step3 PR #735

Conversation

Gyeom commented Dec 18, 2022 • edited

limwoobin 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

Gyeom Dec 20, 2022 • edited

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

Gyeom Dec 20, 2022 • edited

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

Gyeom commented Dec 20, 2022

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

limwoobin left a comment

Choose a reason for hiding this comment

Gyeom commented Dec 18, 2022 •

edited

Gyeom Dec 20, 2022 •

edited

Gyeom Dec 20, 2022 •

edited