[레거시 코드 리팩터링 - 2단계] 오찌(오지훈) 미션 제출합니다. #301

Ohzzi · 2022-10-28T10:53:15Z

안녕하세요 디우... 미션이... 너무... 힘드네요...
아직도 리팩토링할게 보입니다... (머지는 하지 말아주세요...)

리뷰 잘 부탁드립니다...

tco0427

안녕하세요 오찌!!
바쁘신 와중에도 2단계 미션 진행하시느라 고생 많으셨습니다!!
특히 Mapper 사용해주신 부분이 인상깊었어요!!
궁금한 점 포함해서 리뷰를 남기다 보니..코멘트가 조금 많아진 것 같긴 하네요ㅠ.ㅠ
남은 기간 화이팅하세요~!!

tco0427 · 2022-10-30T11:52:30Z

src/main/java/kitchenpos/dto/mapper/OrderDtoMapperImpl.java

+
+    @Override
+    public OrderResponse toOrderResponse(final Order order) {
+        List<OrderLineItemResponse> orderLineItemRespons = order.getOrderLineItems()


Suggested change

List<OrderLineItemResponse> orderLineItemRespons = order.getOrderLineItems()

List<OrderLineItemResponse> orderLineItemResponses = order.getOrderLineItems()

tco0427 · 2022-10-30T11:55:27Z

src/main/java/kitchenpos/dto/mapper/OrderTableDtoMapper.java

+import org.mapstruct.Mapper;
+import org.mapstruct.Mapping;
+
+@Mapper(componentModel = "spring")


찾아보니 Entity와 DTO 간의 매핑을 지원하는 라이브러리인 MapStruct 를 사용해주셨더라구요!!
그런데 혹시 Entity -> DTO, DTO -> Entity 와 같은 부분을 이렇게 별도의 Mapper 를 사용해주신 이유가 궁금합니다..!
저의 경우에는 Service에서 DTO -> Entity 를 수행하고, DTO에서 Entity를 받아 Entity -> DTO 를 수행해주고 있는데요, 현재 방법의 경우 너무 많은 Mapper 에 대한 관리가 힘들지 않을까 하는 의문이 들어서, 오찌의 생각을 들어보고 싶습니다!!

예를 들어 MenuGroup의 create() 도 결국 풀어서 보면 다음과 같을 것 같고

@Transactional public MenuGroupResponse create(final MenuGroupCreateRequest menuGroupCreateRequest) { final MenuGroup entity = menuGroupMapper.toMenuGroup(menuGroupCreateRequest); MenuGroup menuGroup = menuGroupRepository.save(entity); return menuGroupDtoMapper.toMenuGroupResponse(menuGroup); }

Mapper 없이 사용한 방식인 아래와 서비스 단에서의 큰 차이는 없을 것 같아서요..!!

@Transactional public MenuGroupResponse create(final MenuGroupRequest request) { final MenuGroup menuGroup = new MenuGroup(request.getName()); final MenuGroup savedMenuGroup = menuGroupRepository.save(menuGroup); return new MenuGroupResponse(savedMenuGroup); }

추가적으로 Service에서 Mapper에 대한 의존도 없앨 수 있을 것 같아요!

Mapper를 사용하지 않았을 때 Entity <-> DTO 매핑을 하는 로직을 두 가지로 관리해 볼 수 있을 것 같았어요

서비스 내에 DTO 변환 메서드를 만든다

DTO에 정적 팩터리 메서드와 toEntity 메서드를 만든다

1번 같은 경우 사실 Entity <-> DTO 변환 로직이 서비스에서 필요하기는 하나 서비스가 담당해야 할 책임은 아니라고 생각하고, 서비스의 코드가 너무 비대해진다는 생각이 들었습니다.

2번의 경우, DTO가 팩토리의 역할을 하게 된다는 점에서 가능하기는 하지만 썩 좋아보이는 방법이 아니라는 피드백을 제이슨과 호돌맨으로부터 들었기 때문에... 아예 DTO 변환 역할만 하는 객체로 책임을 이관하는 것이 좋겠다는 생각이 들었습니다!

오찌의 생각을 공유해주셔서 감사합니다~_~

tco0427 · 2022-10-30T12:00:23Z

src/main/java/kitchenpos/domain/repository/OrderTableRepository.java

+
+    List<OrderTable> findAllByIdIn(List<Long> ids);
+
+    @Query("select ot from OrderTable ot where ot.tableGroup.id = :tableGroupId")


여기서만 JPQL 을 작성해주신 이유가 궁금해요..!!
이렇게 작성해주지 않았을 때와 쿼리 메소드 방법을 사용했을 때 어떤 차이가 있나요??ㅇㅅㅇ
[Spring Data JPA] findByXXXId 는 불필요한 join을 유발한다 이기 때문일까요??

내 그 이유가 맞습니다 :)

tco0427 · 2022-10-30T12:05:02Z

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

+    private final MenuGroupDtoMapper menuGroupDtoMapper;
+    private final MenuGroupRepository menuGroupRepository;
+
+    public MenuGroupService(final MenuGroupMapper menuGroupMapper, final MenuGroupDtoMapper menuGroupDtoMapper,


이러한 메시지가 뜨네요..ㅠ

빌드 버튼을 한 번 누르면 Mapstruct가 구현체를 생성해줍니다 :)

tco0427 · 2022-10-30T12:08:59Z

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

+    private MenuProduct createMenuProduct(final MenuProductCreateRequest menuProductCreateRequest) {
+        Product product = productRepository.findById(menuProductCreateRequest.getProductId())
+                .orElseThrow(IllegalArgumentException::new);
+        return new MenuProduct(null, null, product.getId(), menuProductCreateRequest.getQuantity(), product.getPrice());


null을 주기보다는 생성자 오버로딩등을 통해서 productId 와 quantity와 price, 필요한 내용만 받는 생성자를 하나 만들면 어떨까요???

tco0427 · 2022-10-30T13:00:52Z

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

+        Long menuGroupId = menuGroupRepository.save(메뉴_그룹을_생성한다("메뉴 그룹"))
                .getId();
-        Long productId = productDao.save(상품을_생성한다("상품", new BigDecimal(1_000)))
+        Long productId = productRepository.save(상품을_생성한다("상품", BigDecimal.valueOf(1_000)))
                .getId();
-        MenuProduct menuProduct = 메뉴_상품을_생성한다(null, productId, 1);
-        Menu menu = 메뉴를_생성한다("메뉴", new BigDecimal(2_000), menuGroupId, List.of(menuProduct));
+        MenuProductCreateRequest menuProductRequest = new MenuProductCreateRequest(productId, 1);
+        MenuCreateRequest menuRequest = new MenuCreateRequest("메뉴", BigDecimal.valueOf(1_001), menuGroupId,
+                List.of(menuProductRequest));

-        assertThatThrownBy(() -> menuService.create(menu)).isInstanceOf(IllegalArgumentException.class);
+        assertThatThrownBy(() -> menuService.create(menuRequest)).isInstanceOf(IllegalArgumentException.class);
    }


해당 테스트도 단위 테스트로 옮겨볼 수 있을 것 같아요!!

tco0427 · 2022-10-30T13:03:43Z

src/test/java/kitchenpos/ui/ProductRestControllerTest.java

+import org.springframework.http.MediaType;
+import org.springframework.test.web.servlet.MvcResult;
+
+class ProductRestControllerTest extends RestControllerTest {


mockMvc 를 이용한 Controller 테스트를 작성해주셨군요!!
개인적으로 테스트 네이밍만 봤을 때는 해당 테스트들이 인수 테스트로 작성되는 것이 보다 적절한 것 같아요!!
(저의 경우에는 예외 케이스들에 대해서 400응답이나 401 등등의 응답이 잘 나가는지를 테스트하기 위해 mcokMvc 를 이용한 Controller 테스트를 작성하고, 저희가 제공하려고 하는 정상 케이스들에 대한 시나리오는 인수 테스트로 제공하려고 합니다!!)
오찌는 어떤 기준으로 테스트를 작성하시는지 궁금해요!!

이 부분은 초반에 컨트롤러 레이어의 테스트를 아예 작성하지 않았다 보니, 정상적으로 응답이 나가는지를 판단하기가 어려워서 최소한의 테스트만 짠 부분입니다. 저도 개인적으로는 인수테스트를 좋아하는데요, 이번 미션은 아무래도 시간적 여유가 많지를 않아서... 컨트롤러 슬라이스 테스트로 우선은 대체해보았습니다.

넵!! 알겠습니다😃

tco0427 · 2022-10-30T13:04:55Z

src/test/java/kitchenpos/domain/repository/MenuRepositoryTest.java

-        Menu savedMenu = menuDao.save(menu);
+        Menu savedMenu = menuRepository.save(menu);


오호..Spring Data JPA 가 제공하는 기본 메소드들(?)에 대해서도 테스트를 작성해주신 것이 흥미로운데요!! 이유를 듣고 싶어요~_~

유지해두면 인터페이스가 Data JPA가 아닌 다른 기능으로 바뀌더라도 손쉽게 바꿀 수 있지 않을까요?
그리고 연관관계 같은 JPA 기능을 테스트 할 때도 좋을 것 같습니다.

인터페이스를 유지한채로 Spring Data JPA 가 아닌 다른 구현체로 바뀌었을 때에도 해당 테스트를 통해서 기존의 데이터 접근을 동일하게 지원하는지 확인하겠다로 이해할 수 있겠네요!

tco0427 · 2022-10-30T13:08:00Z

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

-        OrderTable orderTable1 = tableService.create(주문_테이블을_생성한다(null, 1, true));
-        OrderTable orderTable2 = tableService.create(주문_테이블을_생성한다(null, 0, true));
-        orderTable1.setTableGroupId(
-                tableGroupDao.save(단체_지정을_생성한다(LocalDateTime.now(), List.of(orderTable1, orderTable2))).getId());
-        orderTableDao.save(orderTable1);
+        OrderTable orderTable1 = orderTableRepository.save(주문_테이블을_생성한다(null, 1, true));
+        OrderTable orderTable2 = orderTableRepository.save(주문_테이블을_생성한다(null, 0, true));
+        tableGroupRepository.save(단체_지정을_생성한다(LocalDateTime.now(), List.of(orderTable1, orderTable2)));

-        assertThatThrownBy(() -> tableService.changeEmpty(orderTable1.getId(), orderTable1))
+        assertThatThrownBy(() -> tableService.changeEmpty(orderTable1.getId(), false))


단위 테스트로 이동 가능할 것 같아요!!

tco0427 · 2022-10-30T13:08:24Z

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

+        Long orderTableId = tableService.create(new OrderTableCreateRequest(1, false))
+                .getId();
+        Long menuGroupId = menuGroupRepository.save(메뉴_그룹을_생성한다("메뉴 그룹"))
+                .getId();
+        Long menuId = menuRepository.save(메뉴를_생성한다("메뉴", BigDecimal.ZERO, menuGroupId, List.of()))
+                .getId();
+        OrderLineItem orderLineItem = 주문_항목을_생성한다(null, menuId, 1);
+        orderRepository.save(주문을_생성한다(orderTableId, COOKING, LocalDateTime.now(), List.of(orderLineItem)));
+
+        assertThatThrownBy(() -> tableService.changeEmpty(orderTableId, true))


이 테스트도 단위테스트로 검증이 가능해질 수 있을 것 같습니다!!

tco0427

안녕하세요 오찌!!
2단계 고생 많으셨습니다!
오찌와 이야기하며 프로젝트에 적용했던 내용도 되돌아볼 수 있어 좋은 시간이 되었네요~_~
3단계도 화이팅!!

Ohzzi added 30 commits October 25, 2022 13:21

refactor: EOF 없는 문제 해결

7476db6

refactor: BigDecimal 생성 방식 변경

a72ffa1

refactor: MenuGroup JPA 마이그레이션

bb4f66e

refactor: MenuProduct JPA 마이그레이션

5e6d15e

refactor: Menu JPA 마이그레이션

f5b857f

refactor: MenuGroup의 컨트롤러에서 DTO 사용하도록 수정

2bcfd83

refactor: Menu의 컨트롤러에서 Menu와 MenuProduct를 DTO 사용하도록 수정

f6acb31

refactor: 가격 정보를 VO로 분리

d9b1ce3

refactor: Price JPA 마이그레이션

6a7e0b6

refactor: 엔티티와 DTO 매퍼 분리

003fa25

refactor: 매퍼 패키지 분리

0db4ceb

refactor: 공백 조정

002c4c4

refactor: mapper 메서드 이름 수정

aff83a8

refactor: Product JPA 마이그레이션

6492c55

refactor: OrderTable, TableGroup JPA 마이그레이션

fe65f28

refactor: Table의 컨트롤러에서 DTO 사용하도록 수정

13c0433

refactor: TableGroup의 컨트롤러에서 DTO 사용하도록 수정

060a121

refactor: Order, OrderLineItem JPA 마이그레이션

3761a0b

refactor: MenuGroup의 DTO 사용 범위를 서비스까지 허용

160a220

refactor: Menu의 DTO 사용 범위를 서비스까지 허용

1cff8c0

refactor: 가격이 더 비싼지 판정하는 로직 구현

3491734

refactor: MenuProduct에 price 추가

4d76386

refactor: Menu - MenuProduct 매핑 순서 변경

1f21ca4

refactor: Menu의 서비스 로직을 도메인으로 이동

befaebf

refactor: Product의 불필요한 서비스 로직 제거

f597e5d

refactor: dto 패키지 분리

0fc0e97

refactor: OrderTable의 DTO 사용 범위를 서비스까지 확장

a3a986e

refactor: Table의 서비스 로직을 도메인으로 이동

4413764

refactor: 상품 가격의 생성 위치 변경

f29df2e

refactor: TableGroup의 DTO 사용 범위를 서비스까지 확장

71a07ce

Ohzzi added 3 commits October 28, 2022 18:22

refactor: TableGroup의 서비스 로직을 도메인으로 이동

19d8196

refactor: Order와 Product DTO 사용

62af1f4

refactor: Order의 서비스 로직을 도메인으로 이동

71ef6e8

Ohzzi requested a review from tco0427 October 28, 2022 10:53

Ohzzi added 5 commits October 29, 2022 17:16

refactor: DTO 정리

5a9ba45

refactor: TableGroup DTO 스펙에 맞게 수정

5f7a8d8

refactor: OrderService 내부 메서드 추출

02f536d

refactor: OrderStatus enum화

cc9fd77

refactor: 그룹 지정 해제 로직 Order 안으로 이동

949c1d5

tco0427 suggested changes Oct 30, 2022

View reviewed changes

Ohzzi added 6 commits October 31, 2022 16:21

refactor: ungroup 로직 TableGroup 안으로 이동

2f3ff24

refactor: Order 생성 시 검증 로직 메서드 분리

5439e08

test: 중복 테스트 제거

1307b8e

refactor: 검증 로직 눈에 잘 들어오게 메서드 분리

911816f

refactor: 패키지 구조 정리

55baf46

refactor: mapstruct 제거

4b830f1

tco0427 approved these changes Nov 1, 2022

View reviewed changes

tco0427 merged commit 90662ca into woowacourse:ohzzi Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[레거시 코드 리팩터링 - 2단계] 오찌(오지훈) 미션 제출합니다. #301

[레거시 코드 리팩터링 - 2단계] 오찌(오지훈) 미션 제출합니다. #301

Ohzzi commented Oct 28, 2022

tco0427 left a comment

tco0427 Oct 30, 2022

tco0427 Oct 30, 2022

Ohzzi Oct 31, 2022

tco0427 Oct 31, 2022

tco0427 Oct 30, 2022

Ohzzi Oct 31, 2022

tco0427 Oct 30, 2022

Ohzzi Oct 31, 2022

tco0427 Oct 30, 2022

tco0427 Oct 30, 2022

tco0427 Oct 30, 2022

Ohzzi Oct 31, 2022

tco0427 Nov 1, 2022

tco0427 Oct 30, 2022

Ohzzi Oct 31, 2022 •

edited

Loading

tco0427 Nov 1, 2022

tco0427 Oct 30, 2022

tco0427 Oct 30, 2022

tco0427 left a comment

	List<OrderLineItemResponse> orderLineItemRespons = order.getOrderLineItems()
	List<OrderLineItemResponse> orderLineItemResponses = order.getOrderLineItems()


		List<OrderTable> findAllByIdIn(List<Long> ids);

		@Query("select ot from OrderTable ot where ot.tableGroup.id = :tableGroupId")

		Menu savedMenu = menuDao.save(menu);
		Menu savedMenu = menuRepository.save(menu);

[레거시 코드 리팩터링 - 2단계] 오찌(오지훈) 미션 제출합니다. #301

[레거시 코드 리팩터링 - 2단계] 오찌(오지훈) 미션 제출합니다. #301

Conversation

Ohzzi commented Oct 28, 2022

tco0427 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

Ohzzi Oct 31, 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

tco0427 left a comment

Choose a reason for hiding this comment

Ohzzi Oct 31, 2022 •

edited

Loading