-
Notifications
You must be signed in to change notification settings - Fork 264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[레거시 코드 리팩토링 - 3단계] 허브(방대의) 미션 제출합니다. #610
Changes from all commits
59581d0
04e0190
a4609be
45a79de
865695e
69187e4
865bb3a
afe5d07
47a334d
becc941
b0c5a13
bddd2ed
7a08a36
8b03397
6fa09f5
ee1a248
f0a84c5
229178f
f52aed9
df3bca5
6b25729
15a9df5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,10 @@ Content-Type: application/json | |
{ | ||
"menuId": 1, | ||
"quantity": 1 | ||
}, | ||
{ | ||
"menuId": 2, | ||
"quantity": 2 | ||
} | ||
] | ||
} | ||
|
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
package kitchenpos.application.menu; | ||
|
||
import static java.util.function.Function.identity; | ||
import static java.util.stream.Collectors.toMap; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.stream.Collectors; | ||
import kitchenpos.domain.menu.Menu; | ||
import kitchenpos.domain.menu.MenuProduct; | ||
import kitchenpos.domain.product.Product; | ||
import kitchenpos.domain.product.ProductRepository; | ||
import kitchenpos.dto.menu.MenuProductRequest; | ||
import kitchenpos.dto.menu.MenuRequest; | ||
import kitchenpos.support.money.Money; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class MenuMapper { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mapper를 구현해주셨네요! Mapper가 생긴김에 제안드리는 것인데, Mapper들을 표현 계층으로 올리고 Service에서는 도메인 객체를 받게 할 순 없을까요? Mapper를 구현해주셨으니 간단하게 레이어 간의 의존성도 단방향으로 흐르게 할 수 있지 않을까 싶어 말씀드립니다! 그런데 추가로 Response를 매핑하는 작업도 필요하긴 하겠네요! 작업이 커질 것 같다면 의견만 남겨주셔도 좋습니다~ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저도 테오 의견을 너무 반영하고 싶지만 작업이 너무 커질 것 같아요 살려주세요.. 😢 현재 MenuMapper에 대해 코멘트 남겨주셨지만 아래의 질문은 OrderMapper 기준으로 봐주시면 좋을 것 같아요! 이 부분에 대해서 테오의 생각을 듣고싶어요! 그리고 코드를 작성하면서도 Mapper가 과연 Repository를 의존해도 되나 라는 생각이 들었어요. 해당 부분을 Order와 함께 저장하지 않는다면 Menu의 정보를 저장하는 좋은 방법은 어떤게 있을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍👍 저는 서비스가 도메인 객체를 받을 필요는 없다고 생각합니다(서비스 재사용성이 필요한 상황이라면 이야기가 다르겠지만요!) 저도 일단은 DTO 를 서비스에서 받는 걸 선호하는데요! 만약 응용 계층과 표현 계층의 양방향 의존을 제거하고자 한다면, Mapper는 표현 계층에 위치하는게 맞겠죠! 그런데 이 경우 Repository에 의존하는 Mapper는 어떻게 처리할 것인가에 대한 문제가 생기게 되는데, 허브가 말씀해주신 대로 ID 값을 가지고 복제해서 저장하는 방법을 사용할 수도 있을 것 같아요. Mapper가 Repository를 의존하는 건에 대해서는 저도 확신이 크게 서지는 않네요. 도메인 객체를 조립하기 위해서는 Repository가 필요한게 맞는데, 저라면 Mapper에서 쿼리가 나갈 것으로는 전혀 예측하지 못할 것 같아서요 🥲 근거만 있다면 Mapper에서 Repository를 끌고와도 다 괜찮다고 생각합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 답변 감사합니다 👍 |
||
private final ProductRepository productRepository; | ||
|
||
public MenuMapper(ProductRepository productRepository) { | ||
this.productRepository = productRepository; | ||
} | ||
|
||
public Menu toMenu(MenuRequest request) { | ||
Map<Long, Product> products = getProducts(request); | ||
List<MenuProduct> menuProducts = toMenuProducts(request, products); | ||
return new Menu(request.getName(), Money.valueOf(request.getPrice()), request.getMenuGroupId(), menuProducts); | ||
} | ||
|
||
private Map<Long, Product> getProducts(MenuRequest request) { | ||
return productRepository.findAllByIdIn(request.getMenuProductIds()).stream() | ||
.collect(toMap(Product::getId, identity())); | ||
} | ||
|
||
private List<MenuProduct> toMenuProducts(MenuRequest request, Map<Long, Product> products) { | ||
return request.getMenuProducts().stream() | ||
.map(menuProduct -> toMenuProduct(menuProduct, products.get(menuProduct.getProductId()))) | ||
.collect(Collectors.toList()); | ||
} | ||
|
||
private MenuProduct toMenuProduct(MenuProductRequest menuProduct, Product product) { | ||
if (product == null) { | ||
throw new IllegalArgumentException("상품이 존재하지 않습니다."); | ||
} | ||
return new MenuProduct(product.getId(), product.getName(), product.getPrice(), menuProduct.getQuantity()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
package kitchenpos.application.menu; | ||
|
||
import static java.util.stream.Collectors.toList; | ||
|
||
import java.util.List; | ||
import kitchenpos.domain.menu.Menu; | ||
import kitchenpos.domain.menu.MenuRepository; | ||
import kitchenpos.domain.menu.MenuValidator; | ||
import kitchenpos.dto.menu.MenuRequest; | ||
import kitchenpos.dto.menu.MenuResponse; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Transactional | ||
@Service | ||
public class MenuService { | ||
private final MenuRepository menuRepository; | ||
private final MenuMapper menuMapper; | ||
private final MenuValidator menuValidator; | ||
|
||
public MenuService(MenuRepository menuRepository, MenuMapper menuMapper, MenuValidator menuValidator) { | ||
this.menuRepository = menuRepository; | ||
this.menuMapper = menuMapper; | ||
this.menuValidator = menuValidator; | ||
} | ||
|
||
public MenuResponse create(MenuRequest request) { | ||
Menu menu = menuMapper.toMenu(request); | ||
menu.register(menuValidator); | ||
return MenuResponse.from(menuRepository.save(menu)); | ||
} | ||
|
||
@Transactional(readOnly = true) | ||
public List<MenuResponse> list() { | ||
return menuRepository.findAll().stream() | ||
.map(MenuResponse::from) | ||
.collect(toList()); | ||
} | ||
greeng00se marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package kitchenpos.application.order; | ||
|
||
import static java.util.stream.Collectors.toList; | ||
|
||
import java.util.List; | ||
import java.util.stream.Collectors; | ||
import kitchenpos.domain.menu.Menu; | ||
import kitchenpos.domain.menu.MenuProducts; | ||
import kitchenpos.domain.menu.MenuRepository; | ||
import kitchenpos.domain.order.Order; | ||
import kitchenpos.domain.order.OrderLineItem; | ||
import kitchenpos.domain.order.OrderMenu; | ||
import kitchenpos.domain.order.OrderMenuProduct; | ||
import kitchenpos.dto.order.OrderCreateRequest; | ||
import kitchenpos.dto.order.OrderLineItemRequest; | ||
import org.springframework.stereotype.Component; | ||
|
||
@Component | ||
public class OrderMapper { | ||
private final MenuRepository menuRepository; | ||
|
||
public OrderMapper(MenuRepository menuRepository) { | ||
this.menuRepository = menuRepository; | ||
} | ||
|
||
public Order toOrder(OrderCreateRequest request) { | ||
Long orderTableId = request.getOrderTableId(); | ||
return request.getOrderLineItems().stream() | ||
.map(this::toOrderLineItem) | ||
.collect(Collectors.collectingAndThen(toList(), items -> new Order(orderTableId, items))); | ||
} | ||
|
||
private OrderLineItem toOrderLineItem(OrderLineItemRequest request) { | ||
return new OrderLineItem(null, request.getMenuId(), request.getQuantity(), toOrderMenu(request)); | ||
} | ||
|
||
private OrderMenu toOrderMenu(OrderLineItemRequest request) { | ||
Menu menu = menuRepository.getById(request.getMenuId()); | ||
return new OrderMenu(menu.getName(), menu.getPrice(), toOrderMenuProducts(menu.getMenuProducts())); | ||
} | ||
|
||
private List<OrderMenuProduct> toOrderMenuProducts(MenuProducts menuProducts) { | ||
return menuProducts.getItems().stream() | ||
.map(item -> new OrderMenuProduct(item.getName(), item.getPrice(), item.getQuantity())) | ||
.collect(toList()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
package kitchenpos.application.order; | ||
|
||
import static java.util.stream.Collectors.toList; | ||
|
||
import java.util.List; | ||
import kitchenpos.domain.order.Order; | ||
import kitchenpos.domain.order.OrderRepository; | ||
import kitchenpos.domain.order.OrderStatus; | ||
import kitchenpos.domain.order.OrderValidator; | ||
import kitchenpos.dto.order.OrderCreateRequest; | ||
import kitchenpos.dto.order.OrderResponse; | ||
import kitchenpos.dto.order.OrderStatusUpdateRequest; | ||
import org.springframework.stereotype.Service; | ||
import org.springframework.transaction.annotation.Transactional; | ||
|
||
@Transactional | ||
@Service | ||
public class OrderService { | ||
private final OrderMapper orderMapper; | ||
private final OrderRepository orderRepository; | ||
private final OrderValidator orderValidator; | ||
|
||
public OrderService(OrderMapper orderMapper, OrderRepository orderRepository, OrderValidator orderValidator) { | ||
this.orderMapper = orderMapper; | ||
this.orderRepository = orderRepository; | ||
this.orderValidator = orderValidator; | ||
} | ||
|
||
public OrderResponse create(OrderCreateRequest request) { | ||
Order order = orderMapper.toOrder(request); | ||
order.place(orderValidator); | ||
return OrderResponse.from(orderRepository.save(order)); | ||
} | ||
|
||
@Transactional(readOnly = true) | ||
public List<OrderResponse> list() { | ||
return orderRepository.findAll().stream() | ||
.map(OrderResponse::from) | ||
.collect(toList()); | ||
} | ||
|
||
public OrderResponse changeOrderStatus(Long orderId, OrderStatusUpdateRequest request) { | ||
Order order = orderRepository.getById(orderId); | ||
order.changeOrderStatus(OrderStatus.valueOf(request.getOrderStatus())); | ||
return OrderResponse.from(orderRepository.save(order)); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
허브의 고민을 쉽게 볼 수 있어서 좋네요 💯
확실히 말씀해주신 부분도 납득이 되는군요!
ID를 포함하지 않는 생성자를 지정해주는 경우, 오히려 JPA 구현기술에 의존적으로 ID를 생성할 수 밖에 없다. ID를 도메인을 식별하기 위한 하나의 값으로 보는게 타당하다
라고 이해를 했어요.그런데 궁금한 점이 생겼습니다!
식별자를 생성자에 드러내지 않아서 생기는 문제는 어떤게 있을까요?
반대로 이야기하면, 식별자를 직접 지정해줘야 하는 경우는 언제가 있나요?
저는 아직 도메인 객체에 식별자를 지정해줘야 하는 케이스가 잘 떠오르지 않는 것 같아서요 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
생성자에 드러내지 않아서 생기는 문제는 크게 없을 것 같긴한데, 사용하는 입장에서 어떤 식별자를 사용하는지 알기 힘들 것 같다는 생각이 들어요. 만약 식별자로 UUID를 사용한다면 애플리케이션에서 직접 지정해줘서 생성할 수 있을 것 같아요! #610 (comment) 로 이야기 이어나가보면 좋을 것 같아요!