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

[레거시 코드 리팩터링 - 1단계] 허브(방대의) 미션 제출합니다. #465

Merged
merged 13 commits into from
Oct 12, 2023
85 changes: 71 additions & 14 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,77 @@

## 요구 사항

### 상품

- 상품명과 상품 가격을 입력받아 상품을 등록할 수 있다.
- 상품의 가격은 0원 이상이어야 한다.
- 상품 목록을 조회할 수 있다.

### 메뉴

- 메뉴명, 가격, 메뉴 그룹 번호, 메뉴 상품 목록을 입력받아 메뉴를 등록할 수 있다.
- 메뉴의 가격은 0원 이상이어야 한다.
- 존재하는 메뉴 그룹 번호를 입력받아야 한다.
- 메뉴 상품 목록은 비어있을 수 없다.
Copy link
Member

Choose a reason for hiding this comment

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

메뉴 가격이 0원이면서 상품 목록이 비어있는 경우는 없을까요?

물론 메뉴 가격이 0원인 경우는 없겠지만, 비즈니스 로직 상 제약이 없기 떄문에 충분히 가능한 시나리오라고 여겨져요!

허브는 어떻게 생각하시나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

오 그러네요 상품 목록이 비어있지만 메뉴 가격이 0원인 경우에는 등록할 수 있네요!
해당 부분도 충분히 가능한 시나리오라고 생각됩니다.
해당 부분을 제거하고, 각각의 메뉴 상품의 경우 상품이 존재해야 한다. 로 변경했습니다!

- 메뉴의 가격은 메뉴 상품들의 금액의 합보다 클 수 없다.
- 메뉴 목록을 조회할 수 있다.

### 메뉴 그룹

- 메뉴 그룹명을 입력받아 메뉴 그룹을 등록할 수 있다.
- 메뉴 그룹의 목록을 조회할 수 있다.

### 주문

- 테이블 번호와 주문 항목 목록을 받아 주문을 등록할 수 있다.
- 빈 테이블이 아닌 번호를 입력 받아야 한다.
- 주문 항목 목록은 비있을 수 없다.
- 등록되지 않은 메뉴를 주문할 수 없다.
- 주문이 등록되는 경우 주문의 상태가 조리중(COOKING)으로 변경된다.
- 주문의 상태를 변경할 수 있다.
- 조리중(COOKING), 식사중(MEAL), 완료(COMPLETION) 상태가 존재한다.
- 주문의 상태가 이미 완료(COMPLETION)인 경우 상태를 변경할 수 없다.
- 주문 목록을 조회할 수 있다.

### 단체 지정

- 테이블의 번호를 입력받아 단체 지정 할 수 있다.
- 입력받은 테이블 번호가 1개 이하라면 단체 지정 할 수 없다.
- 빈 테이블이 아닌 경우 단체 지정 할 수 없다.
- 이미 단체 지정이 된 테이블의 경우 단체 지정 할 수 없다.
- 단체 지정을 하는 경우 테이블의 상태가 주문 테이블로 변경된다.
- 그룹 번호를 입력받아 단체 지정을 해제할 수 있다.

### 테이블

- 테이블을 등록할 수 있다.
- 테이블의 상태를 변경할 수 있다.
- 빈 테이블, 주문 테이블 상태가 존재한다.
- 단체 지정이 되어있는 테이블인 경우 테이블의 상태를 변경할 수 없다.
- 테이블의 주문 상태가 조리중이거나 식사중인 경우 테이블의 상태를 변경할 수 없다.
- 테이블에 방문한 손님 수를 지정할 수 있다.
- 지정시 방문한 손님 수는 0명 이상이어야 한다.
- 빈 테이블인 경우 손님 수를 지정할 수 없다.
- 테이블 목록을 조회할 수 있다.

## 용어 사전

| 한글명 | 영문명 | 설명 |
| --- | --- | --- |
| 상품 | product | 메뉴를 관리하는 기준이 되는 데이터 |
| 메뉴 그룹 | menu group | 메뉴 묶음, 분류 |
| 메뉴 | menu | 메뉴 그룹에 속하는 실제 주문 가능 단위 |
| 메뉴 상품 | menu product | 메뉴에 속하는 수량이 있는 상품 |
| 금액 | amount | 가격 * 수량 |
| 주문 테이블 | order table | 매장에서 주문이 발생하는 영역 |
| 빈 테이블 | empty table | 주문을 등록할 수 없는 주문 테이블 |
| 주문 | order | 매장에서 발생하는 주문 |
| 주문 상태 | order status | 주문은 조리 ➜ 식사 ➜ 계산 완료 순서로 진행된다. |
| 한글명 | 영문명 | 설명 |
|----------|------------------|-------------------------------|
| 상품 | product | 메뉴를 관리하는 기준이 되는 데이터 |
| 메뉴 그룹 | menu group | 메뉴 묶음, 분류 |
| 메뉴 | menu | 메뉴 그룹에 속하는 실제 주문 가능 단위 |
| 메뉴 상품 | menu product | 메뉴에 속하는 수량이 있는 상품 |
| 금액 | amount | 가격 * 수량 |
| 주문 테이블 | order table | 매장에서 주문이 발생하는 영역 |
| 빈 테이블 | empty table | 주문을 등록할 수 없는 주문 테이블 |
| 주문 | order | 매장에서 발생하는 주문 |
| 주문 상태 | order status | 주문은 조리 ➜ 식사 ➜ 계산 완료 순서로 진행된다. |
| 방문한 손님 수 | number of guests | 필수 사항은 아니며 주문은 0명으로 등록할 수 있다. |
| 단체 지정 | table group | 통합 계산을 위해 개별 주문 테이블을 그룹화하는 기능 |
| 주문 항목 | order line item | 주문에 속하는 수량이 있는 메뉴 |
| 매장 식사 | eat in | 포장하지 않고 매장에서 식사하는 것 |
| 단체 지정 | table group | 통합 계산을 위해 개별 주문 테이블을 그룹화하는 기능 |
| 주문 항목 | order line item | 주문에 속하는 수량이 있는 메뉴 |
| 매장 식사 | eat in | 포장하지 않고 매장에서 식사하는 것 |

## 테이블

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

Choose a reason for hiding this comment

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

꼼꼼한 기능목록에 ERD까지 좋네요 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

꼼꼼한 리뷰에 코멘트까지 좋네요 👍

17 changes: 8 additions & 9 deletions src/main/java/kitchenpos/application/MenuService.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
package kitchenpos.application;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import kitchenpos.dao.MenuDao;
import kitchenpos.dao.MenuGroupDao;
import kitchenpos.dao.MenuProductDao;
Expand All @@ -10,11 +14,6 @@
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.math.BigDecimal;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;

@Service
public class MenuService {
private final MenuDao menuDao;
Expand All @@ -39,24 +38,24 @@ public Menu create(final Menu menu) {
final BigDecimal price = menu.getPrice();

if (Objects.isNull(price) || price.compareTo(BigDecimal.ZERO) < 0) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("메뉴의 가격은 0원 이상이어야 합니다.");
}

if (!menuGroupDao.existsById(menu.getMenuGroupId())) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("존재하지 않는 메뉴 그룹 아이디입니다.");
}

final List<MenuProduct> menuProducts = menu.getMenuProducts();

BigDecimal sum = BigDecimal.ZERO;
for (final MenuProduct menuProduct : menuProducts) {
final Product product = productDao.findById(menuProduct.getProductId())
.orElseThrow(IllegalArgumentException::new);
.orElseThrow(() -> new IllegalArgumentException("상품이 존재하지 않습니다."));
sum = sum.add(product.getPrice().multiply(BigDecimal.valueOf(menuProduct.getQuantity())));
}

if (price.compareTo(sum) > 0) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("메뉴의 가격은 메뉴 상품들의 금액의 합보다 클 수 없습니다.");
}

final Menu savedMenu = menuDao.save(menu);
Expand Down
23 changes: 11 additions & 12 deletions src/main/java/kitchenpos/application/OrderService.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package kitchenpos.application;

import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;
import kitchenpos.dao.MenuDao;
import kitchenpos.dao.OrderDao;
import kitchenpos.dao.OrderLineItemDao;
Expand All @@ -12,12 +17,6 @@
import org.springframework.transaction.annotation.Transactional;
import org.springframework.util.CollectionUtils;

import java.time.LocalDateTime;
import java.util.ArrayList;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

@Service
public class OrderService {
private final MenuDao menuDao;
Expand All @@ -42,24 +41,24 @@ public Order create(final Order order) {
final List<OrderLineItem> orderLineItems = order.getOrderLineItems();

if (CollectionUtils.isEmpty(orderLineItems)) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("주문 항목 목록이 있어야 합니다.");
}

final List<Long> menuIds = orderLineItems.stream()
.map(OrderLineItem::getMenuId)
.collect(Collectors.toList());

if (orderLineItems.size() != menuDao.countByIdIn(menuIds)) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("등록되지 않은 메뉴를 주문할 수 없습니다.");
}

order.setId(null);

final OrderTable orderTable = orderTableDao.findById(order.getOrderTableId())
.orElseThrow(IllegalArgumentException::new);
.orElseThrow(() -> new IllegalArgumentException("등록되지 않은 테이블에서 주문을 할 수 없습니다."));

if (orderTable.isEmpty()) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("빈 테이블인 경우 주문을 할 수 없습니다.");
}

order.setOrderTableId(orderTable.getId());
Expand Down Expand Up @@ -92,10 +91,10 @@ public List<Order> list() {
@Transactional
public Order changeOrderStatus(final Long orderId, final Order order) {
final Order savedOrder = orderDao.findById(orderId)
.orElseThrow(IllegalArgumentException::new);
.orElseThrow(() -> new IllegalArgumentException("주문을 찾을 수 없습니다."));

if (Objects.equals(OrderStatus.COMPLETION.name(), savedOrder.getOrderStatus())) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("완료된 주문의 상태는 변경할 수 없습니다.");
}

final OrderStatus orderStatus = OrderStatus.valueOf(order.getOrderStatus());
Expand Down
9 changes: 4 additions & 5 deletions src/main/java/kitchenpos/application/ProductService.java
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
package kitchenpos.application;

import java.math.BigDecimal;
import java.util.List;
import java.util.Objects;
import kitchenpos.dao.ProductDao;
import kitchenpos.domain.Product;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.math.BigDecimal;
import java.util.List;
import java.util.Objects;

@Service
public class ProductService {
private final ProductDao productDao;
Expand All @@ -22,7 +21,7 @@ public Product create(final Product product) {
final BigDecimal price = product.getPrice();

if (Objects.isNull(price) || price.compareTo(BigDecimal.ZERO) < 0) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("상품의 가격은 0원 이상이어야 합니다.");
}

return productDao.save(product);
Expand Down
25 changes: 14 additions & 11 deletions src/main/java/kitchenpos/application/TableGroupService.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
package kitchenpos.application;

import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;
import kitchenpos.dao.OrderDao;
import kitchenpos.dao.OrderTableDao;
import kitchenpos.dao.TableGroupDao;
Expand All @@ -10,19 +15,17 @@
import org.springframework.transaction.annotation.Transactional;
import org.springframework.util.CollectionUtils;

import java.time.LocalDateTime;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import java.util.stream.Collectors;

@Service
public class TableGroupService {
private final OrderDao orderDao;
private final OrderTableDao orderTableDao;
private final TableGroupDao tableGroupDao;

public TableGroupService(final OrderDao orderDao, final OrderTableDao orderTableDao, final TableGroupDao tableGroupDao) {
public TableGroupService(
final OrderDao orderDao,
final OrderTableDao orderTableDao,
final TableGroupDao tableGroupDao
) {
this.orderDao = orderDao;
this.orderTableDao = orderTableDao;
this.tableGroupDao = tableGroupDao;
Expand All @@ -33,7 +36,7 @@ public TableGroup create(final TableGroup tableGroup) {
final List<OrderTable> orderTables = tableGroup.getOrderTables();

if (CollectionUtils.isEmpty(orderTables) || orderTables.size() < 2) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("단체 지정하려는 테이블은 2개 이상이어야 합니다.");
}

final List<Long> orderTableIds = orderTables.stream()
Expand All @@ -43,12 +46,12 @@ public TableGroup create(final TableGroup tableGroup) {
final List<OrderTable> savedOrderTables = orderTableDao.findAllByIdIn(orderTableIds);

if (orderTables.size() != savedOrderTables.size()) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("올바른 테이블을 입력해주세요.");
}

for (final OrderTable savedOrderTable : savedOrderTables) {
if (!savedOrderTable.isEmpty() || Objects.nonNull(savedOrderTable.getTableGroupId())) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("비어있지 않거나, 이미 단체 지정이 된 테이블은 단체 지정을 할 수 없습니다.");
}
}

Expand Down Expand Up @@ -77,7 +80,7 @@ public void ungroup(final Long tableGroupId) {

if (orderDao.existsByOrderTableIdInAndOrderStatusIn(
orderTableIds, Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("테이블의 주문 상태가 조리중이거나 식사중인 경우 단체 지정 해제를 할 수 없습니다.");
}

for (final OrderTable orderTable : orderTables) {
Expand Down
19 changes: 9 additions & 10 deletions src/main/java/kitchenpos/application/TableService.java
Original file line number Diff line number Diff line change
@@ -1,16 +1,15 @@
package kitchenpos.application;

import java.util.Arrays;
import java.util.List;
import java.util.Objects;
import kitchenpos.dao.OrderDao;
import kitchenpos.dao.OrderTableDao;
import kitchenpos.domain.OrderStatus;
import kitchenpos.domain.OrderTable;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;

import java.util.Arrays;
import java.util.List;
import java.util.Objects;

@Service
public class TableService {
private final OrderDao orderDao;
Expand All @@ -36,15 +35,15 @@ public List<OrderTable> list() {
@Transactional
public OrderTable changeEmpty(final Long orderTableId, final OrderTable orderTable) {
final OrderTable savedOrderTable = orderTableDao.findById(orderTableId)
.orElseThrow(IllegalArgumentException::new);
.orElseThrow(() -> new IllegalArgumentException("존재하지 않는 테이블입니다."));

if (Objects.nonNull(savedOrderTable.getTableGroupId())) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("단체 지정이 되어있는 경우 테이블의 상태를 변경할 수 없습니다.");
}

if (orderDao.existsByOrderTableIdAndOrderStatusIn(
orderTableId, Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("테이블의 주문 상태가 조리중이거나 식사중인 경우 테이블의 상태를 변경할 수 없습니다.");
}

savedOrderTable.setEmpty(orderTable.isEmpty());
Expand All @@ -57,14 +56,14 @@ public OrderTable changeNumberOfGuests(final Long orderTableId, final OrderTable
final int numberOfGuests = orderTable.getNumberOfGuests();

if (numberOfGuests < 0) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("방문한 손님 수는 음수가 될 수 없습니다.");
}

final OrderTable savedOrderTable = orderTableDao.findById(orderTableId)
.orElseThrow(IllegalArgumentException::new);
.orElseThrow(() -> new IllegalArgumentException("주문 테이블을 찾을 수 없습니다."));
Copy link
Member

Choose a reason for hiding this comment

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

예외 상황에 대해서는 비즈니스적인 의미를 담을 수 있도록 모두 메세지를 추가해주셨네요 👍

아마 허브만의 판단 기준이 있었을테지만, 궁금해서 하나 여쭤봅니다!

해당 서비스 클래스를 레거시로 바라본다면 변경 포인트가 생겨도 괜찮을까요?
변경이 되어도 괜찮다면 이유는 무엇일까요? 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

현재 레거시 코드에서 예외 메시지만 수정을 했는데요!
예외를 구분하기 어렵다면, 테스트 작성 및 리팩터링 단계에서 어떤 예외가 발생했는지 명확히 구분이 어려울 거라고 생각했습니다!
따라서 레거시 코드를 및 로직을 최대한 수정하지 않고, 리팩터링 전 예외 메시지만 추가하도록 수정했습니다.

변경 포인트가 기존의 비즈니스 로직이 변경되는 부분이 아니고, 단순히 예외 메시지를 추가하는 정도라면 생산성 측면에서 괜찮다고 생각하는데 어떻게 생각하시나요?

Copy link
Member

Choose a reason for hiding this comment

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

저는 추후 실무에서 레거시 코드를 접했을 때, 불가피한 이유로 코드 수정이 불가능한 상황이 있을 수도 있겠다고 생각했습니다! 물론 현재는 프로덕션 코드의 수정이 가능하고 제약이 없는 상황인데요.

실무 상황에서는 정책적인 이슈 등으로 인해 쉽게 건들 수 없는 경우도 있을 것 같다고 생각했어요! 또한, 빌드 파일이 바뀌어서는 안되는 경우도 있지 않을까요? 다시 빌드하기 어려운 환경도 있을 것 같아요.

다만 이 부분은 제가 실무를 경험해본 것이 아니라 추측일 뿐입니다! 실제로는 그런 상황이 없을수도 있겠죠 😅

제 개인적인 견해이니 참고만 하시면 좋을 것 같습니다!

Copy link
Member Author

Choose a reason for hiding this comment

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

오.. 코드를 아예 수정하지 않는다는 제약 조건을 설정하고 진행해봐도 재밌겠네요 👍


if (savedOrderTable.isEmpty()) {
throw new IllegalArgumentException();
throw new IllegalArgumentException("빈 테이블에는 손님을 지정할 수 없습니다.");
}

savedOrderTable.setNumberOfGuests(numberOfGuests);
Expand Down
Loading