[레거시 코드 리팩터링 - 3단계] 도기(김동호) 미션 제출합니다. #683
Conversation
|
저는 테스트는 거의 변경하지 않았어요. 다 값검증이고, 블랙박스테스트로 진행했어서 테스트가 문제가 되는일은 없었습니다. |
| @ManyToOne(fetch = LAZY) | ||
| @JoinColumn(name = "product_id") | ||
| private Product product; | ||
| private long productId; |
| @@ -31,9 +27,6 @@ public class Menu { | |||
| @JoinColumn(name = "menu_group_id") | |||
| private MenuGroup menuGroup; | |||
|
|
|||
| @OneToMany(mappedBy = "menu") | |||
| private final List<MenuProduct> menuProducts = new ArrayList<>(); | |||
|
|
|||
| protected Menu() { | |||
There was a problem hiding this comment.
오 아예 MenuProduct에 대한 직접참조도 빼셨군요.
따로 빼신 이유가 있을까요?
| Menu menu = menuRepository.save(new Menu(command.name(), new Price(command.price()), menuGroup)); | ||
|
|
||
| for (MenuProductCreateCommand menuProductCreateCommand : command.menuProducts()) { | ||
| Product product = productRepository.getById(menuProductCreateCommand.productId()); |
There was a problem hiding this comment.
여기서 명시적으로 MenuProductRepository에 save를 하고 있군요.
(여기부터는 지극히 개인적인 의견입니다.)
저는 이 패키지 내부에서 MenuProduct가 aggregateRoot라고 생각하진 않아요.
그래서 MenuProduct같은 도메인은 menu를 통해서 탐색도 되니 직접참조 + JPA의 Cascade 옵션을 통해서 저장했어요.
이런 방법도 있습니당.
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
|
|
||
| public class OrderCreateCommand { |
| for (OrderLineItemCreateCommand orderLineItemCommand : orderLineItemCommands) { | ||
| Menu menu = menuRepository.getById(orderLineItemCommand.menuId()); | ||
| OrderLineItem orderLineItem = new OrderLineItem(order, menu.id(), new Quantity(orderLineItemCommand.quantity())); | ||
| orderLineItemRepository.save(orderLineItem); |
There was a problem hiding this comment.
요기도 Menu, MenuProduct에 남긴 리뷰를 확인해보시면 좋을 것 같아요
|
|
||
| @EventListener | ||
| @Transactional | ||
| public void handle(TableEmptyChangedEvent event) { |
There was a problem hiding this comment.
eventHandler로 검증을 하시는 군요 의존성을 잘 빼신 것 같아요.
메서드 이름을 validate와 같이 동작으로 뒀으면 어땟을까? 싶어요. 핸들은 너무 추상적이더라고요
| if (tableGroupId != null) { | ||
| throw new IllegalStateException("이미 속한 테이블 그룹이 있으면 주문 가능 상태를 변경할 수 없습니다."); | ||
| } | ||
| registerEvent(new TableEmptyChangedEvent(this, empty)); |
There was a problem hiding this comment.
도메인 event를 쓰셨군요. 도메인 이벤트는 jpaRepository에서 save, delete를 호출하는 시점에 event를 발송합니다.
validate하는 로직은 도메인 이벤트로 쓰긴 좋지 않은 것 같아요.
도메인 입장에선 이미 유효성 검증이 끝나고 느끼고, save를 호출한 후에 문제가 생기면 롤백을 하니까요.
만약 로직 사이에 리소스가 많이 드는 작업이 있다면 비효율적이겠죠..?
|
|
||
| TableGroup tableGroup = tableGroupRepository.save(new TableGroup()); | ||
| for (OrderTable orderTable : orderTables) { | ||
| orderTable.group(tableGroup.id()); |
There was a problem hiding this comment.
오 의존성이 TableGroup -> Table 방향이군요. 전 반대로 했는데 도기의 코드를 보니 도기의 방향이 더 나은 것 같네요
안녕하세요 홍실 🧶 그간 잘 지내셨나요??
저는 지원서 작성에 코테 준비에 미션까지.. 하루가 너무 짧네요ㅜㅜ
3단계.. 시간에 쫓겨 많은 걸 희생하여 진행해봤습니다.
무엇을 희생했고, 무엇이 바꼈냐면요
1. ui 패키지와 테스트, Exception 제거
어.. 사실 양심에 찔리기도 하고 이게 맞나 싶지만 현실적으로 시간이 없어 이렇게 결정했습니다.
3단계 요구사항이 의존성을 개선하는 것이기도 해서 선택과 집중을 했다고 말씀드리고 싶어요.
ui랑 custom exception이 없으니까 확실히 service와 domain 계층 간의 의존성에 집중할 수 있었어요.
하지만 테스트가 없으니 내가 짠 코드가 제대로 동작하는지 마음 한 켠이 계속해서 불편하더라구요.
동시에 이번 레거시 코드 리팩터링 미션을 진행하면서 매 단계마다 테스트를 싹 갈아야하는 게 너무 큰일이고 골치아프다는 생각이 들었어요.
제가 테스트를 잘못짠건지? 아니면 당연한 수순인건지? 홍실은 이번 미션 단계별로 진행할 때마다 테스트가 어떠셨나요?
2. Order와 OrderTable 패키지 참조 싸이클 해결
미션을 진행하다보니 이 두 패키지에만 싸이클이 존재하더라구요.
그래서 처음엔 인터페이스를 사용해 의존성 역전으로 해결을 시도해보았다가, 스프링 이벤트도 적용해보고 싶어 이벤트로 해결해보았습니다.
처음 시도해본 노력들이라 제대로 했는지 잘 모르겠네요.
이 부분은 꼭 홍실의 피드백을 받고 싶어요!
그럼 잘 부탁드립니다 🙏
아래는 3단계 진행하면서 도메인과 패키지의 의존성을 그림으로 그려가며 진행해본 결과입니다.
위 AS-IS는 미션 첫 상태의 의존성 상태를 그린 그림입니다.
아래 TO-BE는 현 상태에서 어떻게 개선할 지 구상해본 그림입니다.
두 번째 사진은 개선 이후의 그림입니다.
위는 패키지의 의존성을, 아래는 도메인 레벨의 의존성을 그려보았어요.