[레거시 코드 리팩토링 - 3단계] 다즐(최우창) 제출합니다. #676
Conversation
There was a problem hiding this comment.
안녕하세요 다즐!
리뷰가 늦어서 정말 죄송합니다..😭
여유가 있었더라면 조금 더 이야기 나누고 싶은데
오늘이 마감일이라서 머지할게요!!
저도 아직 3단계 구현 전이라 더 풍부한 리뷰를 드리지 못해서 죄송하네요ㅜㅜ
최대한 양방향 의존을 하지 않았는지에 초점을 맞춰 리뷰했습니다!
아직 양방향 의존이 많이 남아있는데,
이는 4단계 진행하면서 모두 단방향으로 바꾸면서 진행하실 수 있을 것 같아요!
답변은 시간 나실 때 남겨주세요!
그럼 많이 바쁘실텐데 마지막까지 화이팅입니다!! 👍
| List<MenuProductQuantityDto> menuProductQuantities, | ||
| MenuProductMapper menuProductMapper |
There was a problem hiding this comment.
도메인에 MenuProductQuantityDto와 MenuProductMapper를 전달해주면서 도메인 패키지와 어플리케이션 패키지가 양방향 의존이 생겼어요!
패키지 간, 특히 레이어 간의 양방향 의존은 최대한 없도록 하는 게 좋을것 같아요.
추가적으로 생성자에 위 두가지의 클래스가 들어가는 것 자체가 도메인이 서비스에 너무 의존적이라고 느껴져요! 생성자 시그니처만 보고 이해하기도 쉽지 않을 것 같구요!
도메인을 최대한 순수하게 만들어보면 좋을 것 같아요 ㅎㅎ
| List<MenuProductQuantityDto> menuProductQuantities, | ||
| MenuProductMapper menuProductMapper |
There was a problem hiding this comment.
MenuGroup과 Menu의 생명주기가 다르다고 생각이 되는데요!
따라서 Menu와 MenuGroup을 분리해야할 것 같아요. 어떻게 생각하시나요??
| @Embeddable | ||
| public class MenuProductName { |
There was a problem hiding this comment.
이 부분은 예전에 놓쳤었는데, vo라면 equals & hashcode를 재정의해줘야 할 것 같아요!
| @@ -1,7 +1,9 @@ | |||
| package kitchenpos.dto.request; | |||
| package kitchenpos.menu.dto.request; | |||
There was a problem hiding this comment.
dto 패키지를 application과 ui 등등 과 같은 선상에 두신 이유가 있을까요?
지금 구조로는 application과 dto 패키지가 서로 양방향 의존을 하게 되네요!
근데 이 부분은 application의 dto가 없으면 해결되는 부분인 것 같아요.
| List<OrderLineItemQuantityDto> orderLineItemQuantities, | ||
| OrderLineItemMapper orderLineItemMapper, | ||
| OrderValidator orderValidator |
| OrderStatus orderStatus, | ||
| LocalDateTime orderedTime | ||
| LocalDateTime orderedTime, | ||
| OrderValidator orderValidator |
There was a problem hiding this comment.
OrderValidator를 생성자로 전달해주신 이유가 있을까요?
저도 이번에 '도메인 서비스'라는 개념을 알게 되었는데요.
아직 깊게 공부해보지 않아서 잘 모르겠지만,
일단은 도메인 로직을 실행하는 서비스라고 알고있어요.
도메인에서만 해결할 수 없는 검증같은 것들도 도메인 서비스라고 할 수 있을 것 같은데요.
그런 점에서 OrderValidator도 도메인 패키지에 들어가야 할 것 같아요!
그리고 그렇게 해야지만 양방향 의존이 생기지 않아요.
| @Column(name = "table_group_id") | ||
| private Long tableGroupId; |
There was a problem hiding this comment.
OrderTable과 TableGroup을 직접 참조에서 간접 참조로 바꿔주셨군요!
그럼 상위 패키지도 나눌 수 있을 것 같은데 나누지 않은 이유가 있을까요?!
| public class TableValidator { | ||
|
|
||
| private final OrderRepository orderRepository; |
There was a problem hiding this comment.
다즐이 말씀해주신 대로 Order -> Table로 단방향 관계여야하는데 현재는 Order와 Table 패키지가 양방향 의존을 하고 있어요!
| alter table menu_product | ||
| add column name varchar(255); | ||
|
|
||
| alter table menu_product | ||
| add column price decimal(19, 2); |
There was a problem hiding this comment.
제가 요구사항을 제대로 모르고 있었네요😅
메뉴 정보가 변경되더라도 주문 항목이 변경되지 않게 구현한다. 를 위해 필드를 추가해주신거였군요!
| public class MenuProducts { | ||
|
|
||
| @OneToMany(mappedBy = "menu", cascade = CascadeType.PERSIST) | ||
| @OneToMany(fetch = FetchType.EAGER, cascade = CascadeType.PERSIST) | ||
| @JoinColumn(name = "menu_id", nullable = false, updatable = false) | ||
| private List<MenuProduct> menuProducts = new ArrayList<>(); |
안녕하세요 밀리 🤓
3단계는 요구사항을 바탕으로 의존성 분리에 초점을 맞춰 미션을 진행하였습니다!
진행 사항은 크게 아래와 같아요.
추구하고자 했던 의존성 방향은 아래 사진과 같습니다.
처음으로 의존성에 대해 고민하고 직접 구현해봐서 그런지 많이 어려웠던 미션인 것 같네요 🥲
3단계 미션 리뷰도 잘 부탁드립니다 🙇🏻♂️ 많이 배워가도록 할께요 :)