[또링] 2,3단계 - 점진적인 리팩또링 미션 제출합니다. #96
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* docs: 기능 목록 추가 - README.md에 기능 목록 추가 * test: TableServiceTest 작성 - TableService의 테스트 작성 * test: TableGroupService test 작성 - TableGroupService에 대한 테스트 작성 * test: TableService test 보강 * test: ProductServiceTest 작성 - refactoring을 위해 productServiceTest 작성 * test: MenuGroupService test 작성 - refactoring을 위한 MenuGroupServiceTest 작성 * test: MenuService test 작성 - refactoring을 위한MenuServiceTest 작성 * test: OrderService test 작성 - refactoring을 위한 OrderServiceTest 작성 * refactoring: test refactoring - integration test를 만들어 중복 제거 * refactoring: table service를 integration test로 변경 - table service를 slice test에서 integration test로 변경 * refactoring: test refactoring - parameterized test의 source 변경 * docs: README.md 변경 - README.md에 ERD 추가 * refactoring: test에서 저 수준 모듈을 사용하도록 refactoring * refactoring: 부모 test 이름 변경
jnsorn
changed the title
phs1116
suggested changes
Dec 13, 2020
| import kitchenpos.menuproduct.model.MenuProduct; | ||
|
|
||
| public class MenuVerifier { | ||
| public static void validateMenuPrice(BigDecimal price, List<MenuProduct> menuProducts, List<Product> products) { |
There was a problem hiding this comment.
로직이 상당히 복잡한데,
MenuProduct와 Product의 정보를 가지고있는 또다른 자료구조를 만들고, (혹은 이를 매핑하고있는 자료구조)
그 자료구조 스스로가 전체 가격을 판단하도록 해보면 어떨까요?
그 부분은 또 일급컬렉션으로 분리해보면 좋을 것 같아요.
Comment on lines
+19
to
+42
| public static void validateOrderCreation(List<OrderLineItem> orderLineItems, int savedMenuCount, | ||
| OrderTable orderTable) { | ||
| validateMinimumMenuCount(orderLineItems); | ||
| validateMenuExistence(orderLineItems, savedMenuCount); | ||
| validateEmptyTable(orderTable); | ||
| } | ||
|
|
||
| private static void validateEmptyTable(OrderTable orderTable) { | ||
| if (orderTable.isEmpty()) { | ||
| throw new IllegalArgumentException("비어있는 테이블에는 주문할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| private static void validateMenuExistence(List<OrderLineItem> orderLineItems, int savedMenuCount) { | ||
| if (orderLineItems.size() != savedMenuCount) { | ||
| throw new IllegalArgumentException("존재하지 않는 메뉴로 주문할 수 없습니다."); | ||
| } | ||
| } | ||
|
|
||
| private static void validateMinimumMenuCount(List<OrderLineItem> orderLineItems) { | ||
| if (CollectionUtils.isEmpty(orderLineItems)) { | ||
| throw new IllegalArgumentException("주문은 1개 이상의 메뉴를 포함해야 합니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
대부분이 일급컬렉션으로 분리한다면 객체 스스로가 책임을 지고 판단할 수 있도록 할 수 있을 것 같아요. :)
| final Long orderId = savedOrder.getId(); | ||
| final List<OrderLineItem> savedOrderLineItems = new ArrayList<>(); | ||
| for (final OrderLineItem orderLineItem : orderLineItems) { | ||
| orderLineItem.changeOrderId(orderId); |
There was a problem hiding this comment.
changeOrderId도 좋지만,
savedOrder.requestOrder(orderItems) 같은 형태로 조금 더 도메인이 드러나게 만들어줄수도 있을 것 같네요. :)
| .orElseThrow(() -> new IllegalArgumentException("존재하지 않는 주문번호입니다.")); | ||
|
|
||
| order.changeOrderStatus(orderStatus); | ||
| Order savedOrder = orderRepository.save(order); |
There was a problem hiding this comment.
JPA 에서는 트랜잭션 내에서 가져온 데이터는 더티체킹에 의해서 굳이 저장 안해도 자동으로 업데이트가 되요. :)
관련해서 검색해서 보시는것도 좋을 것 같아요.
Comment on lines
+43
to
+46
| if (OrderStatus.COMPLETION.equals(this.orderStatus)) { | ||
| throw new IllegalArgumentException("이미 결제가 끝난 주문은 변경할 수 없습니다."); | ||
| } | ||
| this.orderStatus = orderStatus; |
| final List<Order> savedOrders = orderRepository.findAllByOrderTableId(orderTableId); | ||
| OrderVerifier.validateNotCompleteOrderStatus(savedOrders); | ||
|
|
||
| savedOrderTable.changeEmpty(isEmpty); |
There was a problem hiding this comment.
changeEmpty인데 empty라는 인자를 또 받는게 부자연스럽네요. 그냥 setter지만 이름만 다른 느낌이네요.
Comment on lines
+10
to
+25
| public static void validate(List<Long> orderTableIds, int savedSize) { | ||
| validateMinimumTableCount(orderTableIds); | ||
| validateTableExistence(orderTableIds, savedSize); | ||
| } | ||
|
|
||
| private static void validateMinimumTableCount(List<Long> orderTableIds) { | ||
| if (CollectionUtils.isEmpty(orderTableIds) || orderTableIds.size() < MINIMUM_TABLE_COUNT) { | ||
| throw new IllegalArgumentException("그룹 지정은 테이블의 수가 2보다 커야 합니다."); | ||
| } | ||
| } | ||
|
|
||
| private static void validateTableExistence(List<Long> orderTableIds, int savedSize) { | ||
| if (orderTableIds.size() != savedSize) { | ||
| throw new IllegalArgumentException("존재하지 않는 테이블은 그룹지정할 수 없습니다."); | ||
| } | ||
| } |
There was a problem hiding this comment.
이것도 일급컬렉션으로 분리하고 객체 스스로가 책임을 가지도록 해보아요!
| import kitchenpos.menugroup.model.MenuGroup; | ||
| import kitchenpos.menugroup.repository.MenuGroupRepository; | ||
|
|
||
| class MenuGroupServiceTest extends ServiceTest { |
| @@ -0,0 +1,57 @@ | |||
| package kitchenpos.ordertable.ui; | |||
There was a problem hiding this comment.
ui보다는 presentation layer(표현 계층)이 더 적합할거같네요. :)
혹은 그냥 web, api나 interfaces로 표현하기도 해요.
| } | ||
|
|
||
| @Transactional | ||
| public TableGroupResponseDto create(final TableGroupCreateRequestDto tableGroupCreateRequestDto) { |
There was a problem hiding this comment.
DTO들 자체는 엄밀하게 말하면 presentation 영역의 객체인데요,
어플리케이션 레이어, 혹은 서비스 레이어에서 dto의 객체를 사용하는건 의존성이 양방향으로 보일 수도 있어요.
보통은 같은 객체라도 두개를 격리시켜서도 많이 사용합니다.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
안녕하세요! 화투님 🙂 리뷰해주셔서 감사합니다!
이번 PR에서 구현한 내용
2단계
3단계
리뷰 해주시면 열심히 고민해서 반영해보도록 하겠습니다! 🙏🏻