[레거시 코드 리팩터링 - 2단계] 기론(김규철) 미션 제출합니다.#326
Conversation
- add Order domain test
yeon-06
left a comment
yeon-06
left a comment
There was a problem hiding this comment.
안녕하세요 기론~
코드 리뷰가 늦어져서 죄송합니다😂
몇 가지 코멘트를 남겨서 RC로 해두니 확인 부탁드려요~
| @@ -0,0 +1,16 @@ | |||
| package kitchenpos.application.dto; | |||
There was a problem hiding this comment.
기론만의 dto 위치에 대한 기준이 있으실까요?
request, response 모두 application 내부에 있길래 여쭤봅니다 ㅎ.ㅎ
There was a problem hiding this comment.
패키지 의존성을 단방향으로 가져가려고 했어요! application계층에서는 presentation계층을 알지 못하도록이요
| package kitchenpos.application; | ||
|
|
||
| import java.math.BigDecimal; | ||
| import java.util.ArrayList; |
|
|
||
| public class OrderRequest { | ||
|
|
||
| public static class Create { |
There was a problem hiding this comment.
DTO를 inner static 형식으로 만드셨군요😮!
혹시 어떤 장점 때문에 해당 방식을 택하셨나요?
There was a problem hiding this comment.
DTO를 매번 용도에 따라서 클래스를 만들면 관리할 DTO 클래스들이 너무 많아 지더라구요.
그래서 Request 또는 Response에 대한 dto 클래스만 만들고 내부에서 관리하는 편리함이 있었습니다.
| BigDecimal sum = BigDecimal.ZERO; | ||
| for (final MenuProductRequest.Create menuProduct : requests) { | ||
| final Product product = productDao.findById(menuProduct.getProductId()) | ||
| .orElseThrow(IllegalArgumentException::new); | ||
| sum = sum.add(product.multiplyPrice(menuProduct.getQuantity())); | ||
| } |
There was a problem hiding this comment.
stream을 이용해 더 간단하게 풀 수 있지 않을까요? 👀
(제안 사항이며 반영하지 않으셔도 괜찮습니다)
| savedTableGroup.setOrderTables(savedOrderTables); | ||
|
|
||
| return savedTableGroup; | ||
| final TableGroup savedTableGroup = tableGroupDao.save(new TableGroup(LocalDateTime.now())); |
There was a problem hiding this comment.
이건 개선 사항은 아니고 고민해보시면 좋을 것 같아서 코멘트 남깁니다
현재 로직에서는 상관 없지만 서비스 내부 로직에서 LocalDateTime.now()를 사용하면 테스트하기 불편한 상황이 오게 되는데요
어떻게 테스트를 더 편하게 만들 수 있을지 고민해보시면 좋을 것 같아요 ㅎㅎ
There was a problem hiding this comment.
오 연로그 블로그 참고했습니다~ ㅋㅋㅋㅋ 이런 방법이 있었군요!
| if (orderDao.existsByOrderTableIdInAndOrderStatusIn( | ||
| orderTableIds, Arrays.asList(OrderStatus.COOKING.name(), OrderStatus.MEAL.name()))) { | ||
| throw new IllegalArgumentException(); | ||
| throw new IllegalArgumentException("[ERROR] 주문 상태가 COOKING 또는 MEAL일 때는 그룹을 해제할 수 없습니다."); |
| @@ -64,4 +80,8 @@ public List<MenuProduct> getMenuProducts() { | |||
| public void setMenuProducts(final List<MenuProduct> menuProducts) { | |||
There was a problem hiding this comment.
setter말고 좀 더 의미있는 이름을 짓거나 setter를 제거하여 로직을 개선시킬 수 있지 않을까요?
|
|
||
| private void validateExists(List<OrderTable> savedOrderTables) { | ||
| for (final OrderTable savedOrderTable : savedOrderTables) { | ||
| if (!savedOrderTable.isEmpty() || Objects.nonNull(savedOrderTable.getTableGroupId())) { |
There was a problem hiding this comment.
Objects.nonNull 메서드의 사용 목적을 생각해봅시다😄
https://docs.oracle.com/javase/8/docs/api/java/util/Objects.html#isNull-java.lang.Object-
There was a problem hiding this comment.
Objects.nonNull적용된 곳 전부 바꿨습니다~!
yeon-06
left a comment
yeon-06
left a comment
There was a problem hiding this comment.
안녕하세요 기론!
피드백 반영 확인했고 2단계 미션 고생 많으셨습니다😄
다음 단계에서 뵐게요~!!
2 participants
안녕하세요 연로그~ 우선 jpa를 적용하지 않고 최대한 도메인 안쪽으로 코드를 넣어보려고 했습니다.
조금씩 리펙토링 수정해보겠습니다.
잘부탁드립니다!