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단계] 성하(김성훈) 미션 제출합니다. #631

Merged
merged 18 commits into from
Oct 26, 2023

Conversation

sh111-coder
Copy link

@sh111-coder sh111-coder commented Oct 24, 2023

안녕하세요 말랑!!
이번에는 의존성 관련해서 리팩토링을 주로 했고,
이전 2단계에서 Validator를 생성해서 검증헀던 로직을 도메인으로 옮기는 리팩토링을 진행했습니다!

이번에도 리뷰 잘부탁드려요 ㅎㅎㅎ 🤣


기존 구조에서 의존성 문제

  • Order <-> OrderTable 패키지 참조 : 패키지 의존성 양방향
    • Order에서 OrderTable 객체 참조
    • OrderTableService에서 OrderRepository & OrderStatus 참조
  • OrderTable <-> TableGroup 패키지 & 객체 참조 : 패키지 & 객체 의존성 양방향

의존성 양방향 해결

1. order <-> OrderTable 패키지 참조 : 패키지 의존성 양방향

  • Order에서 OrderTable 객체 직접 참조하던 것 간접 참조로 변경
  • OrderService에서 OrderTableRepository 사용하던 로직 스프링 이벤트 사용해서 의존성 분리
    • 이벤트 발행 로직(Order 생성)과 이벤트 리슨 로직(OrderTable 검증)이 같은 트랜잭션으로 묶여야 하고, 이벤트 발행 후 실행되어야 하므로 별다른 설정 없이 @EventListener로 처리

2. OrderTable <-> TableGroup 패키지 & 객체 참조 : 패키지 & 객체 의존성 양방향

  • TableGroup에서 OrderTable을 의존하는 로직이 많았기 때문에 OrderTable에서 TableGroup 의존성 간접 참조로 끊음

비즈니스 로직 관련 의존성 끊기

  • OrderLineItem에서 Menu를 직접 참조
    • Order → Menu 의존성 방향이라서 의존성 양방향은 존재 X
    • 비즈니스 로직 상으로 메뉴 정보가 변경되더라도 주문 항목이 변경되지 않아야한다. 라는 요구사항이 존재하므로 의존성 아예 끊음
    • 주문할 당시의 정보를 저장하기 위해 OrderLineItem에서 Menu -> raw한 주문 당시 Menu의 price, name으로 변경

@sh111-coder sh111-coder self-assigned this Oct 24, 2023
Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하~~~ 말랑입니다~
3단계도 잘 구현해 주셨네요!
코드를 한번에 너무 많이 리뷰하면 부담되실 것 같아서, 가장 큼지막하게 눈에 보이는 몇몇개에 대해서만 커멘트 남겨드렸어요!

요약하면 다음과 같습니다!

  1. 다른 패키지 간의 참조는 직접참조에서 간접참조로
  2. 이벤트와 인터페이스의 차이와, 이벤트를 좀 더 잘 사용하는 방법들
  3. Service Layer에 남아있는 비즈니스 로직을 Domain Layer로 옮기는 것

궁금한 점 있으시면 얼마든지 질문 주세요!

src/main/java/kitchenpos/menu/domain/MenuProduct.java Outdated Show resolved Hide resolved

@OneToMany(cascade = CascadeType.PERSIST)
@JoinColumn(name = "table_group_id")
private List<OrderTable> orderTables;

Choose a reason for hiding this comment

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

  1. OrderTable과 TableGroup을 다른 패키지로 두신 이유가 있을까요?
  2. OrderTable와 TableGroup을 다른 패키지로 분리하셨으면 이 둘 사이의 참조도 간접참조가 되는 것은 어떨까요?

Copy link
Author

@sh111-coder sh111-coder Oct 25, 2023

Choose a reason for hiding this comment

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

OrderTable과 TableGroup을 다른 패키지로 두신 이유가 있을까요?

Order - OrderLineItem처럼 라이프 사이클이 동일한 느낌이 아니라

OrderTable이 생성될 때 TableGroup이 없는 채로 생성되기 때문에 라이프 사이클이 다른 것 같아 나누게 되었습니다!

OrderTable와 TableGroup을 다른 패키지로 분리하셨으면 이 둘 사이의 참조도 간접참조가 되는 것은 어떨까요?

오,, 확실히 그러는게 더 나을 것 같습니다! 아예 다른 패키지이므로
엔티티에서는 간접 참조를 하는게 낫겠네요!!

TableGroup 도메인에서 참조를 끊고 OrderTable -> TableGroup으로 간접 참조 단방향? 이 되도록 했습니다!
OrderTable Validate 수행하던 로직을 TableGroupValidator에서 처리하도록 했습니다!

양방향이던 것을 간접 참조로 바꾸고 단방향으로 바꾼 적이 처음이라 잘한 건지 모르겠네요,,

Choose a reason for hiding this comment

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

답변 감사합니다~ 답변을 들으니 추가적으로 궁금한 부분이 있어서 질문을 드릴게요!

OrderTable이 생성될 때 TableGroup이 없는 채로 생성되기 때문에 라이프 사이클이 다른 것 같아 나누게 되었습니다!

패키지를 나누는 기준이 생명주이로 판단하는 것 같은데 맞으실까요?
생명주기로만 패키지를 나누게 되면 비즈니스가 조금만 복잡해 지더라도 수십, 수백개의 패키지가 생성될 수도 있을 것 같다는 생각이 들어요!
생명주기는 조금 다르더라도, 서로 밀접한 도메인을 가지고 있다면 하나의 패키지 내부에서 관리할 수도 있지 않을까요?
어떻게 생각하시나요~?

Copy link
Author

Choose a reason for hiding this comment

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

다시 생각해보니 생명주기로만 패키지를 나눴었는데,
비즈니스적으로 비슷하다면 묶는 것이 좋은 것 같아요!!
진짜 감사합니다 ㅎㅎㅎㅎ 👍🏻👍🏻👍🏻

final OrderTable orderTable = orderTableRepository.findById(request.getOrderTableId())
.orElseThrow(OrderTableException.NotFoundOrderTableException::new);
final Long orderTableId = request.getOrderTableId();
eventPublisher.publishEvent(new OrderCreateValidationEvent(orderTableId));

Choose a reason for hiding this comment

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

  1. 의존성을 해결하는 방법으로는 이벤트를 발행하는 방법 말고 인터페이스를 활용하는 방법도 있을텐데요, 왜 이벤트를 발행하셨나요?
    개인적인 생각이지만, 테이블 생성 시 검증 로직은 필수적으로 진행되어야 하기 때문에 이는 이벤트보단 인터페이스를 활용하는 것이 더 적절하다 느껴집니다.

  2. 이벤트를 Service Layer에서 ApplicationEventPublisher를 사용하여 발행하는 방법도 있지만, AbstractAggregateRoot를 사용하는 방법도 있는데요, 다음과 같이도 가능합니다.

@MappedSuperclass
@EntityListeners(AuditingEntityListener.class)
public class CommonDomainModel extends AbstractAggregateRoot<CommonDomainModel> {

    @Id
    @GeneratedValue(strategy = IDENTITY)
    private Long id;

    @CreatedDate
    private LocalDateTime createdDate;

    @Override
    public List<Object> domainEvents() {
        return new ArrayList<>(super.domainEvents());
    }
}
public class Order extends CommonDomainModel {

    public void place() {
         registerEvent(new OrderCreateValidationExcetion());
    }
}
  1. 이벤트에서 OrderCreateValidationEvent를 발행하는 것 자체가 이벤트의 의도와 맞지 않아 보여요!
    이벤트는 니가 뭘 할지는 모르겠지만, 나는 뭘 했으니 알아서 처리해! 라는 느낌이 강한데,
    해당 이벤트의 이름을 보면 나 생성됐으니까 이거 받는 놈은 검증을 해야해! 라는 느낌이네요!

Copy link
Author

@sh111-coder sh111-coder Oct 25, 2023

Choose a reason for hiding this comment

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

의존성을 해결하는 방법으로는 이벤트를 발행하는 방법 말고 인터페이스를 활용하는 방법도 있을텐데요, 왜 이벤트를 발행하셨나요?

이 부분은 LMS를 보다보니 이벤트 개념이 눈에 보였고, 의존성을 해결해야 할 때 이벤트를 써야곘구나! 하고
머리에 박혔던 것 같아요 ㅠㅠ
검증 로직을 이벤트 써봤던 적이 없어서 쓰면서도 조금 애매하다고 생각했었는데,
인터페이스로 의존성을 분리하는 방법을 들어봤는데 생각하지 못했던 것 같네요 ㅠㅠㅠ

저도 인터페이스로 의존성을 분리하는 것이 적절하다고 생각해서 리팩토링하도록 하겠습니다!


AbstractAggregateRoot를 사용하는 방법

오! 대박이네요!! place라는 네이밍이 익숙해서 우아한객체지향 코드를 보니 여기서도 이렇게 이벤트를 발행했었네요!!
이벤트를 적용하면서 서비스 필드에 ApplicationEventPublisher 객체를 의존하는 것이 조금 찝찝했는데요!
AbstractAggregateRoot를 상속받아서 registerEvent를 하면 도메인 내에서 이벤트를 발행할 수 있어서
비즈니스 로직이 더 도메인에 속하게 되어 도메인만 봐도 비즈니스 로직을 이해할 수 있을 것 같아서 좋을 것 같아요!!

다양한 방법까지 알려주셔서 감사합니다!!


이벤트에서 OrderCreateValidationEvent를 발행하는 것 자체가 이벤트의 의도와 맞지 않아 보여요!

저도 처음에는 OrderCreateEvent로 네이밍을 해서 전달하려고 했었습니다!
그런데 해당 이벤트 리슨 시에 생성 검증을 진행하기 때문에 이벤트 발행 시점이 Order를 생성하기 전이었고,
그래서 해당 이벤트는 'Order를 Create할 때 검증 필요해?..' 라는 느낌으로 수정했던 것 같아요!
당시로 돌아가서 굳이 네이밍을 수정하자면 'BeforeOrderCreateEvent'?.. 느낌으로 수정해야할 것 같은데

확실히 코멘트를 쓰다보니 말랑 코멘트처럼 이벤트가 적절하진 않아 보이네요 ㅠㅠ

그래도 덕분에 이벤트 네이밍에 대해서 생각해 볼 수 있었습니다 ㅎㅎㅎ 감사함다 !

Choose a reason for hiding this comment

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

👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍👍

@sh111-coder
Copy link
Author

말랑 리뷰 감사합니다!! ㅎㅎㅎㅎ

말랑 리뷰보고 다른 패키지의 객체들을 간접 참조하다보니 자연스럽게 Domain Layer에서 Validator들을 사용해야 하더라구요!

1. 다른 패키지 간의 참조는 직접참조에서 간접참조로
2. 이벤트와 인터페이스의 차이와, 이벤트를 좀 더 잘 사용하는 방법들
3. Service Layer에 남아있는 비즈니스 로직을 Domain Layer로 옮기는 것

그래서 위와 같이 코멘트 남겨주셨던 부분중에 1번, 3번이 묶여서 진행됐고,

2번 경우에는 이벤트를 제거하고 인터페이스로 의존성을 역전시켜서 구현했습니다!!
원래는 Order <-> OrderTable의 양방향 의존에서 Order -> OrderTable의 의존을 이벤트를 사용해서 끊었었는데,
다시 생각해보니 비즈니스 적으로 Order가 OrderTable을 의존하는 게 맞는 것 같더라고요!

그래서 OrderTable -> Order 의존을 인터페이스로 의존성을 역전시켜서 해결했습니다!

Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

안녕하세요 성하~
리뷰 반영 잘 해주셨네요~
아마 이번 리뷰가 마지막이 될 것 같습니다 :)
몇가지 커멘트 남겼으니 확인 부탁드려요!
리뷰 빡빡하게 한다고 뒤에서 너무 욕 하지 말아주세요...ㅠㅠ

Comment on lines +42 to +43
menuValidator.validateCreate(request.getPrice(), menuProducts);
final Menu menu = new Menu(request.getName(), request.getPrice(), menuGroup, menuProducts);

Choose a reason for hiding this comment

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

Suggested change
menuValidator.validateCreate(request.getPrice(), menuProducts);
final Menu menu = new Menu(request.getName(), request.getPrice(), menuGroup, menuProducts);
final Menu menu = new Menu(request.getName(), request.getPrice(), menuGroup, menuProducts);
menu.register(menuValidator);

위와 같은 방식은 어떨까요?
Service Layer에서 validator를 직접적으로 호출한 뒤 생성하는 것도 문제는 없으나, 메뉴 등록에 대한 행위를 명확히 나타낸다는 점에서 위 방식도 괜찮다고 생각이 드네요~

의견일 뿐 반영하지 않으셔도 괜찮고 성하의 생각만 말씀해 주시면 감사하겠습니다 :)

Copy link
Author

@sh111-coder sh111-coder Oct 26, 2023

Choose a reason for hiding this comment

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

오! 이렇게 할 수도 있군요!!! 다양한 방법 알려주셔서 감사합니다 ㅎㅎㅎ

말랑이 Suggestion 해주신 코드에서는 Menu를 생성한 후에 MenuValidator로 검증을 수행하는데요!
저는 MenuValidator에서 Create 검증을 수행한 후에 Menu가 생성되는게 좋다고 생각합니다!!
그래서 아예 new Menu로 생성할 때 Validator를 넘겨서 검증 & 생성이 같이 되게 하려고 했었는데,
파라미터가 많아져서 조금 찝찝하더라구요 ㅠㅠ
그래서 기존 코드를 바꾸지 않으려고 하는데 괜찮을까요?!

Choose a reason for hiding this comment

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

넹 괜찮습니다~

Comment on lines +42 to +43
final Long orderTableId = request.getOrderTableId();
orderValidator.validateCreate(orderTableId);

Choose a reason for hiding this comment

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

이 부분도 Menu와 마찬가지로 order.place(orderValidator)의 느낌으로 바꿔줄 수 있을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 Menu와 동일하게 생성 & 검증을 동시에 하려고 했는데 파라미터가 많아져서
그대로 놔두게 되었습니다!!

savedOrder.confirmOrderLineItem(menu, orderLineItemRequest.getQuantity());
final OrderLineItem orderLineItem = new OrderLineItem(menu.getName(), menu.getPrice(),
orderLineItemRequest.getQuantity());
orderLineItem.confirmOrder(savedOrder);

Choose a reason for hiding this comment

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

지난 피드백으로 OrderLineItem에서 Order를 모르게끔 하는 피드백을 드렸는데 혹시 일부러 적용하지 않으신 거라면 이유가 있으실까요~?

Copy link
Author

Choose a reason for hiding this comment

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

앗 죄송합니다 ㅜㅜ 제가 리뷰를 놓친 것 같아요!!

기본적으로 저는 단방향은 다대일 단방향을 사용하고, 양방향을 할 때는 1쪽에 연관관계를 설정해서 하는 편이었는데요!

그래서 이렇게 양방향을 하는 이유로는 1쪽의 엔티티에서 다쪽의 엔티티 리스트를 사용하는 경우가 많을 때 양방향을 걸어주게 되었습니다!!

Order 패키지 안에 Order와 OrderLineItem를 같이 넣음으로써 패키지 의존을 해결하고
같은 패키지 안에서의 양방향 의존은 괜찮다고 판단해서 남겨놨던 것 같네요 ㅠㅠ

다시 생각해보니 제이슨이 해주신 DDD 강의에서 애그리거트 루트가 아닌 엔티티에 접근할 때는
애그리거트를 통해 접근하도록 해서 캡슐화를 지켰던 것이 기억이 나네요!!

그래서 OrderLineItem에서 Order로의 의존을 끊고 Order에서만 OrderLineItem을 접근하도록
일대다 단방향으로 리팩토링하도록 하겠습니다!

Comment on lines +42 to 43
orderTableValidator.validateChangeEmpty(orderTableId, savedOrderTable);
savedOrderTable.changeEmptyStatus(request.getEmpty());

Choose a reason for hiding this comment

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

이부분도 changeEmptyStatus()에 validator를 전달하여 검증과 변경이 한번에 이루어지게 해보는 것도 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

이 부분은 변경 시 Validator를 넘겨서 검증 & 변경이 한번에 되도록 해도 괜찮다고 생각했는데요!!
그런데 앞에서 생성 & 검증하는 부분 구현 시 Service에서 Validator를 사용했기 때문에
일관성을 유지하기 위해 변경하지 않는 방향으로 결정했습니다!!

Choose a reason for hiding this comment

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

넹 괜찮습니다~
이 부분에 대해서 제가 느끼는 바를 따로 커멘트로 드릴게요~!

Comment on lines 38 to 39
tableGroupValidator.validateCreate(savedOrderTables, orderTableRequests.size(), savedOrderTables.size());
final TableGroup savedTableGroup = tableGroupRepository.save(TableGroup.create());

Choose a reason for hiding this comment

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

이 부분도 tableGroup.grouping(tables, tableGroupValidator) 등도 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 �Menu과 동일하게 생성 & 검증 시 파라미터가 많아져서 그대로 놔두게 되었습니다!!

@@ -47,24 +44,15 @@ public TableGroupResponse create(final TableGroupCreateRequest request) {

private void associateOrderTable(final List<OrderTable> savedOrderTables, final TableGroup savedTableGroup) {
for (final OrderTable savedOrderTable : savedOrderTables) {
TableGroupValidator.validateOrderTableStatus(savedOrderTable);
savedOrderTable.confirmTableGroup(savedTableGroup);
savedOrderTable.updateTableGroupId(savedTableGroup.getId());

Choose a reason for hiding this comment

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

TableGroup은 OrderTable로 코드상으로 드러나는 의존성이 존재하며,
OrderTable의 경우에는 간접참조이긴 하나, tableGroupId라는 필드명을 통해 논리적으로 양방향 의존성이 잡힌 것 같아요~
이런 문제점이 왜 발생했고, 어떻게 해결할 수 있을까용?

Copy link
Author

Choose a reason for hiding this comment

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

기존에는 OrderTable과 TableGroup이 생명 주기가 다르다고 생각해서 패키지를 분리했었는데,
비즈니스적으로 OrderTable과 TableGroup이 비슷한 도메인이라고 생각해서 패키지를 묶어서 해결했습니다!

이 과정에서 OrderTable이 TableGroup을 간접 참조하던 것을 직접 참조로 변경했습니다!

기존에 TableGroupValidator에서 Order를 참조하고 있었는데,
TableGroup이 OrderTable 패키지로 통합되면서 OrderTable -> Order의 의존이 다시 생겨서
TableGroupValidator를 인터페이스로 만들어서 Order 패키지에서 구현하도록 의존성을 역전시켜서
의존성을 해결했습니다!

Copy link

@shin-mallang shin-mallang left a comment

Choose a reason for hiding this comment

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

3단계 미션 고생하셨습니다 성하~~
appove는 하겠지만, 여러 피드백에 대한 공통적인 커멘트로 하고싶은 말이 있어서 이곳에 남깁니다!

특정 행위에 대한 검증이 필요할 때,
domain.something(validator);

위와 같이 작성하는 것이 아닌

validator.validate();
domain.something();
으로 작성하는 경우 발생할 수 있는 문제점에 대한 내용입니다!
(지극히 제 생각이므로 적당히 걸어 들어주시면 감사하겠습니다)

잘 아시겠지만 도메인은 기본적으로 여러 service에서 사용될 수 있습니다.
비즈니스 로직을 service가 아닌 도메인 객체에서 구현함으로써, 다양한 service에서 동일한 비즈니스 로직에 대한 중복 구현 없이 재활용할 수 있는 것이라 생각해요!

성하가 작성해주신 OrderTable 구조에서도 다른 서비스에서 Table에 대한 update가 필요하다면 validator와 table의 두 메서드를 각각 호출함으로써 코드의 중복 없이 테이블의 수정 기능을 사용할 수 있겠죠~

그런데 이렇게 업데이트라는 하나의 행위를 위해 '여러' 도메인 객체의 메서드를 '각각 순서에 맞게' 실행시켜 주어야 한다는 점이 개발자로 하여금 고려해야 할 것도 많아지고, 실수가 생길 확률이 굉장히 높아질 것 같아요!

지금의 밸리데이터와 같운 클래스들이 많아졌을 때 각각 메서드 호출 전후로 무엇인가를 실행시켜 주어야 한다는 점을 처음 코드를 보는 개발자가 잘 파악할 수 있을까요?
실수로 빼먹을 가능성도 높아질 것 같아요.

위에서 menu.register(validator);
order.place(validator); 등을 제안한 이유도 같은 문맥인데요,

이러한 클래스들이 수십, 수백개가 되었을 때 각각의 클래스들을 활용함에 있어서, 검증기를 써야하는지, 써야한다면 어떤 순서로 써야하는지 등을 파악하기 쉬울까요??

결국 이것들이 개발하는 개발자가 인지해야 하는 것들을 늘림으로써, 개발자들이 힘들어질 것 같다눈 생각이 들어요~

그에 비해
update 시 validator를 인자로 받게 되면, 따로 인식하지 않더라도 업데이트 시 검증기가 필요하구나를 바로 알 수 있어서 개발자가 생각하고 있어야 하는 부분을 덜어줄 수 있기에 장점을 가진다 생각합니다 :)

approve는 했지만 이에 대해서 어떻게 생각하는지 성하의 의견이 듣고싶군요 ㅎㅎ
말 많아서 죄송합니다 :(
3단계 미션 너무 고생하셨어요!!

@shin-mallang shin-mallang merged commit 835f927 into woowacourse:sh111-coder Oct 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants