[레거시 코드 리팩터링 - 2단계] 다즐(최우창) 미션 제출합니다. #568
Conversation
| private ProductQuantityDto convertToProductQuantity(MenuProductRequest menuProductRequest) { | ||
| Product product = productRepository.findById(menuProductRequest.getProductId()) | ||
| .orElseThrow(() -> new IllegalArgumentException("등록되지 않은 상품입니다.")); | ||
| return new ProductQuantityDto(product, menuProductRequest.getQuantity()); |
There was a problem hiding this comment.
ProductQuantityDto로 변환해서 Menu의 updateProducts에 전달하는 이유가 있나요?
MenuProduct라는 도메인을 MenuService에서 모르게하기 위함인가요..?!
ProductQuantityDto를 도메인에 넘겨주면서 domain과 application 패키지가 양방향 의존을 하게 되는 것 같아요!
이를 해결할수는 없을까요?🤔
There was a problem hiding this comment.
처음에 MenuProduct라는 도메인을 보았을 때 Menu와 Product를 이어주는 단순한 중간 테이블로 이해했어요. 그렇기에 Menu 안에서 Product와 연결을 만들어주어야 하지 않을까라는 생각을 했던 것 같아요.
밀리의 리뷰를 읽고 다시 한번 고민해 보았을 때, Quantity라는 추가적인 정보도 제공하고 있기에 단순히 중간 테이블로만 보는 것은 어려울 것 같네요. 그렇기에 양방향 의존을 가지는 dto를 두는 것보다 MenuProduct 생성 책임을 application으로 전달하는게 좋을 것 같아요! 배워갈 수 있었어요 감사합니다 🙇🏻♂️
| private MenuQuantityDto convertToMenuQuantity(OrderLineItemRequest orderLineItem) { | ||
| Menu menu = menuRepository.findById(orderLineItem.getMenuId()) | ||
| .orElseThrow(() -> new IllegalArgumentException("등록되지 않은 메뉴입니다.")); | ||
| return new MenuQuantityDto(menu, orderLineItem.getQuantity()); |
There was a problem hiding this comment.
이 부분도 ProductQuantityDto와 동일하게 궁금해요!
저는 서비스에서 직접 OrderLineItem을 만들어서 Order에 넣어주도록 했었는데요. 다즐의 코드를 보니까 고민이 엄청 되네요..!
OrderLineItem을 만드는 주체가 도메인이 되어야 할 지 서비스에서 만들어서 넣어줘야할 지 더 생각해봐야겠어요!
There was a problem hiding this comment.
저도 계속해서 고민 중인데 우선은 서비스에서 만드는 방법을 선택했습니다!
3단계 미션에서 의존성을 고민하면서 같이 고민할 것 같아요. 밀리도 생각이 정리되었을 때 알려주시면 감사합니다 ㅎㅎ
There was a problem hiding this comment.
저는 아직 양방향 의존 없이 도메인에서 직접 OrderLineItem을 만드는 방법이 떠오르지 않네요..
저도 3단계 하면서 계속 고민해볼 것 같아요 🥹
| orderDao.save(savedOrder); | ||
|
|
||
| savedOrder.setOrderLineItems(orderLineItemDao.findAllByOrderId(orderId)); | ||
| public OrderResponse changeOrderStatus(Long orderId, OrderRequest orderRequest) { |
There was a problem hiding this comment.
changeOrderStatus의 파라미터로 OrderRequest를 주고 있는데, create와 동일한 DTO를 사용하셨네요!
주문을 등록하는 API와 주문 상태를 변경하는 API는 서로 다른 body 값을 보내도록 하는데 같은 DTO를 사용하게 되면 헷갈릴 여지가 있을 것 같아요!
그리고 저는 DTO는 재사용하지 않아야 한다고 생각해서 (특히 Request DTO) API별로 다른 DTO를 만들어주었는데,
다즐은 어떻게 생각하시나요?!
There was a problem hiding this comment.
저도 동의합니다 ! UI와 맞닿아있기에 변화가 자주 발생하는 부분으로 각각의 요청, 응답 데이터는 분리하는 게 좋다고 생각해요.
기존의 코드에서 동일한 모델을 생성, 수정에 사용하고 있었기에 이를 최대한 반영하기 위해 동일한 요청 데이터 모델을 구현했습니다 !
나중에 바꿔야 할 작업이기에 미리 수정하면 좋을 것 같네요 🤓
| public static OrderStatus from(String status) { | ||
| return Arrays.stream(OrderStatus.values()) | ||
| .filter(orderStatus -> orderStatus.name().equals(status)) | ||
| .findAny() | ||
| .orElseThrow(() -> new IllegalArgumentException("등록되지 않은 주문 상태입니다.")); | ||
| } |
| OrderTable orderTable = orderTableRepository.findById(orderRequest.getOrderTableId()) | ||
| .orElseThrow(() -> new IllegalArgumentException("테이블 정보가 존재하지 않습니다.")); |
There was a problem hiding this comment.
이것은 정말 취향차이인데, 저는 findOrderTable() 이런식으로 메서드로 분리한다면 하나의 큰 메서드 읽을 때 흐름을 이해하기 쉽더라구요!
아래 OrderTable이 비었는지 확인하는 부분도 validateOrderTableEmpty() 이런식으로요!
There was a problem hiding this comment.
매번 프로젝트를 진행할 때 하는 고민인데 OrderTable에 대한 의존성이 있는 서비스마다 findOrderTable 메서드가 중복되서 나타나더라구요. 이럴 때 어떤 방법으로 중복을 해결하시나요? 🤔
There was a problem hiding this comment.
각 서비스 간의 중복같은 경우는 크게 신경쓰지 않았던 것 같아요.
사실 깊게 생각해본 적이 없어요..😅
근데 서비스에서 각각 OrderTable을 의존한다면 정말 어쩔수 없는 부분이라는 생각이 들어요.
중복을 해결하는 방법은 아직 제 머리로는 떠오르지 않네요..ㅜㅜ
그래도 덕분에 좋은 고민거리가 생긴 것 같아요 감사합니다!!
| } | ||
|
|
||
| private void validate(int numberOfGuests) { | ||
| if (numberOfGuests < 0) { |
| } | ||
|
|
||
| private void validate(long quantity) { | ||
| if (quantity < 1) { |
| @Embedded | ||
| private MenuName menuName; | ||
|
|
||
| @Embedded | ||
| private Price price; |
|
|
||
| List<Order> findAllByOrderTable(OrderTable orderTable); | ||
|
|
||
| boolean existsByOrderTableIdInAndOrderStatusIn(List<Long> orderTableIds, List<OrderStatus> list); |
There was a problem hiding this comment.
오?! 위 메서드도 쿼리를 작성하지 않아도 되는군요?! 몰랐어요....! 🥹
There was a problem hiding this comment.
JPA에서 메서드명 기반으로 대부분의 쿼리를 만들 수 있더라구요 👍
| testTransactional.invoke(() -> { | ||
| Order order = orderRepository.findById(orderId).get(); | ||
| assertSoftly(softly -> { | ||
| softly.assertThat(response).usingRecursiveComparison() | ||
| .ignoringFields("orderStatus") | ||
| .isEqualTo(OrderResponse.from(order)); | ||
| softly.assertThat(response.getOrderStatus()).isEqualTo(OrderStatus.MEAL.name()); | ||
| }); |
There was a problem hiding this comment.
오 멋있네요 👍
근데 한 가지 궁금한게 assertSoftly 절도 꼭 testTransactional 안에 들어가야 하나요??
There was a problem hiding this comment.
검증에 필요한 데이터를 트랜잭션 범위 내에서 만들어서 반환하도록 하면 트랜잭션 범위 안에 없어도 될 것 같아요 !
| "orderTableId": 1, | ||
| "orderLineItems": [ | ||
| { | ||
| "menuId": 1, | ||
| "quantity": 1 | ||
| } | ||
| ] |
안녕하세요 밀리 😀
프로젝트와 미션을 병행하다 보니 너무 늦어지게 되었네요 ㅜ
2단계 미션에서는 아래와 같은 작업들을 진행해 보았어요!
JPA적용서비스와 도메인 모델과 테스트 코드를 깔끔하게 작성하는 시간도 가져보았는데, 수정이 끝도 없이 발생할 것 같아 3단계, 4단계 미션을 진행하면서 점진적으로 수정해나가려고 합니다 :) 추가로 아래와 같은 고민도 하게 되었는데 밀리의 생각이 궁금해요!
예외를 어디서 발생시켜야 하는가?
주문을 등록하는 상황을 예로 들어볼게요. 요구사항에서 확인할 수 있듯 비어있는 테이블에 대해서는 주문을 진행할 수 없습니다. 이때 비어있는지에 대한 상태는 주문 테이블이 알고 있는데 이에 대한 검증을
checkEmpty와 같은 메서드를 주문 테이블 도메인에 만들어서 도메인 내부에서 예외를 발생시켜야 할지,isEmpty메서드를 통해 비어있는지 아닌지에 대한 값을 받아서 서비스에서 예외를 발생시켜야 할지 고민이 되더라고요.주문 테이블이 비어있다고 해서 잘못된 상황은 아니기에 이에 대한 판단은 서비스에서 이루어지도록 했는데 해당 부분에 대해서 어떻게 생각하시는지 궁금합니다!
이번 리뷰도 많이 배워가겠습니다 🙇🏻♂️