[레거시 코드 리팩터링 - 3단계] 제이미(임정수) 미션 제출합니다. #767

JJ503 · 2023-10-28T03:22:23Z

안녕하세요 오리!

이번 미션에서는 3단계에 첨부되어 있는 우아한객체지향 영상을 참고해 최대한 의존성 제거를 하려 노력했습니다.
그런데, 이런 고민을 이번에 처음 해보기에 제대로 한 건지 잘 모르겠네요...

의존성 제거를 위해 설계는 다음과 같이 진행했습니다.
연두색 박스의 경우 객체를 묶은 경우이며, 함께 묶인 객체들 외에는 ID를 통해 간접참조 하도록 했습니다.
또한, 이미지 상에는 없지만, repository 참조에 대해 validator 및 manager로 분리해 진행했는데 잘한 것인지 잘 모르겠네요..!

이번 미션도 리뷰 잘 부탁드립니다~!

carsago

제이미 간단한 코멘트를 남겨보았습니다 확인해주세요.

carsago · 2023-10-28T06:19:18Z

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

+public class MenuService {
+
+ private final MenuRepository menuRepository;
+ private final MenuProductRepository menuProductRepository;


현재 제이미가 구현한 코드에서 MenuProductRepository라는 클래스가 반드시 필요할까요?
질문을 바꿔서 MenuProductRepository를 통해 MenuProduct를 영속화 시키는 경우가 있을까요?

기존에 MenuProductRepository를 참조한 이유는 MenuRepository에서 findAll()를 수행할 때 fetch join을 수행하지 않고 있었기에 사용해 주었습니다.
하지만, 아래 피드백 반영을 진행하며 fetch join 되도록 수정했기 때문에 더 이상 사용하지 않게 되었습니다.

그런데 원래도 menuProduct의 경우 굳이 영속화해주지 않더라도 Menu를 모두 불러올 때 lazy 처리가 되긴 하지만, getter를 수행할 시 쿼리를 보내 조회하겠군요..!
결론적으로 fetch join과 무관하게 필요가 없었던 로직이네요.
감사합니다!

carsago · 2023-10-28T06:21:19Z

src/main/java/kitchenpos/menu/domain/MenuProduct.java

 @JoinColumn(name = "menu_id", foreignKey = @ForeignKey(name = "fk_menu_product_menu"))
 private Menu menu;


MenuProduct가 Menu를 알아야한다면 어떤 이유에서였을까요?

Menu에서 @OneToMany를 설정할 때 mappedBy로 하는 게 익숙해져 @JoinColumn을 쓸 생각을 아예 못했네요..!
Menu만 MenuProduct를 알도록 수정했습니다!

carsago · 2023-10-28T06:25:04Z

src/main/java/kitchenpos/menu/domain/MenuValidator.java

+ public void validateMenuPrice(final List<MenuProduct> menuProducts, final Price menuPrice) {
+ final Map<Long, Product> products = findAllProducts(menuProducts);
+ final Price totalPrice = calculateTotalPrice(menuProducts, products);
+
+ if (menuPrice.isHigherThan(totalPrice)) {
+ throw new InvalidMenuPriceException("메뉴 가격이 상품들의 가격 합보다 클 수 없습니다.");
+ }
+ }


현재 이 검증을 테스트 하기위해선 반드시 DB가 필요하네요. 그렇다면 실제로 DB를 연결하거나 mocking이 필요할텐데.. 테스트하기부담스럽다는 생각이 듭니다.

말씀해 주신 내용을 해결하기 위해 MenuProduct의 Product의 price가 저장될 수 있도록 수정해 보았습니다.
그런데, 검증을 위해 product의 내용을 가져왔다는 아직은 마음에 걸리네요..!
혹시 해당 부분에 대해선 어떻게 생각하시나요?

음 어차피 MenuProduct에 price가 필요하니까 괜찮지 않을까요? Product 가격이 변경됨에 따라 MenuProduct 값도 변경 되는건 피해야하니..

제가 이해한 것이 맞는지 궁금합니다!
MenuProduct의 경우 Menu를 지정할 때의 Product의 가격이 저장되어야 하므로, Menu를 지정한 후 Product의 가격이 변해도 이를 Menu 및 MenuProduct에서 영향을 받아서는 안 된다는 의미가 맞을까요?

carsago · 2023-10-28T06:26:35Z

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

+ for (final Menu menu : menus) {
+ final MenuProducts menuProducts = findAllMenuProducts(menu);
+ menu.updateMenuProducts(menuProducts);
+ }


이런 로직없이 Menu를 반환할 순 없을까요?
구체적으로 MenuProductRepository 없이 필요한 정보를 줄 순 없을까요

기존 로직을 리팩터링 하다 보니 아무 생각 없이 해당 로직을 제거하지 않았네요…
위 피드백에도 적었지만, fetch join 없이도 getter 수행 시 n+1 문제가 발생해서라도 정보를 조회해왔겠네요.
하지만, n+1 문제가 발생하는 것은 별로 좋지 않으니 수정하는 김해 menuRepository에서 findAll() 수행 시 EntityGroup를 통해 MenuProduct를 fetch join해 가져올 수 있도록 수정했습니다.

carsago · 2023-10-28T06:29:47Z

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

+@Service
+public class OrderService {
+ private final OrderRepository orderRepository;
+ private final OrderLineItemRepository orderLineItemRepository;


OrderLineItemRepository에서도 전 MenuProductRepository와 똑같은 생각이 들어요.

해당 부분도 menu-menuProduct와 동일한 수정했습니다!

carsago · 2023-10-28T06:40:36Z

src/main/java/kitchenpos/order/domain/OrderLineItems.java

+ @OneToMany(mappedBy = "order", fetch = FetchType.LAZY, cascade = {CascadeType.PERSIST, CascadeType.REMOVE})
+ private List<OrderLineItem> values = new ArrayList<>();


객체간의 양방향 참조가 필요한 순간도 있을 수 있다고 생각합니다.
다만 지금 제이미는 어떤 이유에서 양방향으로 두셨을까요? 또, 단위테스트를 할 때 객체를 생성하는게 너무 어렵진 않으셨나요?

해당 부분도 menu-menuProduct와 동일한 이유였습니다.
그래서 현재는 수정했습니다!

carsago · 2023-10-28T06:43:08Z

src/main/java/kitchenpos/order/domain/OrderManagerImpl.java

+import java.util.List;
+
+@Component
+public class OrderManagerImpl implements OrderManager {


Manager랑 Validator는 무엇이 다른가요?_?

Validator의 경우 검증을 한다는 의미가 너무 강하기 때문에, 검증이 아닌 다른 행위를 하는 경우 Manager라고 지칭해 주었습니다.
예를 들어, OrderTableManager(현, TableGroupManager)의 경우 검증이 아닌 addOrderTables와 ungroup을 수행하고 있습니다.
그런데 지금 보니 네이밍이 명확하지 않은 것 같아 TableGroupManager로 수정했습니다.
또한, OrderManager의 경우도 네이밍이 명확하지 않으며, 검증만 하고 있기 때문에 OrderTableValidator로 수정했습니다.

그런데 만들면서도 헷갈렸는데, 현재 인터페이스와 ~Impl로 정의한 클래스들은 의존성 역전을 위해 존재합니다.
현재 의존성의 방향은 order → orderTable → tableGroup으로 되어 있는 반면, 로직을 수행하는 경우 의존성의 방향이 반대가 되어 양방향이 되어 버리기 때문입니다.
하지만, MenuValidator의 경우 원래 의존성 방향이 menu / menuProduct → menuGroup / product였으며, 검증도 동일한 방향으로 의존하고 있습니다.
이 경우는 validator로 분리한 것이 의미가 없는 일이었을까요?

저 같은 경우 이번 미션에서는 의존성을 해결하기 위해서만 interface를 활용했는데, 그런 부분이 아니라도 복잡한 상황에서는 할 수 있다고 생각해요. (다만 미션은 그정도로 복잡하지 않다고 여기지만요)

예를 들어 A Service가 오직 검증을 위해 5개의 Repository를 가진다면, 이걸 하나의 Validator로 묶어서 1개의 Validator만 A Service가 의존하도록 한다면, mocking이나 fake 객체를 만들어서 테스트 하기 훨씬 수월하겠죠?

좋은 의견 감사합니다!

처음에 아무 생각 없이 의존성 방향은 고려하지 않고 의존성이 발생하면 무조건 validator를 만들게 되어 의미 없이 남발하게 되었네요...
즉, 원래의 의도와는 다른 validator였습니다.. ㅎㅎ
하지만, 오리 말대로 Validator로 모든 검증을 하나의 로직에서만 수행한다면 테스트 시 좀 더 수월할 수 있겠네요!

carsago · 2023-10-28T06:45:42Z

src/main/java/kitchenpos/order/domain/OrderValidator.java

+@Component
+public class OrderValidator {
+
+ private OrderTableRepository orderTableRepository;


지금은 order 패키지도 table 패키지에 의존하고, table도 order 패키지에 의존하게 된 상태로 보입니다.
구체적으로 order 패키지는 OrderValidator 내부의 OrderTableRepository를 통해 table 패키지에 의존하고
ordertable 패키지는 TableService의 OrderRepository를 통해 order 패키지에 의존하네요

헉 그렇네요..!
원래라면 Order는 OrderTable을 참조하고 있기에 해당 방향으로만 repository도 참조하게 하려고 했습니다.
그런데 TableService에서 OrderRepository를 참조하는 것을 했다고 생각하고 넘어가버렸네요.

carsago · 2023-10-28T06:52:34Z

src/main/java/kitchenpos/ordertable/domain/OrderTableManagerImpl.java

+ private List<Long> convetToIds(final List<OrderTable> orderTables) {
+ return orderTables.stream()
+ .map(OrderTable::getId)
+ .collect(Collectors.toList());
+ }


Suggested change

private List<Long> convetToIds(final List<OrderTable> orderTables) {

return orderTables.stream()

.map(OrderTable::getId)

.collect(Collectors.toList());

}

private List<Long> convertToIds(final List<OrderTable> orderTables) {

return orderTables.stream()

.map(OrderTable::getId)

.collect(Collectors.toList());

}

carsago

제이미 3단계 고생하셨습니다.
얼마 안남았으니 화이팅입니다. 그리고 간단한 코멘트 달았으니 꼭 읽어주세요!!

carsago · 2023-10-28T15:42:56Z

src/main/java/kitchenpos/menu/domain/Menu.java

+ System.out.println("price : " + price.getValue());
+ System.out.println("totalPrice : " + totalPrice.getValue());


Suggested change

System.out.println("price : " + price.getValue());

System.out.println("totalPrice : " + totalPrice.getValue());

carsago · 2023-10-28T15:47:11Z

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

+ return new MenuProducts(menuProducts);
+ }
+
+ private static MenuProduct createMenuProduct(final Map<Long, Product> products, final MenuProductDto menuProductDto) {


Suggested change

private static MenuProduct createMenuProduct(final Map<Long, Product> products, final MenuProductDto menuProductDto) {

private MenuProduct createMenuProduct(final Map<Long, Product> products, final MenuProductDto menuProductDto) {

carsago · 2023-10-28T15:51:00Z

src/main/java/kitchenpos/common/vo/Price.java

 import javax.persistence.Embeddable;
 import java.math.BigDecimal;
 import java.util.Objects;

 @Embeddable
 public class Price {

+ public final static Price ZERO = new Price(BigDecimal.ZERO);


@Test void 제로는_영원히_제로다() { // given Price zero = Price.ZERO; // when zero.sum(new Price(BigDecimal.valueOf(1000))); // then assertThat(Price.ZERO).isEqualTo(new Price(BigDecimal.ZERO)); }

이 테스트 코드는 통과할까요?

오.. 해당 부분을 아예 고려하지 않았네요.
sum() 수행 시 새로운 객체가 반환되도록 수정했습니다..!

carsago · 2023-10-28T15:56:14Z

src/main/java/kitchenpos/menu/domain/MenuValidator.java

+ public void validateMenuPrice(final List<MenuProduct> menuProducts, final Price menuPrice) {
+ final Map<Long, Product> products = findAllProducts(menuProducts);
+ final Price totalPrice = calculateTotalPrice(menuProducts, products);
+
+ if (menuPrice.isHigherThan(totalPrice)) {
+ throw new InvalidMenuPriceException("메뉴 가격이 상품들의 가격 합보다 클 수 없습니다.");
+ }
+ }


음 어차피 MenuProduct에 price가 필요하니까 괜찮지 않을까요? Product 가격이 변경됨에 따라 MenuProduct 값도 변경 되는건 피해야하니..

carsago · 2023-10-28T16:00:03Z

src/main/java/kitchenpos/order/domain/OrderManagerImpl.java

+import java.util.List;
+
+@Component
+public class OrderManagerImpl implements OrderManager {


저 같은 경우 이번 미션에서는 의존성을 해결하기 위해서만 interface를 활용했는데, 그런 부분이 아니라도 복잡한 상황에서는 할 수 있다고 생각해요. (다만 미션은 그정도로 복잡하지 않다고 여기지만요)

예를 들어 A Service가 오직 검증을 위해 5개의 Repository를 가진다면, 이걸 하나의 Validator로 묶어서 1개의 Validator만 A Service가 의존하도록 한다면, mocking이나 fake 객체를 만들어서 테스트 하기 훨씬 수월하겠죠?

JJ503 added 10 commits October 27, 2023 17:39

refactor: 패키지를 도메인 기준으로 변경

8b4aa4b

refactor: MenuProduct 리스트 필드를 일급컬렉션으로 변경

b6a7e71

refactor: OrderLineItem 리스트 필드를 일급컬렉션으로 변경

1d5731d

refactor: Order와 OrderTable의 의존성 제거

f32eb22

refactor: OrderTable과 TableGroup의 의존성 제거

408e9af

refactor: MenuProduct와 Product의 의존성 제거

7a71756

refactor: Menu와 MenuGroup의 의존성 제거

359fee4

refactor: import 와일드카드 제거

25acec0

refactor: menu와 menuGroup의 추가적인 의존성 제거

e0cd5ac

refactor: 오탈자 수정

ece8abc

carsago requested changes Oct 28, 2023

View reviewed changes

JJ503 added 6 commits October 28, 2023 22:27

refactor: Menu와 MenuProduct의 연관관계를 단방향으로 수정

4a52b81

refactor: Order와 OrderLineItem의 연관관계를 단방향으로 수정

c916360

style: 오탈자 수정

c1e5588

refactor: 의존성 방향이 양방향인 경우에 대해 수정함

d18e078

refactor: 메뉴 가격에 대한 검증 역할을 Menu로 변경

4bf174a

refactor: DB 테이블 조건에 따른 도메인 엔티티 컬럼 조건 추가

b180f32

carsago approved these changes Oct 28, 2023

View reviewed changes

carsago merged commit fc303fd into woowacourse:jj503 Oct 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[레거시 코드 리팩터링 - 3단계] 제이미(임정수) 미션 제출합니다. #767

[레거시 코드 리팩터링 - 3단계] 제이미(임정수) 미션 제출합니다. #767

JJ503 commented Oct 28, 2023

carsago left a comment

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 28, 2023 •

edited

Loading

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023 •

edited

Loading

JJ503 Oct 29, 2023 •

edited

Loading

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 29, 2023

carsago Oct 28, 2023

JJ503 Oct 28, 2023

carsago Oct 28, 2023

carsago left a comment

carsago Oct 28, 2023

carsago Oct 28, 2023

carsago Oct 28, 2023

JJ503 Oct 29, 2023

carsago Oct 28, 2023 •

edited

Loading

carsago Oct 28, 2023

		@JoinColumn(name = "menu_id", foreignKey = @ForeignKey(name = "fk_menu_product_menu"))
		private Menu menu;

		@OneToMany(mappedBy = "order", fetch = FetchType.LAZY, cascade = {CascadeType.PERSIST, CascadeType.REMOVE})
		private List<OrderLineItem> values = new ArrayList<>();

		System.out.println("price : " + price.getValue());
		System.out.println("totalPrice : " + totalPrice.getValue());

	private static MenuProduct createMenuProduct(final Map<Long, Product> products, final MenuProductDto menuProductDto) {
	private MenuProduct createMenuProduct(final Map<Long, Product> products, final MenuProductDto menuProductDto) {

[레거시 코드 리팩터링 - 3단계] 제이미(임정수) 미션 제출합니다. #767

[레거시 코드 리팩터링 - 3단계] 제이미(임정수) 미션 제출합니다. #767

Conversation

JJ503 commented Oct 28, 2023

carsago 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

JJ503 Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carsago Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

JJ503 Oct 29, 2023 • edited Loading

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

carsago 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

carsago Oct 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JJ503 Oct 28, 2023 •

edited

Loading

carsago Oct 28, 2023 •

edited

Loading

JJ503 Oct 29, 2023 •

edited

Loading

carsago Oct 28, 2023 •

edited

Loading