[레거시 코드 리팩터링 - 2단계] 성하(김성훈) 미션 제출합니다. #557
Conversation
There was a problem hiding this comment.
안녕하세요 성하!
2단계 미션도 잘 구현해 주셨네요!
제가 예비군 때문에 시간이 너무 없어서..(아직 저도 2단계 미션도 못 끝낸 상태라..)
예비군 끝나고 리뷰 봐드리기가 조금 애매해서 조금조금씩 남기도록 할게요!
(양해 부탁드립니다..!)
이번 단계에서 도메인 내부로 비즈니스 로직을 옮겨주신 부분 잘 보았슴니다:)
그치만 조금만 더 개선되면 훨씬 좋을 것 같아서 앞부분 코드에 몇몇 커멘트 남겼으니 확인 부탁드려요!
뒤에는 아직 못 읽어서 확인하지는 못했지만 아마 비슷하게 코드를 작성하셨을 것 같아 전반적으로 변경해주셨으면 하는 부분에 대해 이곳에 적겠습니다!
현재 도메인 모델의 생성자나 메서드를 통해 비즈니스 로직을 옮겨주신 부분은 너무 좋지만, 일시적으로 불변식이 깨지는 오류가 발생할 여지가 많은 것 같습니다.
그 이유는 생성 혹은 변경 작업과 validation 작업이 분리되어 있기 때문인데요, 이러한 부분을 하나의 단위로 뭉쳐주시는 것은 어떨까 싶어요!
예를 들어 가격 변경 시 가격이 0원 이상이어야 한다면,
menu.changePrice();
menu.validatePrice();
대신
menu.changePrice() 메서드 내부에서 validate가 우선적으로 실행되어 불변식이 깨지지 않도록 하는 느낌입니다!
아 추가적으로 몇몇 검증 로직이 도메인 레이어가 아닌 서비스 레이어에서 진행되고 있는데 이 부분은 Validtor를 도메인 패키지에 두어 이를 사용함으로써 비즈니스 규칙을 도메인 내부에서만 관리하게 해보는언 어떨까요?
뒷 부분에 대해서는 이따 또 시간이 될 때 추가로 리뷰 남겨드릴 테니, 우선 말씀드린 부분부터 반영해주시면 좋을 것 같아요!
궁금한 점 있으시면 커멘트나 dm 주세요:)
| final Menu savedMenu = menuRepository.save(menu); | ||
|
|
||
| associateMenuProduct(request.getMenuProducts(), savedMenu); | ||
| menu.verifyPrice(); |
There was a problem hiding this comment.
price verify 하는 과정을 생성자 내부로 옮기는 것은 어떨까요?
현재 상태라면 Menu 객체의 불변식이 지켜지지 않은 채로 객체가 생성되고 있으니까요!
There was a problem hiding this comment.
현재 verifyPrice() 로직에서 연관관계 설정이 완료된 List<MenuProduct>를 내부적으로 사용하기 때문에
verify() 로직은 Menu에서 MenuProduct의 연관관계 설정이 완료된 후에 실행되어야 하는 것 같아요!
연관관계 설정은 생성 시가 아니라 편의 메소드를 통해 하는 것이 좋은 것 같아서 일단 놔두게 되었습니다!
There was a problem hiding this comment.
저는 다음과 같은 방식을 의미하였습니다!
- 우선 MenuProduct에서는 Menu에 대한 의존성을 뺍니다. (@onetomany에서 One을 연관관계의 주인으로 만드는 방식)
- 이후 다음과 같은 형식으로 검증합니다.
public Menu(String name, BigDecimal price, MenuGroup menuGroup, List<MenuProduct> menuProducts) {
validateCreate(price, menuGroup, menuProducts);
this.name = name;
this.price = price;
this.menuGroup = menuGroup;
this.menuProducts = menuProducts;
}
private void validateCreate(BigDecimal price, MenuGroup menuGroup, List<MenuProduct> menuProducts) {
// 이런저런 검증 로직들
}이렇게 하면 생성 시 불변식 검증이 가능하지 않을까요?
제 코드가 정답은 아니지만, 감이 잘 잡히지 않으시면 제걸 한번 확인해 보셔도 좋을 것 같아요!
There was a problem hiding this comment.
의존성 관련한 부분은 3단계때 진행하려고 했는데
불변식을 유지하려면 의존성을 바꿔야하네요 ㅠㅠㅠ
우선 일대다 단방향은
- 매핑한 객체가 관리하는 외래 키가 다른 테이블에 있다.
- 연관 관계 처리를 위한 UPDATE SQL을 추가로 실행해야 한다.
해당 문제가 있다고 알고 있는데요!
지금 상황에서는 UPDATE 쿼리가 안 날라가더라구요.. 정확한 문제를 파악을 못해서 시간을 쓰지않고 일단
리팩토링 했습니다!
그래서 결론적으로는 Trade-Off가 있을 것 같은데
- 불변식을 유지하기 위해서 위와 같은 일대다 단방향을 유지한다
- 일대다 단방향의 단점이 크다고 생각해서 불변식이 유지되지 않더라도 의존성을 끊지 않는다.
저는 보통 2번으로 진행했었어서 이번에는 1번으로 진행해보고 싶어서 리팩토링했습니다!!
| final Product product = productRepository.findById(menuProductRequest.getProductId()) | ||
| .orElseThrow(NotFoundProductException::new); | ||
| final MenuProduct menuProduct = new MenuProduct(product, menuProductRequest.getQuantity()); | ||
| menuProduct.confirmMenu(savedMenu); |
There was a problem hiding this comment.
MenuProduct 같은 경우 생명주기가 Menu와 완전히 동일하다고 생각합니다~
Service Layer에서 Menu와 별개로 저장하는 것이 아닌 jpa의 기능을 활용해서 Menu 저장 시 MenuProduct들도 함께 저장하게 해보는 것은 어떨까요?
그리고 MenuProduct의 생성 역시 Menu 내부에서 Product를 받아 생성하게 해보면 어떨까요?
(그리고 지금 이 상태라면 MenuProduct가 잘 저장이 되나요..?
연관관계 주인이 Menu인데다, cascade 설정도 없어서 안될 것 같은데 아닌가요..?)
There was a problem hiding this comment.
포트스트맨으로 테스트해보니 저장이 안되네요 ㅠㅠ Repository.save를 깜빡한 것 같아요!!
역시 말랑.. 꼼꼼하십니다,,, 소프트콘 100개 적립
Service Layer에서 Menu와 별개로 저장하는 것이 아닌 jpa의 기능을 활용해서 Menu 저장 시 MenuProduct들도 함께 저장하게 해보는 것은 어떨까요?
이렇게 하는 이유는 서비스에서 사용하지 않아도 되는 Repository 사용을 줄이기 위함일까요?
엔티티단에서 cascade 옵션을 활용해서 Menu 저장 시 MenuProduct를 저장하게 했습니다!!
이렇게 하니까 MenuProduct를 영속화하는 책임이 MenuService가 아니라 Menu가 되면서
도메인 로직으로 변경되는? 느낌이 있긴 한 것 같아요!
그리고 MenuProductRepository가 Service에서 사용되지 않으면서
제이슨 DDD 강의에서 들었던 도메인 외부에서는 애그리거트 루트(Menu)의 Repository만
참조해서 자식 엔티티인 MenuProduct를 접근하려면
Menu를 거쳐 접근하는 느낌이 드는 것 같네요!!
MenuProduct의 생성 역시 Menu 내부에서 Product를 받아 생성하게 해보면 어떨까요?
이 부분은 제안해주신 의도가 외부(Service)에서 Menu만 알도록 하는 것이라고 이해했습니다!
그래서 menu.confirmMenuProduct 메소드를 구현하고
그 내부에서 MenuProduct를 생성하고, 연관관계를 맺도록menuProduct.confirmMenu()를 호출하였습니다!
추가로, menuProduct.confirmMenu()가 외부에서 호출되지 않고 Menu에서만 호출되도록
Menu와 MenuProduct를 같은 패키지로 위치하게 하고 접근 제어자를 default로 주고 싶었지만
현재는 도메인 사이의 의존성을 고려하지 않고 기존 코드의 의존성대로 진행하는게 맞는 것 같아서 public으로 유지헀습니다!
++ 그런데 리팩토링하면서 들었던 생각은 Menu가 MenuProduct를 생성하게 되면서, Product를 의존하게 되었는데요!
기존에는 MenuProduct만 의존하고 MenuProduct가 의존하는 Product에 대해서는 Menu가 몰라도 됐었습니다!
그런데 이렇게 리팩토링하면서 Menu가 Product를 알게 되는데 이에 대해서는 어떻게 생각하시는지 궁금합니다!!
There was a problem hiding this comment.
이렇게 하는 이유는 서비스에서 사용하지 않아도 되는 Repository 사용을 줄이기 위함일까요?
이 이유도 분명 있긴 하지만, 정확히는 생명주기가 같은 도메인 객체의 관리를 편하게 하기 위해서라고 할 수 있겠습니다!
관리 포인트가 늘어나면 그만큼 복잡도가 증가할 것이라 생각해요!
추가로, menuProduct.confirmMenu()가 외부에서 호출되지 않고 Menu에서만 호출되도록
Menu와 MenuProduct를 같은 패키지로 위치하게 하고 접근 제어자를 default로 주고 싶었지만
현재는 도메인 사이의 의존성을 고려하지 않고 기존 코드의 의존성대로 진행하는게 맞는 것 같아서 public으로 유지헀습니다!
++ 그런데 리팩토링하면서 들었던 생각은 Menu가 MenuProduct를 생성하게 되면서, Product를 의존하게 되었는데요!
기존에는 MenuProduct만 의존하고 MenuProduct가 의존하는 Product에 대해서는 Menu가 몰라도 됐었습니다!
그런데 이렇게 리팩토링하면서 Menu가 Product를 알게 되는데 이에 대해서는 어떻게 생각하시는지 궁금합니다!!
이부분은 제가 착각했네요 ㅠㅠ
Menu에서 굳이 MenuProduct를 생성할 필요는 없었던 것 같습니다..!!
말씀해주신 의존성 부분도 문제가 될 뿐더러, 이렇게 했을 때 얻을 수 있는 장점이 없는 것 같아요..!
| savedOrderLineItems.add(orderLineItemDao.save(orderLineItem)); | ||
| } | ||
| savedOrder.setOrderLineItems(savedOrderLineItems); | ||
| final Order order = new Order(orderTable, OrderStatus.COOKING, LocalDateTime.now()); |
There was a problem hiding this comment.
이 위에있는 검증 로직들은 OrderValidator를 만들어 도메인 레이어에서 검증하게 해보는 것은 어떨까요?
추가로 Order가 최초 생성되는 경우 Cooking 상태가 되는 것과, 주문 시간이 now인 것을 생성자 혹은 정적 팩터리 메서드를 통해 도메인의 책임으로 넘겨보는 것은 어떨까요?
There was a problem hiding this comment.
이 위에있는 검증 로직들은 OrderValidator를 만들어 도메인 레이어에서 검증하게 해보는 것은 어떨까요?
이 부분도 검증 책임을 OrderValidator에게 위임함으로써 책임 분리가 더 세세하게 이루어진다는 점에서 좋은 것 같아요!
그래서 일단은 2개 검증을 모두 OrderValidator를 생성해서 위임했습니다!
그런데 2가지 검증 중에서 첫 번째 검증이 다음과 같은데요!
if (orderLineItemRequests.size() != menuRepository.countByIdIn(menuIds)) {
throw new OrderException.NotFoundOrderLineItemMenuExistException();
}
이 부분에서는 Request에 관한 로직이 관여함으로써 Validator에서 int로 size를 받아서 검증을 하고 있어요!
이렇게 int로 전달해서 Request와 직접적인 관련은 없지만 간접적으로 Request의 size를 받는다는 점에서
OrderValidator 객체에 도메인이 아닌 Request 관련 로직이 들어간다고 생각이 들어서 조금 찝찝했는데요!
말랑의 생각은 어떤지 궁금합니다!!
추가로 Order가 최초 생성되는 경우 Cooking 상태가 되는 것과, 주문 시간이 now인 것을 생성자 혹은 정적 팩터리 메서드를 통해 도메인의 책임으로 넘겨보는 것은 어떨까요?
미션 진행 시에도 생각했었는데 테스트 시에 OrderStatus를 변경하는 코드가 있어서
해당 테스트를 하려면 setter를 사용해야한다고 생각해서 진행하지 않았었어요!!
근데 다시보니 이미 Status를 바꾸는 비즈니스 로직이 존재해서 해당 로직인 changeStatus를 사용하면 되더라구요!!
그래서 정팩메로 저장 시 기본 상태를 저장하도록 리팩토링 진행했습니다!! 감사합니다 말랑 ㅎㅎㅎ 👍🏻👍🏻👍🏻👍🏻
There was a problem hiding this comment.
이 부분에서는 Request에 관한 로직이 관여함으로써 Validator에서 int로 size를 받아서 검증을 하고 있어요!
이렇게 int로 전달해서 Request와 직접적인 관련은 없지만 간접적으로 Request의 size를 받는다는 점에서
OrderValidator 객체에 도메인이 아닌 Request 관련 로직이 들어간다고 생각이 들어서 조금 찝찝했는데요!
말랑의 생각은 어떤지 궁금합니다!!
저도 이렇게 하는 방식은 살짝 찜찜한 느낌이 있는 것 같아요~
그런데 저 검증 로직이 반드시 필요할까요?
어차피 OrderLineItem을 만들 때 Menu를 찾아와야 하는데, 만약 메뉴 Id가 잘못된 것이 있다면 그쪽에서 터지지 않을까요?
위 검증 로직 자체를 빼는 것은 어떨까요~?
|
안녕하세요 말랑!!! 빠르고 꼼꼼한 리뷰 감사합니다!! 이번에는 말랑의 리뷰대로 Validate 로직을 Validator를 만들어서 책임을 위임하거나, 진행하면서 몇 가지 궁금한 부분들을 말랑의 코멘트에 답변하면서 질문도 해버렸어요 ㅎㅎㅎㅎ 리뷰 잘 부탁드립니다!! |
There was a problem hiding this comment.
안녕하세요 성하~
처음 리뷰에서 제가 말을 정확하게 하지 못했던 부분이 있는 것 같아서 죄송하네요 ㅠㅠ
이번에는 좀 더 구체적으로 말씀드릴게요!
(먼제 제가 말씀드리는 방식이 정답은 아니지만, 이런 방식도 있다 느낌으로 학습해 보셨으면 해서 말씀드리는 거예요!)
- 모든 검증을 Domain 객체 내부에서만 진행하게 해보시면 좋을 것 같아요!
- Repository가 필요하다면 Validator를 사용하고, 그렇지 않다면 도메인 객체 내부에서 검증을 진행하시면 좋을 것 같습니다.
- 생명주기가 완전히 동일한 객체에 대해서는 @onetomany와, 연관관계의 주인으로 One을 지정함으로써, One에서 Many를 관리할 수 있도록 바꿔보시는 것도 좋을 것 같아요!
- 이렇게 설정하면 Service 단의 복잡도를 감소시킬 수 있을 것이라 생각합니다. 또한 지금 문제가 되었던 생성 시 불변식이 지켜지지 않는 문제들도 해결할 수 있을 것 같아요!
(두번째 Request Change라 죄송하네요 ㅠㅠ 이부분만 적용되면 바로 Approve 드릴게요!)
| .collect(Collectors.toList()); | ||
| OrderValidator.validateOrderLineItemSize(orderLineItemRequests.size(), menuRepository.countByIdIn(menuIds)); |
There was a problem hiding this comment.
OrderValidator를 static하게 사용할 것이라면, 굳이 사용할 필요가 없는 것 같아요..!!
저 같은 경우 Validator는 도메인 로직 검증을 위해 Repository의 사용이 필요한 경우 사용합니다..!!
이런 경우라면 Validator를 따로 두지 않고 도메인 객체 내부(Order)에서 처리할 것 같아요!
답변으로 남겼지만 OrderValidator에서 validateOrderLineItemSize()를 제거하고, 나머지 검증을 Order 객체에게 맡겨보는 것은 어떨까요?
|
|
||
| for (final Order order : orders) { | ||
| order.setOrderLineItems(orderLineItemDao.findAllByOrderId(order.getId())); | ||
| private void associateOrderLineItem(final List<OrderLineItemRequest> orderLineItemRequests, |
There was a problem hiding this comment.
@onetomany와 연관관계의 주인을 One을 두는 방식을 사용하면 해당 메서드도 필요없게 됩니다!
| .map(OrderTableFindRequest::getId) | ||
| .collect(Collectors.toUnmodifiableList()); | ||
| final List<OrderTable> savedOrderTables = orderTableRepository.findAllByIdIn(orderTableIds); | ||
| TableGroupValidator.validateOrderTableSize(orderTableRequests.size(), savedOrderTables.size()); |
There was a problem hiding this comment.
이 부분도 OrderValidator와 비슷한 피드백을 드릴게요!
|
|
||
| final List<Long> orderTableIds = orderTables.stream() | ||
| .map(OrderTable::getId) | ||
| .collect(Collectors.toList()); | ||
|
|
||
| if (orderDao.existsByOrderTableIdInAndOrderStatusIn( | ||
| orderTableIds, Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))) { | ||
| if (orderRepository.existsByOrderTableIdInAndOrderStatusIn( |
There was a problem hiding this comment.
이러한 검증 로직이 Validator 내부로 이동해야 Service Layer -> Domain Layer로 도메인 규칙이 옮겨질 것 같아요!
| // | ||
| // @PutMapping("/api/orders/{orderId}/order-status") | ||
| // public ResponseEntity<Order> changeOrderStatus( | ||
| // @PathVariable final Long orderId, | ||
| // @RequestBody final Order order | ||
| // ) { | ||
| // return ResponseEntity.ok(orderService.changeOrderStatus(orderId, order)); | ||
| // } |
| public Long getId() { | ||
| return id; | ||
| @OneToMany(mappedBy = "tableGroup") | ||
| private final List<OrderTable> orderTables = new ArrayList<>(); |
There was a problem hiding this comment.
현재 OrderTable의 개수가 2개 이상이어야 한다는 검증을 도메인이 아닌 DTO에서 하고 있는 것 처럼 보여요~
이런 부분을 어떻게 개선할 수 있을까요?
|
|
||
| for (final Menu menu : menus) { | ||
| menu.setMenuProducts(menuProductDao.findAllByMenuId(menu.getId())); | ||
| private void associateMenuProduct(final List<MenuProductRequest> menuProductRequests, final Menu savedMenu) { |
There was a problem hiding this comment.
우선 MenuProduct에서는 Menu에 대한 의존성을 뺍니다. (@onetomany에서 One을 연관관계의 주인으로 만드는 방식)
이를 적용하면 해당 메서드도 제거할 수 있습니다~
| OrderValidator.validateOrderTable(orderTable); | ||
|
|
||
| if (orderTable.isEmpty()) { | ||
| throw new OrderException.CannotOrderStateByOrderTableEmptyException(); | ||
| } | ||
|
|
||
| order.setOrderTableId(orderTable.getId()); | ||
| order.setOrderStatus(OrderStatus.COOKING.name()); | ||
| order.setOrderedTime(LocalDateTime.now()); | ||
|
|
||
| final Order savedOrder = orderDao.save(order); | ||
| final Order order = Order.from(orderTable); |
There was a problem hiding this comment.
이 부분도 Order의 생성자에서 검증하도록 하면 더 좋을 것 같아요~
여기서는 굳이 Validator를 쓸 필요가 없는 것 같습니다!
| if (savedOrderTable.isExistTableGroup()) { | ||
| throw new OrderTableException.AlreadyExistTableGroupException(); | ||
| } | ||
|
|
||
| if (orderDao.existsByOrderTableIdAndOrderStatusIn( | ||
| orderTableId, Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))) { | ||
| if (orderRepository.existsByOrderTableIdAndOrderStatusIn( | ||
| orderTableId, Arrays.asList(OrderStatus.COOKING, OrderStatus.MEAL))) { | ||
| throw new CannotChangeEmptyStateByOrderStatusException(); | ||
| } | ||
|
|
||
| savedOrderTable.setEmpty(orderTable.isEmpty()); | ||
| savedOrderTable.changeEmptyStatus(request.getEmpty()); |
There was a problem hiding this comment.
이러한 검증 로직들도 changeEmptyStatus() 메서드 내부로 모두 옮겨보는 것은 어떨까요?
|
안녕하세요 말랑!! DM으로 얘기 나눴었는데욥!! 지금 시간이 많이 나질 않아서 리뷰 많이 해주셨는데 죄송합니다 ㅠㅠ 😭 |
안녕하세요 말랑!
이번 2단계에서는 서비스 리팩토링을 진행했습니다!
JPA로 마이그레이션하면서 주입해서 테스트하는게 훨씬 생산성이 좋은 것 같더라구요!
그래서 모킹을 제거하고 테스트하게 됐습니다!
2단계도 잘부탁드려요 ㅎㅎㅎ