🚀 1단계 - 리팩터링(상품) by jwoo-o · Pull Request #336 · next-step/ddd-tactical-design

jwoo-o · 2025-02-17T14:19:56Z

안녕하세여 리뷰어님 1단계 리펙터링(상품)
리뷰 요청 드립니다.

product domain 기준으로 name, price를 일급객체로 분리하여
해당 객체에서 검증하도록 작성하였습니다.

리뷰 잘 부탁드립니다.

doc : 요구사항 작성

kkt219a

안녕하세요 진우님 ! 😊
이번 미션을 함께하게 된 김규태입니다.
1단계 미션 잘 진행해주셨어요! 🔥
몇가지 코멘트를 남겼는데 확인 부탁드려요!
궁금하신 점 있으시면 편하게 댓글이나 DM으로 연락주세요.☺️

kkt219a · 2025-02-18T09:41:19Z

src/main/java/kitchenpos/products/tobe/application/ProductService.java

+        final List<Menu> menus = menuRepository.findAllByProductId(productId);
+        for (final Menu menu : menus) {
+            BigDecimal sum = BigDecimal.ZERO;
+            for (final MenuProduct menuProduct : menu.getMenuProducts()) {
+                sum = sum.add(
+                    menuProduct.getProduct()
+                               .getPrice()
+                               .multiply(BigDecimal.valueOf(menuProduct.getQuantity()))
+                             );
+            }
+            if (menu.getPrice().compareTo(sum) > 0) {
+                menu.setDisplayed(false);
+            }
+        }


ProductService의 메뉴 가격에 대한 검증책임을 분리해보면 어떨까요? :)

다음 단계인 메뉴 리펙터링부분 에서 해당 코드가 변경이 될거 같습니다!

kkt219a · 2025-02-18T09:42:38Z

src/main/java/kitchenpos/products/tobe/domain/Product.java

+    public void updatePrice(BigDecimal price) {
+        this.price = ProductPrice.from(price);
+    }


Service에서는 changePrice, 도메인에서는 updatePrice라고 표현해주셨는데,
둘을 다르게 표현한 의도 있으실까요? :)
그리고 ProductPrice 대신 BigDecimal을 파라미터로 받게 한 이유도 궁금합니다 ㅎㅎ

request의 값을 넘겨주면 Product에서 값객체에 대한 생성에 대한 책임을 가져갈려고 하였습니다.

kkt219a · 2025-02-18T09:43:13Z

src/main/java/kitchenpos/products/tobe/domain/ProductName.java

+import kitchenpos.products.tobe.infra.PurgomalumClient;
+
+@Embeddable
+public class ProductName {


vo로 정의해보셨군요 👍👍

kkt219a · 2025-02-18T09:45:24Z

src/main/java/kitchenpos/products/tobe/infra/PurgomalumClient.java

@@ -0,0 +1,5 @@
+package kitchenpos.products.tobe.infra;
+
+public interface PurgomalumClient {


PurgomalumClient 인터페이스가 infra 패키지에 위치하고 있는데, ProductName 생성 시 직접 참조되고 있습니다. domain 패키지에 위치하는 것이 더 자연스러울 것 같은데 어떻게 생각하시나요? 그리고 진우님이 생각하시기에 비속어 검증은 도메인의 책임이 맞다고 보시는지 궁금합니다 ㅎㅎ

domain에 존재해야 되는거 같아요!
그리고 비속어 검증은 결국 product에 이름이 비속어 들어갈 수 없다는 비즈니스 규칙이므로 도메인의 책임인거 같아요!

kkt219a · 2025-02-18T09:48:49Z

src/main/java/kitchenpos/products/tobe/domain/ProductPrice.java

+import java.math.BigDecimal;
+
+@Embeddable
+public class ProductPrice {


ProductPrice pp1 = ProductPrice.from(new BigDecimal(500L)) ProductPrice pp2 = ProductPrice.from(new BigDecimal(500L))

pp1과 pp2는 같을까요? :)

euqals and hashcode가 필요하겠군요

kkt219a · 2025-02-18T09:49:15Z

src/main/java/kitchenpos/products/tobe/domain/ProductPrice.java

+    }
+
+    public static ProductPrice from(BigDecimal price) {
+        validate(price);


생성자에서 검증하시지 않은 이유가 궁금해요!

객체 생성자 실행되기 전에 객체가 검증이 되는 흐름이 맞을거 같아 정적팩터리 메서드를 만들어서 정적팩터리에서 검증하였습니다.

kkt219a · 2025-02-18T09:49:36Z

src/main/java/kitchenpos/products/tobe/domain/ProductPrice.java

+
+    private static void validate(BigDecimal price) {
+        if (price == null || price.compareTo(BigDecimal.ZERO) < 0) {
+            throw new IllegalArgumentException("가격은 0 이상이어야 합니다.");


예외도 커스텀화 해보면 어떨까요?

kkt219a · 2025-02-18T09:49:58Z

src/test/java/kitchenpos/products/tobe/ProductServiceTest.java

+import org.junit.jupiter.params.provider.NullSource;
+import org.junit.jupiter.params.provider.ValueSource;
+
+class ProductServiceTest {


도메인 객체들에 대한 테스트를 작성해보면 좋을 것 같습니다!

kkt219a

피드백 잘 반영해주셨습니다!
몇가지 추가로 코멘트를 남겼는데 확인 부탁드릴게요!

kkt219a · 2025-02-19T10:39:00Z

src/main/java/kitchenpos/products/tobe/domain/ProductPrice.java

+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (o == null || getClass() != o.getClass()) {
+            return false;
+        }
+        ProductPrice that = (ProductPrice) o;
+        return Objects.equals(price, that.price);
+    }
+
+    @Override
+    public int hashCode() {
+        return Objects.hashCode(price);
+    }


ProductPrice의 동등성을 보장해주셨군요 👍🏻
ProductName도 같은 맥락이라면 동등성을 보장해줘야 할 것 같아요!

kkt219a · 2025-02-19T10:46:23Z

src/main/java/kitchenpos/products/tobe/domain/Product.java

+    public void changePrice(BigDecimal price) {
+        this.price = ProductPrice.from(price);
+    }


만약 추후 price가 BigDecimal이 아닌 String도 지원해야한다면,
상품의 가격 변경을 오버로딩해야할 것 같아요 :)
그래서 상품이 가격을 생성할 책임까지 가져갈 필요가 있을까라고 생각되는데 어떻게 생각하실까요?

kkt219a · 2025-02-19T10:48:25Z

src/test/java/kitchenpos/products/tobe/domain/ProductNameTest.java

+
+    @BeforeEach
+    void setUp() {
+        purgomalumClient = Mockito.mock(PurgomalumClient.class);


ProductService에서 Fake를 사용하셨던데, Mock을 사용하신 건 다른 의도가 있으실까요?

kkt219a · 2025-02-19T10:50:02Z

src/test/java/kitchenpos/products/tobe/domain/ProductNameTest.java

+    @Test
+    @DisplayName("상품명이 null일 때 ProductNameRequiredException이 발생해야 한다.")
+    void testFrom_NullName_ThrowsProductNameRequiredException() {
+        // Arrange
+        String name = null;
+
+        // Act & Assert
+        assertThrows(ProductNameRequiredException.class, () -> {
+            ProductName.from(name, purgomalumClient);
+        });
+    }
+
+    @Test
+    @DisplayName("상품명이 비어 있을 때 ProductNameRequiredException이 발생해야 한다.")
+    void testFrom_EmptyName_ThrowsProductNameRequiredException() {
+        // Arrange
+        String name = "";
+
+        // Act & Assert
+        assertThrows(ProductNameRequiredException.class, () -> {
+            ProductName.from(name, purgomalumClient);
+        });
+    }


@ParameterizedTest와 @NullAndEmptySource를 활용해보면 어떨까요? :)

kkt219a · 2025-02-19T10:51:49Z

src/test/java/kitchenpos/products/tobe/domain/ProductNameTest.java

+    }
+
+    @Test
+    @DisplayName("상품명에 비속어가 포함되어 있을 때 ProfanityException이 발생해야 한다.")


지금처럼 작성해주신 것도 좋지만,
작성하셨던 요구사항이나 모델링을 기반으로 간단하게 표현해보면 어떨까요? :)

상품명은 비속어를 포함할 수 없다.

kkt219a · 2025-02-19T10:54:50Z

src/main/java/kitchenpos/products/tobe/domain/Product.java

+
+@Table(name = "product")
+@Entity
+public class Product {


Product에 대한 테스트가 빠진 것 같아요!

jwoo-o added 3 commits February 17, 2025 23:15

🚀 1단계 - 리팩터링(상품)

d0ec2cb

doc : 요구사항 작성

feat : 요구사항 기준으로 리펙토링 진행

e6cc1f6

test : 리펙토링 된 서비스 테스트 작성

0021fa2

kkt219a requested changes Feb 18, 2025

View reviewed changes

jwoo-o added 4 commits February 18, 2025 20:10

refect : PurgomalumClient domain영역으로 이동

48bdba2

feat : 커스터 예외 생성

2d2f2ff

test : 도메인 객체 테스트 작성

7039a93

refect : 메서드명 동일하도록 변경

500063d

kkt219a requested changes Feb 19, 2025

View reviewed changes

		@@ -0,0 +1,5 @@
		package kitchenpos.products.tobe.infra;

		public interface PurgomalumClient {

Conversation

jwoo-o commented Feb 17, 2025

Uh oh!

kkt219a left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kkt219a left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants