[김대겸] Step2 PR #681

Gyeom · 2022-12-13T14:20:28Z

안녕하세요 리뷰어님,

정말 힘들었던 미션이었습니다 ㅠㅠㅠㅠ

도메인 생성 관련 테스트 코드 작성
서비스 레이어에 있던 비즈니스 로직을 각 도메인에 책임 부여
dao 정상 동작을 위해, dao호출 인터페이스를 매개변수로 넘겨 일단 비즈니스 로직이 정상적으로 동작되도록 유지
DTO 개선 및 테스트 코드 리팩토링 (DTO가 변경되면서 계속 테스트코드 에러발생)
Price, Quantity 등 원시값 포장 클래스 생성 및 테스트 코드 리팩토링
네이밍, 예외처리 등 컨벤션 관련해서 전반적으로 검토
부족한 도메인 테스트 코드 작성

크게는 위와 같은 순서로 코드를 작성하도록 노력했습니다.
한기능의 한개의 커밋으로 개발하고 싶었지만, 다른 클래스에 참조가 되는 부분이 많다보니 많이 헤멨습니다 ㅠㅠ

리팩토링에 대해 정말 많이 느끼고 배울 수 있었던 미션이었습니다.

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

limwoobin

안녕하세요 대겸님
2단계 미션 잘 진행해주셨습니다 👍👍
수정된 코드량도 어마어마하고 정말 고생많으셨어요!

고려해보면 좋을만한 부분들이 있어 코멘트 남겨드렸습니다.
확인하시고 다시 요청부탁드립니다!
추가로 고민되거나 의논하고싶은 부분이 있다면 언제든 편하게 dm주세요 ㅎㅎ

limwoobin · 2022-12-14T13:48:27Z

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

-    public MenuGroup create(final MenuGroup menuGroup) {
-        return menuGroupDao.save(menuGroup);
+    public MenuGroupResponse create(final MenuGroupRequest request) {
+        return MenuGroupResponse.from(menuGroupRepository.save(MenuGroup.of(request.getName())));


라인하나에 너무 많은 로직이 함축되어있네요 ㅎㅎ
중간에 변수로 한번 빼면은 보다 가독성이 좋아지지않을까 싶습니다!

지금 보니 그렇네요 ㅎㅎ 중간에 변수를 한번 빼도록 하겠습니다~!

limwoobin · 2022-12-14T13:53:47Z

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

+    private List<MenuProduct> findAllMenuProductsByProductId(List<MenuProductRequest> menuProductRequests) {
+        return menuProductRequests.stream()
+                .map(menuProductRequest -> menuProductRequest.toMenuProduct(findProductById(menuProductRequest.getProductId())))
+                .collect(Collectors.toList());
+    }

-        return menus;
+    private Product findProductById(final Long id) {
+        return productRepository.findById(id)
+                .orElseThrow(() -> new IllegalArgumentException(id + "에 해당하는 상품을 찾을 수 없습니다."));
    }


productId만 추출해 inquery로 개선해볼 수 있을것같습니다 ㅎㅎ

음 그렇게 되면 toMenuProduct함수를 통해 사용되는 quantity를 사용할 수 있을지... 고민입니다 ㅠㅠ

toMenuProduct는 그대로 quantity를 사용하고
in query로 조회한 데이터의 사이즈와 요청받은 사이즈를 비교해 예외를 처리해보면 어떨까 싶습니다 ㅎㅎ
현재 id를 예외메시지에 같이 나타내고 있어 중간에 몇가지 과정이 더 필요할 수 있겠군요 ㅠ

limwoobin · 2022-12-14T13:55:04Z

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

+        if (Objects.isNull(orderLineItems)) {
+            throw new IllegalArgumentException("요청정보에 주문정보가 존재하지 않습니다.");
        }


빈 컬렉션이면 해당 예외에 안걸리지 않을까요? size로도 같이 체크해보심 좋을것같습니다 ㅎㅎ

이 부분도 수정하도록 하겠습니다~! 감사합니다 :)

limwoobin · 2022-12-14T13:56:19Z

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

-        for (final Order order : orders) {
-            order.setOrderLineItems(orderLineItemDao.findAllByOrderId(order.getId()));
-        }
+    private List<Long> mapToMenuIds(final List<OrderLineItemRequest> orderLineItems) {


id만 추출하는로직으로 보이는데 메소드명이 안맞는것같아요!

좀 더 명확한 의미를 갖고 있는 메소드명을 고민해서 변경하도록 하겠습니다~!

limwoobin · 2022-12-14T13:58:57Z

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

+    public OrderResponse create(final OrderRequest request) {
+        Long orderTableId = request.getOrderTableId();
+        OrderTable orderTable = orderTableRepository.findById(orderTableId)
+                .orElseThrow(() -> new IllegalArgumentException(orderTableId + "에 해당하는 주문테이블을 찾을 수가 없습니다."));


id도 같이 예외메시지에 포함하신거 좋네요 ㅎㅎ
다만 직접스트링으로 선언하는것 보다는 별도의 예외메시지를 관리하는 클래스나 enum을 고려해보시는건 어떨까요??

넵 enum으로 예외메시지 관리할 수 있도록 수정하겠습니다 ~!

limwoobin · 2022-12-14T14:26:14Z

src/main/java/kitchenpos/dto/OrderResponse.java

+public class OrderResponse {
+    private Long id;
+    private final Long orderTableId;
+    private final String orderStatus;


enum을 사용안하고 String으로 사용하신 이유가있나요??

나중에 바꿔야지 하고 미루다가 잊었네요 ㅠㅠ enum 사용할수있도록 수정 하겠습니다~!
감사합니다 :)

limwoobin · 2022-12-14T14:26:43Z

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

-    private Long orderTableId;
+    @ManyToOne(fetch = FetchType.LAZY)
+    @JoinColumn(name = "order_table_id")
+    private OrderTable orderTable;
    private String orderStatus;


enum을 사용하시는게 좋을것같아요!

limwoobin · 2022-12-14T14:27:40Z

src/main/java/kitchenpos/ui/MenuGroupRestController.java

+    public ResponseEntity<MenuGroupResponse> create(@RequestBody final MenuGroupRequest request) {
+        final MenuGroupResponse created = menuGroupService.create(request);


request, response 로 리턴하시는거 좋네요 ㅎㅎ

limwoobin · 2022-12-14T14:28:03Z

src/main/java/kitchenpos/ui/MenuGroupRestController.java

+    public ResponseEntity<MenuGroupResponse> create(@RequestBody final MenuGroupRequest request) {
+        final MenuGroupResponse created = menuGroupService.create(request);


경로는 RequestMapping으로 빼서 사용하시면 보다 깔끔할것같네요
전체적으로도 확인해보시면 좋을것같아요!

@RequestMapping을 통해 분리해보록 하겠습니다~!

limwoobin · 2022-12-14T14:31:47Z

src/test/java/kitchenpos/domain/MenuTest.java

+
+class MenuTest {
+
+    @Test


여기엔 @DisplayName 이 빠져있네요!

실수로 빠뜨렸네요 ㅠㅠ 추가하도록 하겠습니다~!

limwoobin · 2022-12-14T14:38:43Z

리팩토링 순서에 대해 고민하신 방법으로 진행하셧군요 ㅎㅎ 굉장히 좋은것같습니다
저같은 경우는 인수테스트의 보호를 기반으로 리팩토링을 진행했었는데요
리팩토링 대상 메소드에 대해 카피본을 추가로 생성한 이후
카피한 메소드에서 리팩토링을 진행하고 단위테스트, 인수테스트를 카피한 메소드가 동작하게끔 수정하여 정상동작하는지 검증했습니다.
이후 기존 메소드를 fadeout 하는 방식으로 진행했었어요
이렇게 리팩토링했을때 제가 느꼈던 장점은 기존 레거시를 안정적으로 보호할 수 있다는
점이 가장 컸던것 같습니다 :)

Gyeom · 2022-12-15T15:06:00Z

리뷰어님~ 안녕하세요

꼼꼼한 피드백 덕분에 많이 배울 수 있었습니다.
피드백 반영해서 리뷰 요청 드립니다~!

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

limwoobin

안녕하세요 대겸님
2단계 피드백 꼼꼼하게 잘 반영해주셨네요 👍👍
정말 코드량만큼 고생도 많으셧을거라 생각됩니다

몇가지 코멘트를 남겨드렷으니 다음미션 하면서 같이 확인해주심 좋을것같아요
그럼 2단계는 여기서 머지하겠습니다
고생많으셨습니다!

limwoobin · 2022-12-16T07:15:18Z

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

-        for (final Menu menu : menus) {
-            menu.setMenuProducts(menuProductDao.findAllByMenuId(menu.getId()));
-        }
+    private List<Product> toProduct(final List<MenuProductRequest> menuProductRequests) {


findProducts와 같은 이름은 어떨까요?
repository를 통해 조회하는것이라 더 알맞지 않을까 싶습니다 ㅎㅎ

repository를 통해 조회하는것은 findXX 규칙 좋네요 ㅎㅎ
감사합니다~!

limwoobin · 2022-12-16T07:15:36Z

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

+        Map<Long, Product> productIdToProduct = new HashMap<>();
+        for (Product product : products) {
+            productIdToProduct.put(product.getId(), product);
        }


오 map으로 처리하셧군요 ㅎㅎ

넵 복잡도가 N이 초과하지 않는 방법이 어떤게 있을까 고민하다가 방법이 잘 떠오르지 않아 map을 활용했습니다ㅠㅠ

저도 이런경우 map을 활용하고는 합니다
루프를 돌면서 찾기엔 복잡도가 너무올라가서요 ㅎㅎ

limwoobin · 2022-12-16T07:17:32Z

src/main/java/kitchenpos/common/ErrorMessage.java

@@ -0,0 +1,36 @@
+package kitchenpos.common;
+
+public enum ErrorMessage {


예외메시지를 별도로 분리해 상수로 처리하셧네요 ㅎㅎ
보다 깔끔해진것같습니다!

limwoobin · 2022-12-16T07:18:58Z

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

+            orderTable.unGroup();
+            orderTableRepository.save(orderTable);


이미 알고계신 내용일수도 있지만
@transactional 이 끝나면 더티체킹이 발생해 굳이 save를 선언하지 않아도
변경사항이 적용되게 됩니다 ㅎㅎ

불필요한 로직이 있었네요~ 수정하도록 하겠습니다 ㅎㅎ 감사합니다!

명시하는것도 저는 괜찮다고 생각해요!

limwoobin · 2022-12-16T07:19:57Z

src/main/java/kitchenpos/domain/OrderStatus.java

 public enum OrderStatus {
-    COOKING, MEAL, COMPLETION
+    COOKING("조리"), MEAL("식사"), COMPLETION("계산완료");


피드백을 잘 반영해주셧네요 :)

limwoobin · 2022-12-16T07:20:56Z

src/main/resources/application.yml

+
+  jpa:
+    properties:
+      hibernate:


batch size 설정을 넣어주셔도 좋을것같습니다!

이런 방법이 있었군요 ㅎㅎ 좋은 정보 감사합니다 👍

limwoobin · 2022-12-16T07:22:53Z

src/main/java/kitchenpos/dto/MenuGroupRequest.java

+    public MenuGroup toEntity() {
+        return MenuGroup.of(name);
+    }


오 의존성 방향 좋네요 ㅎㅎ
다만 공통으로 쓰이는게 아니니 toEntity보다는 도메인에 대한 네이밍을 활용하는건 어떨지 제안드려봅니다
예를들면 toMenuGroup과 같이요 ㅎㅎ

보다 의미가 명확해질 것 같아요 ㅎㅎㅎ 다른 도메인도 같이 변경하도록 하겠습니다~!

limwoobin · 2022-12-16T07:25:20Z

src/main/java/kitchenpos/dto/OrderResponse.java

+public class OrderResponse {
+    private Long id;
+    private final Long orderTableId;
+    @Enumerated(EnumType.STRING)


해당 어노테이션은 없어도 될것같아요 ㅎㅎ

아이고,, 제가 실수로 넣은 것 같습니다 ㅠㅠㅠ 수정하도록 할게요~! 감사합니다

limwoobin · 2022-12-16T07:27:07Z

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

+    @JoinColumn(name = "table_group_id")
+    private TableGroup tableGroup;
+    @JsonIgnore
+    @BatchSize(size = 5)


아 여기서 선언해주셧군요 ㅎㅎ
다른 OneToMany에서 동일하게 사용하시려면 yml파일에 작성하시는것도
고려해보심 좋습니다!

daegyeom.kim added 30 commits December 11, 2022 11:58

refactor(order): 테스트 검증 로직 위치 변경

3a4a92c

refactor: 서비스 테스트 코드 접근제한자 제거

f11c9e2

refactor: 중복 fixture 리팩토링

d843a0a

docs(readme): step2 서비스 리팩터링 요구사항 작성

b3a831a

refactor(product): 상품 도메인 리팩토링 및 테스트

d06e9e7

refactor(menuGroup): 메뉴그룹 도메인 리팩토링 및 테스트

236e116

refactor(menu): 메뉴 도메인 리팩토링 및 테스트 추가

3cec725

refactor(orderTable): 주문 테이블 도메인 리팩토링 및 테스트 추가

ce41530

refactor(orderTable): 단체지정 도메인 리팩토링 및 테스트 추가

8c6aaa2

refactor(order): 주문 도메인 리팩토링 및 테스트 추가

f363ee9

refactor(product): ProductRepository 생성 및 JPA 엔티티 적용

c8ec268

refactor(menuGroup): MenuGroupRepository 생성 및 JPA 엔티티 적용

50db07d

refactor(menu): MenuRepository 생성 및 JPA 엔티티 적용

f2eee5e

refactor(menu): 테스트 오류 관련 리팩토링

f67dfb6

refactor(order): 주문 관련 Repository 생성 및 JPA 엔티티 적용

28cb570

refactor(orderTable): 주문테이블 관련 테스트 정상동작 되도록 리팩토링

d97e55e

refactor(tableGroup): 단체테이블 관련 테스트 정상동작 되도록 리팩토링

bd1727f

refactor(order): 주문 관련 테스트 정상동작 되도록 리팩토링

40d085b

refactor(orderTable): 주문 테이블 관련 테스트 정상동작 되도록 리팩토링

07e8f32

refactor(dao): dao 파일 제거

5657830

refactor(entity): setter 함수 제거

5592eb5

refactor(dto): setter 함수 제거

a386a70

refactor: 사용하지 않는 import 제거

6579b95

refactor(tableGroup): 테이블 그룹 도메인 리팩토링

015091e

refactor(ordertable): 주문테이블 도메인 리팩토링

c396799

refactor(menu): 메뉴테이블 도메인 리팩토링

9856654

refactor(tableGroup): 테이블그룹 fixture 리팩토링

5a5ac0b

refactor: 불필요한 fixture 함수 삭제

aa96263

refactor: 사용하지 않는 fixture setup 삭제

1458e86

refactor(orderStatus): 주문상태 도메인 테스트코드 작성

42f14cf

daegyeom.kim added 5 commits December 14, 2022 00:22

test(menuProduct): 메뉴상품 도메인 테스트 코드 추가

33d0aab

test(menu): 메뉴 도메인 테스트 코드 추가

5f770d6

test(orderTable): 주문테이블 도메인 테스트 코드 추가

9fa6e82

test(price): 가격 도메인 테스트 코드 추가

12d4ffa

test(tableGroup): 테이블그룹 도메인 테스트 코드 추가

8de614e

limwoobin requested changes Dec 14, 2022

View reviewed changes

daegyeom.kim added 18 commits December 15, 2022 01:12

test(menuGroupService): 가독성을 위해 중간에 변수로 분리

9925336

refactor(orderService): orderLineItem size 체크로직 추가

15a575f

refactor(orderService): menuId 추출을 위한 메소드명 변경

691d303

refactor(tableGroupService): 주문상태 검증 로직을 위한 함수 생성

210412e

refactor(orderTable): group, ungroup 메소드 생성

0ae4c29

refactor(orderTable): 테이블그룹 생성자 매개변수 제거

358ce74

refactor(orderTable): addOrder 메소드 네이밍 수정

ee3e2ac

refactor(orderTable): order에 batch size 설정

54fb454

refactor(tableGroup): orderTables에 batch size 설정

af7c111

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

f259189

refactor: request 객체에서 toEntity 함수 사용

0929e35

refactor(menuResponse): toMenuResponse 함수 네이명 변경

efb7771

refactor(controller): @RequestMapping 사용

73e2eff

test(menu): 누락된 displayname 작성

a6f2836

refactor(orderStatus): @Enumurate 사용

929031b

refactor(orderTables): 주문테이블 목록 일급컬렉션 사용

c05ceee

refactor(orderStatus): enum description 추가

c68807f

refactor(errorMessage): 예외 메시지 관리를 위한 클래스 추가

3296da2

daegyeom.kim added 2 commits December 16, 2022 10:17

refactor(productRepository): 트랜잭션 횟수 줄이기 위해 findAllByIdIn 활용

d3d39b9

refactor(menuService): 샹품ID validation 추가

f341f49

limwoobin approved these changes Dec 16, 2022

View reviewed changes

limwoobin merged commit aa07f83 into next-step:gyeom Dec 16, 2022

		public ResponseEntity<MenuGroupResponse> create(@RequestBody final MenuGroupRequest request) {
		final MenuGroupResponse created = menuGroupService.create(request);

		@@ -0,0 +1,36 @@
		package kitchenpos.common;

		public enum ErrorMessage {

[김대겸] Step2 PR #681

[김대겸] Step2 PR #681

Conversation

Gyeom commented Dec 13, 2022

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

Gyeom Dec 14, 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

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

limwoobin commented Dec 14, 2022

Gyeom commented Dec 15, 2022

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

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 14, 2022 •

edited