[레거시 리팩터링 - 2단계] 도기(김동호) 미션 제출합니다. #561

kdkdhoho · 2023-10-18T14:58:59Z

안녕하세요 홍실! 2단계 진행해보았습니다.
여러 변경 사항들이 있어 거두절미하고 말씀드리겠습니다.

변경 사항

컨트롤러 테스트를 삭제하고 서비스 테스트를 통합 테스트로 변경했습니다.
제한된 시간 내에 리팩터링 미션을 모두 잘 수행하기 위해선 테스트 코드의 선택과 집중이 필요하다고 판단했습니다.
그리고 프로덕션 코드가 계속해서 변경되는 상황에서 mocking은 비효율적이고 테스트로서 가치가 없다고 판단했기 때문입니다.
JPA 적용
JPA를 잘 사용하진 못하지만 JdbcTemplate으로만 객체 참조를 관리하는 것은 매우 많은 리소스가 소요될 것으로 판단되어 시도해봤습니다.

다만 하나 아쉬운 점은 JPA를 적용한 이후에 제이슨이 Spring Data Jdbc 사용하면 재밌을 것이라는 말을 들었는데요.
남은 미션 기간을 고려했을 때, 여유가 있다면 약간의 DDD 개념(애그리거트)을 접목시켜서 다시 구현해보고 싶네요.

약간은 높아진 도메인 이해도를 바탕으로 요구사항을 다시 작성해봤습니다.

리뷰하실 때 참고해주세요!

이번 단계를 거의 나흘동안 보다 보니 고착 상태에 빠진 것 같아요.. 게다가 비슷한 작업의 반복이다 보니 더 심하게 빠진 것 같네요 🥲
리팩터링 진행하면서 커밋 단위를 잘게 나누기 힘들더라구요.. 🥲

그럼 이번에도 잘 부탁드립니다 💪

hong-sile

안녕하세요!! 도기 리뷰가 너무 늦었네요.. 죄송합니다.
이젠 바쁜 것들이 사라져서 빠르게 리뷰해드리겠습니다.

전반적으로 잘 구현해주신 것 같아요. 사이즈가 좀 많이 큰데 대단합니다.

제가 궁금한 부분에 대해서 리뷰 남겼어요! 꼭 반영하실 필요는 없고, 읽어보시고 적당하다 싶으면 반영해주시면 될 것 같아요.

레벨 4 너무 수고하셨습니다!

src/main/java/kitchenpos/dao/JdbcTemplateMenuDao.java

README.md

src/main/java/kitchenpos/dao/jpa/JpaOrderRepository.java

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

hong-sile · 2023-10-22T04:06:32Z

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

-            sum = sum.add(product.getPrice().multiply(BigDecimal.valueOf(menuProduct.getQuantity())));
+        for (MenuProductCreateRequest menuProductCreateRequest : request.menuProducts()) {
+            Product product = productRepository.getById(menuProductCreateRequest.productId());
+            new MenuProduct(menu, product, menuProductCreateRequest.quantity());


이건 궁금한 점입니다.

여기서 menuProduct를 생성만하는데 이렇게만 해도 JPA가 쿼리를 쏘나요?

일단 도기가 짜준 테스트코드에선 insert 쿼리를 쏘지 않는 것 같아서요

엇.. 저장되는 줄 알고 있었는데 확인해보니 안되고 있었네요..
JPA에게 용빼는 재주가 있는 줄 알았는데 🤣

ㅋㅋㅋㅋ 객체 생성만하면 비영속 상태라 저장이 되지 않네요.
이번 미션을 통해서 JPA를 리마인드 하는 것도 좋겠네요

MenuProduct를 생성자를 통해 생성할 때, Menu가 참조하는 List<MenuProduct>에 add 해줌으로써 Menu와 함께 영속 상태로 해주길 기대했는데 안되나보군요 !

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

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

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

hong-sile · 2023-10-22T04:20:21Z

src/test/java/kitchenpos/application/TableServiceTest.java

+    @Autowired
+    private JpaOrderTableRepository orderTableRepository;
+    @Autowired
+    private JpaOrderRepository orderRepository;


제가 미션하면서 고민된 부분이 있어서 이번 리뷰를 하면서 도기에게 하나 여쭤보려고 해요.

통합테스트를 작성하다보면, 어쩔수 없이 값 세팅을 위하여(givne을 만족하기 위해) 다른 객체들을 미리 저장하는 상황이 생겨요.

이 때 repository로 값을 저장하는 방법과 service로 값을 저장하는 방법이 있을 것 같아요.

도기는 Repository로 미리 값을 세팅해두셨는데 그렇게 한 이유를 여쭤볼 수 있을까요?

개인적으로 service를 통해서 값을 저장해야 하는 이유가 있다면, service를 통해 저장하고 그렇지 않으면 repository를 통해 값을 세팅합니다.
아무래도 service 메서드를 호출하면, 해당 메서드를 올바르게 사용하는 추가적인 인지 비용이 듦과 동시에 해당 메서드에 문제가 생기면 테스트에 영향이 가기 때문에 최대한 지양하는 게 좋다고 생각합니다.

일단 저는 service를 이용해서 값을 넣도록 구현하였는데요.(이게 정답이란 뜻은 아닙니다. 저도 뭐가 답인지 모르겠어요)

추가적인 인지비용이 드는게 엄청 크긴 해요. 진짜 짜다보니까 프로덕션 코드를 이해하지 못하면 테스트를 못 짜겠더라고요.
하지만, 반대로 그런 강제성이 있어서 오류가 생기는 부분이 적다고 생각하기도 합니다.

Repository로 저장하다가 실제로, 유효성검사가 되지 않은 엔티티가 저장되기도 한다든지? 이런 문제도 있을수 있다고 생각하거든요.
아무리 도메인으로 객체의 validation을 밀어넣는다고 하더라도 service에서 검증되는 부분도 있으니까요.
(저는 domain 객체에서 모든 validation을 한다는 것이 불가능하다고 생각합니다.)

뭐가 맞는지는 모르곘네요. 그냥 궁금해서 여쭤봤고 이건 제 의견입니다.

도메인으로 객체의 validation을 밀어넣는다고 하더라도 service에서 검증되는 부분도 있으니까요.

만약 service에서 유효성 검증이 이뤄진다고 하면, 아무래도 유효하지 않은 데이터가 세팅될 가능성이 있다는 점 동의합니다.
하지만 이번 미션, 지금 제 코드 기준으로는 service에서 도메인 유효성 검증이 이뤄지는 부분은 없기에 더욱이 Repository를 통해 값을 세팅한다고 말씀드린 것 같아요!

늘 그렇듯, 트레이드 오프가 존재하는 문제네요.

kdkdhoho · 2023-10-24T02:05:52Z

안녕하세요 홍실!
리뷰 반영해보았습니다. 모두 제가 놓쳤던 부분이거나 실수한 부분을 짚어주셔서 놀랐어요 😮
꼼꼼하고 정말 열심히 리뷰해주셔서 감사합니다.

이번 리뷰 반영하면서 한 가지 질문이 생겼어요.

VO의 기본생성자 처리

Price VO를 적용할 때, value 필드에 final 키워드를 붙이고 싶었어요.
그런데 JPA를 사용하다보니 기본 생성자를 꼭 만들어줘야 했습니다.
그런데 기본 생성자가 호출되면 final 키워드가 붙은 value는 어떻게 처리해줄까 고민이 되었습니다.
처음엔 예외를 터트리는 방식을 적용했지만, JPA가 리플렉션을 통해 기본 생성자를 호출하는 과정에서 예외가 터져 Price 객체 사용이 불가했습니다.
그래서 final을 빼고 기본 생성자를 비어보았습니다. 근데 이건 결국 암시적으로 value에 null이 할당된다는 말인데, 이럴 바엔 명시적으로 final 키워드도 붙여주고 value = null; 을 선언했습니다.
대신 외부에서 호출하는 걸 경고하기 위한 목적으로 @deprecated를 달아줬습니다.

그래도 아직 같은 domain 패키지 내에서는 Price의 기본 생성자 호출이 가능하고, 이로 인해 예상치 못한 문제가 발생할 수 있습니다.

만약 홍실이라면 어떻게 처리하실건가요?

hong-sile

수고하셨습니다! 도기 전체적으로 요구사항은 다 만족한 것 같아서 approve하겠습니다.

VO에 대한 부분은 따로 해당 부분에 코멘트 남겼어요!!!

step3에서 뵈요!!

hong-sile · 2023-10-24T07:12:39Z

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

+    @Deprecated
+    protected Price() {
+        value = null;
+    }
+


저는 Deprecated 좋은 것 같아요. IDE에서 명시적으로 보여지기도하고, 의미도 나타내니까요.
(저는 이런 아이디어 못 떠올렸는데 엄청 좋은데요?)
VO를 쓰기 위해서 불변임을 보장하고 싶긴한데, 이게 JPA 때문에 침범하는게 고민이긴 하거든요.

저는 final 붙여보려다가 잘 안되서 포기했었습니다.. ㅋㅋㅋㅋ 붙이는 방법이 아예 없는건 아닌데 너무 귀찮더라고요.
한 번 Embedabble이 아닌 converter를 찾아보면 좋을 것 같네요.

저는 필드는 가변으로 두고 setter를 풀어두긴 했었습니다.
그리고 getter로 필드를 접근할 때 null을 체크하는 validation 로직을 넣을 것 같아요.(null이면 Exception을 반환하게 하는거죠)

그리고, 내부에서 value를 사용할 때 필드 접근을 하는게 아니라 무조건 메서드로 접근하게 구현하는거죠.

이런 방법도 있긴 한데 저는 Deprecated 도 너무 좋은 것 같습니다. 어차피 protected로 막혀있어서 쓸 상황 자체가 한정적이고, Deprecated면 코드 짤 때 명시적으로 보이니까요.
상속을 하면 Deprecated 생성자가 명시적으로 보일진 모르겠는데, VO를 상속하는 사람은...없을거라 생각해서 도기가 고민하고 사용하신 방법만으로도 충분한 것 같습니다.

덧붙여 이전에 찾아봤던 블로그 글인데 한 번 보면 좋을 것 같아요.

참고자료

그리고 getter로 필드를 접근할 때 null을 체크하는 validation 로직을 넣을 것 같아요.(null이면 Exception을 반환하게 하는거죠)
그리고, 내부에서 value를 사용할 때 필드 접근을 하는게 아니라 무조건 메서드로 접근하게 구현하는거죠.

오 이런 방법도 있겠군요! 근데 이 방법도 getter로 접근을 하지 않으면 생기는 문제는 존재하게 되겠네요ㅜ
자료 공유 너무너무 감사합니다 홍실 !!

hong-sile · 2023-10-24T07:16:55Z

src/main/java/kitchenpos/dto/request/MenuGroupCreateRequest.java

+
+public class MenuGroupCreateRequest {
+
+    private final String name;


dto를 통해 생성해주는게 좋네요.
하지만 spring에서 jackson이 json 데이터를 객체로 변환할 때, 필드가 하나인 dto로 변환하면 에러가 터져요.

실제로 .http 파일로 돌려보니 오류가 발생하네요.

어떤 방법으로 해결할 수 있을까요?

hong-sile · 2023-10-24T07:25:19Z

README.md

@@ -1,94 +1,80 @@
 # 키친포스

-## 요구 사항
+## 용어 사전


전체적인 내용이라서 readme에 남길게요.

그 이번 리뷰하면서 .http파일로 실제 요청을 쏴봤는데, 값이 저장이 안되는 건지 아니면 불러오는 데 이상한건지
다 빈값이 날라오더라고요.

작게는 아마 request에서 값을 제대로 못읽어서 제대로 저장이 안되는 것 같은데,

한번 확인해보시고 수정해주세요.

hong-sile · 2023-10-24T07:26:10Z

src/main/java/kitchenpos/application/TableGroupService.java

-
-        if (orderTables.size() != savedOrderTables.size()) {
-            throw new IllegalArgumentException();
+        if (orderTableIds.size() != orderTables.size()) {


hong-sile · 2023-10-24T07:27:25Z

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

+        for (MenuProductCreateRequest menuProductCreateRequest : request.menuProducts()) {
+            Product product = productRepository.getById(menuProductCreateRequest.productId());
+            MenuProduct menuProduct = new MenuProduct(menu, product, menuProductCreateRequest.quantity());
+            menuProductRepository.save(menuProduct);


이 부분은 필수 반영사항은 아닙니다.

Repository로 명시적으로 save해줄수도 있지만,
menu에서 직접참조로 객체를 들고 있으니 jpa의 cascade 옵션으로도 저장하는 방법도 있겠네요.

hong-sile · 2023-10-24T07:30:39Z

src/test/java/kitchenpos/common/IntegrationTest.java

+
+@SpringBootTest
+@DisplayNameGeneration(value = ReplaceUnderscores.class)
+public abstract class IntegrationTest {


굳이 테이블을 drop & create를 하지 않아도 될 것 같아요.

단순히 truncate만 해주더라도 값을 테스트 격리를 위해 값을 초기화하는데는 아무런 문제가 없을 것 같아요!

hong-sile · 2023-10-24T07:31:03Z

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

    }

-    public void setId(final Long id) {
+    public Order(


👍 잘 지워주셨군요

hong-sile · 2023-10-24T07:37:08Z

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

+        return new Price(amount);
+    }
+
+    public Price add(Price other) {


price에서 add, calculateAmount와 같이 계산을 하는 메서드를 추가한 것이 좋네요

hong-sile · 2023-10-24T07:43:25Z

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

-            assertThatThrownBy(() -> menuService.create(menu))
-                    .isInstanceOf(IllegalArgumentException.class);
+            assertAll(
+                    () -> assertThat(menu.id()).isNotNull(),


값검증을 할 때는 여러 필드를 비교해야하죠..

menuGroup은 usingRecursiveComparsion으로 값을 비교해주는군요

menu 객체가 존재한다면 menu자체도 해당 방법으로 비교해줄수 있을 것 같아요.

어떻게 할 수 있을까요?(질문이 너무 추상적이네요. 이 부분은 이해가 안 될수도 있을 것 같네요. 의도를 모르시겠다 싶으시면 dm 주세요)

kdkdhoho added 9 commits October 16, 2023 11:18

docs: 단계별로 나누어 README 작성

42f5a38

remove: JdbcTemplate Dao 전체 삭제

81616ae

remove: ControllerTest 전체 삭제

dea4013

rename: MockTest 패키지 위치 변경

fbd75d5

chore: JPA 사용을 위한 ddl-auto 속성 추가

1d3b2c7

docs: 조금은 높아진 도메인 지식으로 요구사항 재정의

09629ef

feat: 도메인별 JpaRepository 생성

cd85553

feat: 서비스 리팩터링

8190287

remove: MockTest 어노테이션 제거

49c41f5

kdkdhoho self-assigned this Oct 18, 2023

kdkdhoho requested a review from hong-sile October 18, 2023 14:59

hong-sile requested changes Oct 22, 2023

View reviewed changes

kdkdhoho added 18 commits October 23, 2023 15:46

test: 실패하는 테스트 수정

b337214

style: 상수 아래에 개행 추가

29d0cd0

refactor: 모든 커스텀 예외가 BaseException을 상속받도록 수정

f14ce51

refactor: 파라미터 변수명 수정

d77b58f

feat: 테이블 그룹 생성 시에 존재하지 않는 테이블로 요청을 보냈을 경우의 예외 검증 로직 추가

0bb2a8d

fix: Menu 저장 시에 MenuProduct를 저장하지 않는 현상 수정

fa1687f

rename: IntegrationTest 어노테이션 패키지 위치 변경

e8f56ff

refactor: 잘못 사용중이던 생성자 제거

df60022

feat: Price 값객체 추가

ab918df

refactor: Price VO를 사용하도록 리팩터링

c45e71a

fix: Price 객체에 null 체크 빠진 부분 추가

fb67107

feat: 수량을 나타내는 Quantity 값객체 추가

d953239

refactor: Quantity 객체를 사용하도록 수정

02377fe

fix: Price의 잘못된 생성자 타입 변경

b83f234

test: 성공 케이스 테스트 시에 값 검증을 하도록 수정

9a9ebe0

refactor: 도메인 객체에서 List 타입의 필드에 final 추가

b05beb1

style: 생성자 위치 변경

83d4c3f

refactor: OrderLineItem이 Quantity를 사용하도록 수정

a05a969

hong-sile approved these changes Oct 24, 2023

View reviewed changes

hong-sile merged commit 39d037e into woowacourse:kdkdhoho Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[레거시 리팩터링 - 2단계] 도기(김동호) 미션 제출합니다. #561

[레거시 리팩터링 - 2단계] 도기(김동호) 미션 제출합니다. #561

kdkdhoho commented Oct 18, 2023 •

edited

Loading

hong-sile left a comment

hong-sile Oct 22, 2023

kdkdhoho Oct 23, 2023

hong-sile Oct 24, 2023

kdkdhoho Oct 24, 2023

hong-sile Oct 22, 2023

kdkdhoho Oct 24, 2023

hong-sile Oct 24, 2023

kdkdhoho Oct 24, 2023 •

edited

Loading

kdkdhoho commented Oct 24, 2023 •

edited

Loading

hong-sile left a comment •

edited

Loading

hong-sile Oct 24, 2023

kdkdhoho Oct 24, 2023

hong-sile Oct 24, 2023

hong-sile Oct 24, 2023

hong-sile Oct 24, 2023 •

edited

Loading

hong-sile Oct 24, 2023

hong-sile Oct 24, 2023

hong-sile Oct 24, 2023 •

edited

Loading

hong-sile Oct 24, 2023

hong-sile Oct 24, 2023


		public class MenuGroupCreateRequest {

		private final String name;

[레거시 리팩터링 - 2단계] 도기(김동호) 미션 제출합니다. #561

[레거시 리팩터링 - 2단계] 도기(김동호) 미션 제출합니다. #561

Conversation

kdkdhoho commented Oct 18, 2023 • edited Loading

변경 사항

리뷰하실 때 참고해주세요!

hong-sile 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

kdkdhoho Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

kdkdhoho commented Oct 24, 2023 • edited Loading

VO의 기본생성자 처리

hong-sile left a comment • 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

hong-sile Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hong-sile Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kdkdhoho commented Oct 18, 2023 •

edited

Loading

kdkdhoho Oct 24, 2023 •

edited

Loading

kdkdhoho commented Oct 24, 2023 •

edited

Loading

hong-sile left a comment •

edited

Loading

hong-sile Oct 24, 2023 •

edited

Loading

hong-sile Oct 24, 2023 •

edited

Loading