[레거시 코드 리팩터링 - 2단계] 우르(김현우) 미션 제출합니다. #571

java-saeng · 2023-10-19T08:32:25Z

안녕하세요 아마란스 !!!

이번에 82개의 files changed가 있는데,, 미리 죄송합니다.

많은 이유가 기존에 클래스를 리팩터링 하는 과정에서 컴파일 에러가 너무 나서 테스트하기 힘들더라구요.
그래서 리팩터링 대상 클래스에 postfix로 2를 붙인 다음에, 리팩터링 완료하고 나서 숫자 2를 지웠습니다.

ex) ProductDao2 생성 -> 기존 ProudctDao 제거 -> ProductDao2를 ProductDao 로 수정

이번 미션에서는 JPA를 사용하지 않고 JdbcTemplate을 사용해서 구현해봤습니다.

중간에 매우 많은 파일 삭제하는 커밋이 있는데, 그게 위에서 작성한 부분이라 바로 넘기셔도 좋을 것 같아요

이번 리뷰 잘 부탁드립니다 !!

- Repository, Entity 추가

amaran-th

안녕하세요 우르! 저도 미션 2단계를 다 하지 못한 상황이라…조금 늦게 리뷰를 해드리게 되었네요🥲

이번 미션의 핵심 요구사항이 단위 테스트하기 어려운 코드와 단위 테스트 가능한 코드를 분리하는 것인 만큼, 서비스 코드가 얼마나 잘 분리되어있는지에 초점을 맞춰서 리뷰하였습니다.
클래스별로 수행하는 책임이 명확하고 작은 단위로 잘쪼개져있어서, 코드를 읽는 내내 감탄했습니다👍

몇 가지 신경쓰이는 부분에 대해 코멘트 남겨두었으니, 확인 및 반영&답변 후 재요청 부탁드립니다!

+) 그리고 이전 1단계 미션에서 리뷰에 달아주신 코멘트들도 모두 확인했습니다…!
코멘트가 달리지 않은 것에 대해서 제 쪽에서 먼저 말씀을 드렸어야했는데, ‘나중에 말씀드려야겠다’고 생각해놓고 그대로 잊어버린 것 같아요…🥲
소통이 미흡했던 점 사과드립니다.

amaran-th · 2023-10-21T07:29:45Z

src/main/java/kitchenpos/domain/MenuGroupRepository.java

@@ -0,0 +1,15 @@
+package kitchenpos.domain;


개인적으로, 저희 프로젝트에서 하던 것처럼 repository 패키지를 별도로 만들어주는 것도 좋을 것 같습니다!

수정했습니다 !!!

amaran-th · 2023-10-21T08:14:38Z

src/main/java/kitchenpos/dao/mapper/MenuGroupMapper.java

@@ -0,0 +1,14 @@
+package kitchenpos.dao.mapper;


Mapper를 따로 분리해서 구현해주셨군요...!
저의 경우 entity 클래스가 toDomain() 메서드를 구현하게 했는데, 우르가 구현하신 방식이 좀더 역할이 잘 쪼개진 것 같은 느낌이 드네요👀

amaran-th · 2023-10-21T09:37:34Z

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

-    }
+  private Long id;
+  private String name;
+  private BigDecimal price;


price를 별도의 값객체로 분리해주면 관련 로직을 더 단순하게 만들 수 있을 것 같아요~!

그 방법도 좋은 것 같네요
다만 현재는 domain이 하나의 domain 패키지에 존재할텐데, 만약 도메인이 패키지별로 분리가 된다면 price는 어디에 있는게 좋을까요??

그래서 위와 같은 생각으로 따로 값 객체를 만들지 않았습니다.

common이라는 패키지에는 넣기 너무 애매한 것 같아서요,,

음 말씀을 들어보니 패키지 소속이 애매하긴 하네요,,,
저의 경우엔 패키지를 어그리거트 별로 크게 menu, order, table로 나눠주었는데요,
Product와 Menu가 유사한 라이프 사이클을 갖는다고 생각해서 동일한 패키지에 넣어주었는데, Price는 이 두 객체에서만 사용하니 menu 패키지에 포함시켜주었습니다.

amaran-th · 2023-10-21T09:45:46Z

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

+import java.math.BigDecimal;
+import java.util.List;
+
+public class MenuProducts {


일급 컬렉션 분리 좋습니다👍👍

amaran-th · 2023-10-21T09:52:39Z

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

+  }
+
+  public void changeStatus(final OrderStatus orderStatus) {
+    if (this.orderStatus == OrderStatus.COMPLETION) {


이 로직을 OrderStatus의 메서드로 만들어도 좋을 것 같습니다!

오... 그렇네요 수정했습니다

amaran-th · 2023-10-21T11:12:39Z

src/main/java/kitchenpos/dao/mapper/OrderMapper.java

+    );
+  }
+
+  public static OrderTable mapToOrderTable(final OrderTableEntity entity) {


⚠이 메서드는 OrderTableMapper에 정의되어 있는 메서드와 중복되는 것 같습니다...!

감사합니다 !!

amaran-th · 2023-10-21T11:44:44Z

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

-        return orderTableDao.save(savedOrderTable);
+  private void validateOrderTableNotCompletion(final Long orderTableId) {
+    if (orderRepository.existsByOrderTableIdAndOrderStatusIn(
+        orderTableId, OrderStatus.notCompletion())


👍👍👍

amaran-th · 2023-10-21T11:47:58Z

src/main/java/kitchenpos/domain/OrderTable.java

-        return id;
-    }
+  private Long id;
+  private Long tableGroupId;


앗 여기는 TableGroup 객체로 바꾸지 않으셨네요...!
혹시 특별한 이유가 있으신가요?👀

OrderTable은 TableGroup를 가지는게 필수가 아니기도하고, OrderTable과 TableGroup은 경계가 있는 것 같아서 간접 참조를 했습니다.

아하, 납득했습니다!👍👍

amaran-th · 2023-10-21T12:03:44Z

src/test/java/kitchenpos/application/OrderServiceTest.java

+        OrderLineItemFixture.createOrderLineItem(menu1),
+        OrderLineItemFixture.createOrderLineItem(menu),
+        OrderLineItemFixture.createOrderLineItem(menu3)
+    );


data.sql을 지우고 fixture로 대체해주셨군요!
굿입니다👍👍👍

amaran-th · 2023-10-21T12:11:23Z

src/test/java/kitchenpos/dao/TableGroupRepositoryImplTest.java

+        .usingRecursiveFieldByFieldElementComparator()
+        .containsExactlyInAnyOrderElementsOf(List.of(tableGroup1, tableGroup));
+  }
+}


마땅히 달 만한 곳이 없어서 여기에 답니다!

도메인 클래스에 구현되어 있는 비즈니스 로직에 대한 단위 테스트도 작성해주시면 좋을 것 같아요!
(생성자에 대한 유효성 검증, 일급 컬렉션의 비즈니스 로직 등등)

넵 !!!! 도메인에 꼭 필요한 테스트의 단위 테스트 작성했습니다!

java-saeng

안녕하세요 아마란스 !!

리뷰 감사히 잘 받았습니다. 말씀하신 테스트들 추가했습니다

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

java-saeng · 2023-10-21T13:24:47Z

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

-    }
+  private Long id;
+  private String name;
+  private BigDecimal price;


그 방법도 좋은 것 같네요
다만 현재는 domain이 하나의 domain 패키지에 존재할텐데, 만약 도메인이 패키지별로 분리가 된다면 price는 어디에 있는게 좋을까요??

그래서 위와 같은 생각으로 따로 값 객체를 만들지 않았습니다.

common이라는 패키지에는 넣기 너무 애매한 것 같아서요,,

java-saeng · 2023-10-21T13:28:39Z

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

+  }
+
+  public void changeStatus(final OrderStatus orderStatus) {
+    if (this.orderStatus == OrderStatus.COMPLETION) {


오... 그렇네요 수정했습니다

java-saeng · 2023-10-21T13:31:56Z

src/main/java/kitchenpos/dao/mapper/OrderMapper.java

+    );
+  }
+
+  public static OrderTable mapToOrderTable(final OrderTableEntity entity) {


감사합니다 !!

java-saeng · 2023-10-21T13:32:41Z

src/main/java/kitchenpos/domain/OrderTable.java

-        return id;
-    }
+  private Long id;
+  private Long tableGroupId;


OrderTable은 TableGroup를 가지는게 필수가 아니기도하고, OrderTable과 TableGroup은 경계가 있는 것 같아서 간접 참조를 했습니다.

java-saeng · 2023-10-21T13:33:51Z

src/main/java/kitchenpos/domain/MenuGroupRepository.java

@@ -0,0 +1,15 @@
+package kitchenpos.domain;


수정했습니다 !!!

java-saeng · 2023-10-21T13:42:34Z

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

-    public void setSeq(final Long seq) {
+    public MenuProduct(
+        final Long seq,
+        final Product product,


수정했습니다 !

java-saeng · 2023-10-23T07:30:48Z

src/test/java/kitchenpos/dao/TableGroupRepositoryImplTest.java

+        .usingRecursiveFieldByFieldElementComparator()
+        .containsExactlyInAnyOrderElementsOf(List.of(tableGroup1, tableGroup));
+  }
+}


넵 !!!! 도메인에 꼭 필요한 테스트의 단위 테스트 작성했습니다!

amaran-th

안녕하세요~!
리뷰도 잘 반영해주셨고 요구사항도 충족된 것 같아 이만 approve하도록 하겠습니다!
3단계에서 봬요!

amaran-th · 2023-10-23T11:46:16Z

src/main/java/kitchenpos/domain/OrderTable.java

-        return id;
-    }
+  private Long id;
+  private Long tableGroupId;


아하, 납득했습니다!👍👍

amaran-th · 2023-10-23T12:31:30Z

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

-    }
+  private Long id;
+  private String name;
+  private BigDecimal price;


음 말씀을 들어보니 패키지 소속이 애매하긴 하네요,,,
저의 경우엔 패키지를 어그리거트 별로 크게 menu, order, table로 나눠주었는데요,
Product와 Menu가 유사한 라이프 사이클을 갖는다고 생각해서 동일한 패키지에 넣어주었는데, Price는 이 두 객체에서만 사용하니 menu 패키지에 포함시켜주었습니다.

java-saeng added 30 commits October 18, 2023 17:14

feat : Product 리팩터링하기 위해 Product2 생성

4879c1c

- Repository, Entity 추가

feat : MenuGroup Repository 도입

9eddb5e

feat : Menu Repository 도입

41978e6

feat : MenuProduct Repository 도입

e03c1df

feat : TableGroup Repository 도입

4f18dc9

feat : OrderTable Repository 도입

798f29b

feat : Order Repository 도입

44b1b7b

feat : OrderLineItem Repository 도입

4c0d454

refactor : MenuGroupService에서 Dao -> Repository 변경

3893325

feat : MenuProduct에서 Menu 직접 참조 삭제

3377c4c

feat : Menu를 생성할 때 MenuProduct 도 같이 맵핑하기

c6524e9

refactor : MenuService가 DAO가 아닌 Repository 사용하도록 수정

827c1fe

refactor : ProductService가 DAO가 아닌 Repository 사용하도록 수정

042fc27

refactor : TableService가 DAO가 아닌 Repository 사용하도록 수정

1ea99a4

refactor : OrderService가 DAO가 아닌 Repository 사용하도록 수정

06418c4

refactor : TableGroupService가 DAO가 아닌 Repository 사용하도록 수정

9821391

fix : test 실패하는 것 수정

9664cbd

refactor : 리팩터링 한 xxx2 클래스를 전부 2 떼주기

4abfe31

refactor : OrderService 리팩터링

311417c

refactor : MenuService 리팩터링

0053c46

refactor : TableGroupService 리팩터링

949f69a

refactor : TableService 리팩터링

1df1abb

refactor : ProductRepositoryImpl 리팩터링

529fa64

refactor : ProductRepositoryImpl에서 Entity -> Domain 변환을 mapper에서 진행

e0faf37

refactor : MenuGroupRepositoryImpl 에서 Entity -> Domain 변환을 mapper에서 진행

72e6cad

refactor : MenuRepositoryImpl 에서 Entity -> Domain 변환을 mapper에서 진행

5823e80

refactor : OrderRepositoryImpl 에서 Entity -> Domain 변환을 mapper에서 진행

f18b5e4

refactor : OrderTableRepositoryImpl 에서 Entity -> Domain 변환을 mapper에서 진행

1b52489

refactor : TableGroupRepositoryImpl 에서 Entity -> Domain 변환을 mapper에서 진행

82a2c91

refactor : 유틸리티 클래스에 private 생성자 추가

c016999

amaran-th self-requested a review October 21, 2023 07:07

amaran-th requested changes Oct 21, 2023

View reviewed changes

java-saeng added 12 commits October 21, 2023 22:31

refactor : 완료상태인지 OrderStatus에게 메시지 보내기

3757483

refactor : OrderTableMapper 메서드 중복 제거

e23119a

refactor : repository 패키지 수정

c3c1ff7

docs : http 수정

f90c27c

refactor : 사용하지 않는 생성사 삭제

adfc33e

test : MenuProducts 테스트 추가

73f1865

test : OrderStatus 테스트 추가

6c22507

feat : orderStatus에 this 추가 및 테스트 추가

2dd6bee

feat : OrderTables 테스트 추가

32ea701

fix : TableGroup 생성 시 OrderTable의 tableGroupId 수정

f5aeaf1

fix : Product 맵핑 시 id까지 맵핑

105b341

refactor : 메서드 이름 변경

76296e2

java-saeng commented Oct 23, 2023

View reviewed changes

amaran-th approved these changes Oct 23, 2023

View reviewed changes

amaran-th merged commit 3538f87 into woowacourse:java-saeng Oct 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[레거시 코드 리팩터링 - 2단계] 우르(김현우) 미션 제출합니다. #571

[레거시 코드 리팩터링 - 2단계] 우르(김현우) 미션 제출합니다. #571

java-saeng commented Oct 19, 2023

amaran-th left a comment

amaran-th Oct 21, 2023

java-saeng Oct 21, 2023

amaran-th Oct 21, 2023

amaran-th Oct 21, 2023

java-saeng Oct 21, 2023

amaran-th Oct 23, 2023

amaran-th Oct 21, 2023

amaran-th Oct 21, 2023

java-saeng Oct 21, 2023

amaran-th Oct 21, 2023

java-saeng Oct 21, 2023

amaran-th Oct 21, 2023

amaran-th Oct 21, 2023

java-saeng Oct 21, 2023

amaran-th Oct 23, 2023

amaran-th Oct 21, 2023

amaran-th Oct 21, 2023

java-saeng Oct 23, 2023

java-saeng left a comment

java-saeng Oct 21, 2023

java-saeng Oct 21, 2023

java-saeng Oct 21, 2023

java-saeng Oct 21, 2023

java-saeng Oct 21, 2023

java-saeng Oct 21, 2023

java-saeng Oct 23, 2023

amaran-th left a comment

amaran-th Oct 23, 2023

amaran-th Oct 23, 2023

[레거시 코드 리팩터링 - 2단계] 우르(김현우) 미션 제출합니다. #571

[레거시 코드 리팩터링 - 2단계] 우르(김현우) 미션 제출합니다. #571

Conversation

java-saeng commented Oct 19, 2023

amaran-th 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

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

java-saeng 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

amaran-th left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment