Skip to content
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

Merged
merged 22 commits into from
Oct 26, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
59581d0
docs: ID 생성자에 대한 생각 추가
greeng00se Oct 16, 2023
04e0190
refactor: 테이블의 상태 변경시 테이블의 주문 상태를 확인하는 로직 이벤트로 분리
greeng00se Oct 16, 2023
a4609be
refactor: 테이블의 그룹 해제시 테이블의 주문 상태를 확인하는 로직 이벤트로 분리
greeng00se Oct 16, 2023
45a79de
refactor: 핸들러 테스트 통합 테스트로 변경
greeng00se Oct 16, 2023
865695e
refactor: 공통 패키지 분리
greeng00se Oct 21, 2023
69187e4
refactor: product 패키지 분리
greeng00se Oct 23, 2023
865bb3a
refactor: table 패키지 분리
greeng00se Oct 23, 2023
afe5d07
refactor: tablegroup 패키지 분리
greeng00se Oct 23, 2023
47a334d
refactor: menugroup 패키지 분리
greeng00se Oct 23, 2023
becc941
refactor: menu 패키지 분리
greeng00se Oct 23, 2023
b0c5a13
refactor: order 패키지 분리
greeng00se Oct 23, 2023
bddd2ed
refactor: OrderTable, TableGroup 양방향 관계 제거
greeng00se Oct 23, 2023
7a08a36
refactor: tableGroup 메서드명 변경
greeng00se Oct 23, 2023
8b03397
refactor: 단체 지정 해제 기능 추가
greeng00se Oct 23, 2023
6fa09f5
refactor: OrderTable 도메인 이벤트 사용하도록 수정
greeng00se Oct 23, 2023
ee1a248
refactor: default 메서드 사용하도록 수정
greeng00se Oct 23, 2023
f0a84c5
refactor: OrderService default 메서드 사용하도록 수정
greeng00se Oct 23, 2023
229178f
refactor: 검증부 도메인 서비스로 분리
greeng00se Oct 23, 2023
f52aed9
refactor: 메뉴 생성 로직 개선
greeng00se Oct 23, 2023
df3bca5
refactor: 주문 메뉴 저장 로직 수정
greeng00se Oct 25, 2023
6b25729
test: 정상적으로 작동하지 않는 주문 메뉴 테스트 수정
greeng00se Oct 25, 2023
15a9df5
refactor: style 수정
greeng00se Oct 25, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,12 @@
## 테이블

<img width="1011" alt="image" src="https://github.com/greeng00se/greeng00se.github.io/assets/58586537/1c2a352e-bed7-4c0f-89ed-8d6d31487b9c">

## 생각

ID를 받는 생성자를 제거하는 과정에서 생긴 고민

1. 모든 필드를 지정해줄 수 있는 주생성자를 만드는 건 구현 기술에 침투적인 생성자가 생기는 것인가?

id 생성자를 제거한다면, 결국 도메인을 식별하기 위한 값을 넣으려면 jpa와 같은 기술에 의존적인 생성을 해야하는데, 오히려 제거하는 것이 구현 기술에 침투적인 생성자를 만드는 것이 아닌가?
결국 해당 도메인을 식별하기 위한 값으로 본다면, jpa를 사용하지 않았을 때 값을 지정해줘야 할텐데, 해당 부분을 위해 남겨둬야하지 않을까?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

허브의 고민을 쉽게 볼 수 있어서 좋네요 💯

확실히 말씀해주신 부분도 납득이 되는군요!
ID를 포함하지 않는 생성자를 지정해주는 경우, 오히려 JPA 구현기술에 의존적으로 ID를 생성할 수 밖에 없다. ID를 도메인을 식별하기 위한 하나의 값으로 보는게 타당하다라고 이해를 했어요.

그런데 궁금한 점이 생겼습니다!

식별자를 생성자에 드러내지 않아서 생기는 문제는 어떤게 있을까요?
반대로 이야기하면, 식별자를 직접 지정해줘야 하는 경우는 언제가 있나요?

저는 아직 도메인 객체에 식별자를 지정해줘야 하는 케이스가 잘 떠오르지 않는 것 같아서요 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

생성자에 드러내지 않아서 생기는 문제는 크게 없을 것 같긴한데, 사용하는 입장에서 어떤 식별자를 사용하는지 알기 힘들 것 같다는 생각이 들어요. 만약 식별자로 UUID를 사용한다면 애플리케이션에서 직접 지정해줘서 생성할 수 있을 것 같아요! #610 (comment) 로 이야기 이어나가보면 좋을 것 같아요!

52 changes: 0 additions & 52 deletions src/main/java/kitchenpos/application/MenuService.java

This file was deleted.

60 changes: 0 additions & 60 deletions src/main/java/kitchenpos/application/OrderService.java

This file was deleted.

51 changes: 0 additions & 51 deletions src/main/java/kitchenpos/application/TableGroupService.java

This file was deleted.

50 changes: 50 additions & 0 deletions src/main/java/kitchenpos/application/menu/MenuMapper.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mapper를 구현해주셨네요!

Mapper가 생긴김에 제안드리는 것인데, Mapper들을 표현 계층으로 올리고 Service에서는 도메인 객체를 받게 할 순 없을까요?

Mapper를 구현해주셨으니 간단하게 레이어 간의 의존성도 단방향으로 흐르게 할 수 있지 않을까 싶어 말씀드립니다!

그런데 추가로 Response를 매핑하는 작업도 필요하긴 하겠네요! 작업이 커질 것 같다면 의견만 남겨주셔도 좋습니다~

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저도 테오 의견을 너무 반영하고 싶지만 작업이 너무 커질 것 같아요 살려주세요.. 😢

현재 MenuMapper에 대해 코멘트 남겨주셨지만 아래의 질문은 OrderMapper 기준으로 봐주시면 좋을 것 같아요!

이 부분에 대해서 테오의 생각을 듣고싶어요!
테오는 controller용 request를 받고 service는 도메인 객체를 받도록 구현하시는 편인가요?
만약 해당 방식으로 구현한다면, 현재 구조에서(Order가 Menu의 정보를 복사해야하는 경우) 어떤 방식으로 Order를 생성하여 서비스로 전달하는 것이 좋을까요?

그리고 코드를 작성하면서도 Mapper가 과연 Repository를 의존해도 되나 라는 생각이 들었어요.
테오라면 어떻게 작성하실껀가요?

해당 부분을 Order와 함께 저장하지 않는다면 Menu의 정보를 저장하는 좋은 방법은 어떤게 있을까요?
(저는 시간이 좀 널널했다면 Mapper가 Repository 의존하지 않고 <- 이 부분에서 어색함을 많이 느꼈고 이게 맞나 �싶습니다., ID를 UUID로 전부 변경하고, 생성시 이벤트를 발행해서, 해당 이벤트에 있는 id값 가지고 Menu를 복제해서 저장하는 방법도 좋을 것 같다는 생각이 들어요. 현재 구조랑 비교해서 봐주시면 정말 좋을 것 같습니다!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍👍

저는 서비스가 도메인 객체를 받을 필요는 없다고 생각합니다(서비스 재사용성이 필요한 상황이라면 이야기가 다르겠지만요!)

저도 일단은 DTO 를 서비스에서 받는 걸 선호하는데요!

만약 응용 계층과 표현 계층의 양방향 의존을 제거하고자 한다면, Mapper는 표현 계층에 위치하는게 맞겠죠!

그런데 이 경우 Repository에 의존하는 Mapper는 어떻게 처리할 것인가에 대한 문제가 생기게 되는데, 허브가 말씀해주신 대로 ID 값을 가지고 복제해서 저장하는 방법을 사용할 수도 있을 것 같아요.

Mapper가 Repository를 의존하는 건에 대해서는 저도 확신이 크게 서지는 않네요. 도메인 객체를 조립하기 위해서는 Repository가 필요한게 맞는데, 저라면 Mapper에서 쿼리가 나갈 것으로는 전혀 예측하지 못할 것 같아서요 🥲

근거만 있다면 Mapper에서 Repository를 끌고와도 다 괜찮다고 생각합니다!

Copy link
Member Author

Choose a reason for hiding this comment

The 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());
}
}
39 changes: 39 additions & 0 deletions src/main/java/kitchenpos/application/menu/MenuService.java
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
@@ -1,12 +1,12 @@
package kitchenpos.application;
package kitchenpos.application.menugroup;

import static java.util.stream.Collectors.toList;

import java.util.List;
import kitchenpos.dao.MenuGroupRepository;
import kitchenpos.domain.MenuGroup;
import kitchenpos.dto.MenuGroupRequest;
import kitchenpos.dto.MenuGroupResponse;
import kitchenpos.domain.menugroup.MenuGroup;
import kitchenpos.domain.menugroup.MenuGroupRepository;
import kitchenpos.dto.menugroup.MenuGroupRequest;
import kitchenpos.dto.menugroup.MenuGroupResponse;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

Expand Down
45 changes: 45 additions & 0 deletions src/main/java/kitchenpos/application/order/OrderService.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
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 OrderRepository orderRepository;
private final OrderValidator orderValidator;

public OrderService(OrderRepository orderRepository, OrderValidator orderValidator) {
this.orderRepository = orderRepository;
this.orderValidator = orderValidator;
}

public OrderResponse create(OrderCreateRequest request) {
Order order = request.toOrder();
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));
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package kitchenpos.application.order;

import kitchenpos.domain.order.Order;
import kitchenpos.domain.order.OrderRepository;
import kitchenpos.domain.table.OrderTableChangedEvent;
import org.springframework.context.event.EventListener;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

@Component
public class OrderTableChangedEventHandler {

private final OrderRepository orderRepository;

public OrderTableChangedEventHandler(OrderRepository orderRepository) {
this.orderRepository = orderRepository;
}

@Transactional
@EventListener(OrderTableChangedEvent.class)
public void handle(OrderTableChangedEvent event) {
orderRepository.findByOrderTableId(event.getOrderTableId())
.ifPresent(Order::validateChangeTableStatusAllowed);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
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.table.OrderTable;
import kitchenpos.domain.table.OrderTableRepository;
import kitchenpos.domain.tablegroup.TableGroupUngroupedEvent;
import org.springframework.context.event.EventListener;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

@Component
public class TableGroupUngroupedEventHandler {

private final OrderRepository orderRepository;
private final OrderTableRepository orderTableRepository;

public TableGroupUngroupedEventHandler(OrderRepository orderRepository, OrderTableRepository orderTableRepository) {
this.orderRepository = orderRepository;
this.orderTableRepository = orderTableRepository;
}

@Transactional
@EventListener(TableGroupUngroupedEvent.class)
public void handle(TableGroupUngroupedEvent event) {
List<Long> tableIds = getTableIds(event.getTableGroupId());
orderRepository.findAllByOrderTableIds(tableIds)
.forEach(Order::validateUngroupTableAllowed);
}

private List<Long> getTableIds(Long tableGroupId) {
return orderTableRepository.findAllByTableGroupId(tableGroupId).stream()
.map(OrderTable::getId)
.collect(toList());
}
}
Loading