[레거시 코드 리팩터링 - 2단계] 허브(방대의) 미션 제출합니다. #547
Conversation
| import javax.persistence.Converter; | ||
| import kitchenpos.vo.Money; | ||
|
|
||
| @Converter(autoApply = true) |
There was a problem hiding this comment.
전역 설정으로 컨버터를 적용해서 VO를 구현할 수 있군요.. 역시 👍
There was a problem hiding this comment.
조영호님의 객체지향 강의에서 쏙 가져왔습니다 ㅋ_ㅋ
| } | ||
|
|
||
| Money object = (Money) o; | ||
| return Objects.equals(amount.doubleValue(), object.amount.doubleValue()); |
There was a problem hiding this comment.
BigDecimal을 doubleValue로 바꿔서 비교를 해도 괜찮을까요?!
BigDecimal의 equals를 사용하지 않은 이유가 있으신가요?
There was a problem hiding this comment.
Money 코드는 거의 복사 붙여넣기긴 한데 ㅠ_ㅠ.. BigDecimal의 equals를 보면 다음과 같이 설명이 되어있습니다.
Compares this BigDecimal with the specified Object for equality. Unlike compareTo, this method considers two BigDecimal objects equal only if they are equal in value and scale (thus 2.0 is not equal to 2.00 when compared by this method).
비교시 BigDecinal의 equals를 사용하면 2.0과 2.00이 다르다고 표현하기 때문에 doubleValue로 변경하여 비교하는게 좋을 것 같아요.
| public class OrderTableRequest { | ||
|
|
||
| private Long id; | ||
| private Long tableGroupId; | ||
| private int numberOfGuests; | ||
| private boolean empty; | ||
|
|
There was a problem hiding this comment.
PR 본문에서 말씀주신게 요거군요!
저도 DTO를 어떤 형식으로 정의해야 될지 고민을 많이 했었는데 (API 응답 명세는 심지어 존재하지도 않아서..)
레거시와 호환성을 유지하기 위해서는 요렇게 하는게 제일 적합한 방법인 것 같네요 💯
There was a problem hiding this comment.
말해주신대로 호환성을 유지하기 위해 애매한 부분은 다음과 같이 유지하는게 좋을 것 같아요.
예를들면 service 로직에 null로 세팅하는 부분이 있는 코드들은 말이죵
| private String name; | ||
|
|
||
| public MenuGroupRequest(String name) { | ||
| this.name = name; | ||
| } | ||
|
|
There was a problem hiding this comment.
어플리케이션을 작동시키고 직접 요청을 보내보면 해당 Request가 HttpMessageConverter에 의해 제대로 파싱되지 않는 것 같아요! 요청을 보냈을 때 400이 뜨네요!
Resolved [org.springframework.http.converter.HttpMessageNotReadableException: JSON parse error: Cannot construct instance of
kitchenpos.dto.MenuGroupRequest
There was a problem hiding this comment.
더 꼼꼼했어야하는데.. 필드 하나인 경우 기본 생성자를 다 추가했습니다. controller 테스트의 중요성을 느끼고 갑니다 ㅠㅠ
|
|
||
| return savedMenu; | ||
| Map<Long, Product> products = productRepository.findAllByIdIn(request.getMenuProductIds()).stream() | ||
| .collect(toMap(Product::getId, identity())); |
There was a problem hiding this comment.
개인적으로 Map<Long, Product> 타입의 products를 DTO에 넘겨서 처리한다는게 조금은 어색하게 느껴지는 것 같아요.
DTO 때문에 표현 계층과 서비스 계층의 결합도가 높아지는 건 어쩔 수 없는 것 같지만, 엔티티를 다시 DTO로 넘겨주어 매핑을 처리하는게 어색하다고 느껴지는데 허브는 어떻게 생각하시나요?
DTO는 값만 전달하고, 서비스에서 매핑을 수행하는 것과 대비해 어떤 이점이 있나요?
There was a problem hiding this comment.
일단 매핑하는 부분을 다른 클래스에 둔다면 서비스 로직의 가독성이 증가하는 부분이 가장 큰 장점일 것 같아요.
Mapper 클래스 사용, 표현 계층과 서비스 계층 서로 다른 DTO 사용, 서비스에서 매핑하는 것도 고려를 했지만 현재 크기에서는 DTO에서 변환해도 충분하다고 생각했습니다.
다른 엔티티도 넘겨서 처리해서 제가 코드를 작성할 때는 크게 어색하지 않다고 생각했었는데, 테오 말 듣고 보니 다른 dto 변환 형식과 조금 달라서 어색함을 느낄 수도 있다고 생각되네요..
There was a problem hiding this comment.
현재 DTO가 컨버터의 역할까지 수행하고 있어서 조금 어색했던 것 같아요! 그리고 표현 계층과 응용 계층의 양방향 의존이 조금 더 강해지는 느낌이었어서.. 이 부분은 step3 진행하시면서 자연스레 고민하시게 될 것 같네요 ㅎㅎ
There was a problem hiding this comment.
레벨 3 진행하면서 추가적으로 고민해보겠습니다 👍
| public static List<OrderStatus> notCompletion() { | ||
| return List.of(COOKING, MEAL); | ||
| } | ||
| } |
There was a problem hiding this comment.
이 부분 아래 리뷰보고 다시 생각해봤는데 Order로 넣으면 다른 로직과 일관성을 위해 orderStatus != COMPLETION으로 하는게 가장 깔끔하겠네요 👍
| @Query("select o from OrderTable o where o.tableGroup.id = :tableGroupId") | ||
| List<OrderTable> findAllByTableGroupId(Long tableGroupId); |
There was a problem hiding this comment.
요것만 직접 @Query를 지정해주신 이유가 궁금해요! 😌
There was a problem hiding this comment.
OrderTable이 TableGroup을 객체 참조하고 있는데 id로 조회해서 Query로 설정해두었어요!
| if (orderRepository.existsByOrderTableIdInAndOrderStatusIn(orderTableIds, OrderStatus.notCompletion())) { | ||
| throw new IllegalArgumentException("테이블의 주문 상태가 조리중이거나 식사중인 경우 단체 지정 해제를 할 수 없습니다."); | ||
| } |
There was a problem hiding this comment.
저는 해당 기능이 핵심 비즈니스 로직이라고 판단해서 도메인 객체 내부로 캡슐화했거든요..!!
저는 개인적으로 비즈니스 로직이 쿼리로 새어나가는 걸 좋아하지 않아서 Repository에서는 기본적인 쿼리만 나가는 걸 선호하는 것 같아요(성능 상 병목이 생기지 않는 경우에 한해서)
허브는 이에 대해 어떻게 생각하시옵는지 궁금하네요 ㅎㅎ (허브신 의견 기대중)
There was a problem hiding this comment.
말씀해주신대로 비즈니스 로직이 도메인 내부로 가는게 비즈니스 로직을 관리하기 편하다고 생각합니다. 👍👍👍
TableGroupService나 TableService같은 경우 의존을 끊어줘야할 것 같은 부분이 눈에 보이네용
코멘트 남겨주신 로직이 그 본문에 남겨둔 이 부분인데용(3단계에서 분리하려고 크게 신경쓰지 못했는데 테오 매의 눈 ㄷㄷ)
해당 부분을 Order로 위치 시키면 메서드명이 많이 중요할 것 같다고 생각했어요.
서비스 로직에서 OrderStatus이 Completion이 아닌지 확인하는 메서드를 호출하고 예외를 던지는 것 보다 도메인 로직 안에 확인하는 로직이 있으면 좋다고 생각되는데요.
이 때 단체 지정 해제와 테이블의 상태를 변경할 때 각각 검증이 되어야하는데 로직이 동일하더라도, 비즈니스 요구사항이 다른 상황이니 검증하는 메서드를 각각 두는게 좋다고 생각됩니다.
테오는 어떻게 생각하시나요?
There was a problem hiding this comment.
해당 부분 Order 내부로 이동시켜보았습니다 👍 한결 깔끔해졌네용!!
감사합니다 테5️⃣
There was a problem hiding this comment.
public void validateUngroupTableAllowed() {
if (orderStatus != COMPLETION) {
throw new IllegalArgumentException("테이블의 주문 상태가 조리중이거나 식사중인 경우 단체 지정 해제를 할 수 없습니다.");
}
}
public void validateChangeTableStatusAllowed() {
if (orderStatus != COMPLETION) {
throw new IllegalArgumentException("테이블의 주문 상태가 조리중이거나 식사중인 경우 테이블의 상태를 변경할 수 없습니다.");
}
}요 부분 말씀해주신 것이군요 ㅎㅎ저도 개인적으로 표면적으로 드러나는 코드는 중복이지만 이게 의미적으로 100% 중복이라고는 생각하지 않습니다! 언제든지 단체 지정 해제와 테이블 상태 변경 조건이 변경될 수 있기 때문에 분리해서 관리하는게 좋을 것 같네요 💯
| @@ -4,11 +4,7 @@ | |||
|
|
|||
| public class OrderLineItemFixture { | |||
There was a problem hiding this comment.
앞서 Fixture에 관해 논의했던 문제가 깔쌈하게 해결되었네요 ㅎㅎ
| .map(OrderTable::getId) | ||
| .map(OrderTableIdRequest::new) | ||
| .collect(collectingAndThen(toList(), TableGroupRequest::new)); | ||
| } |
There was a problem hiding this comment.
이렇게 Request까지 해당하는 도메인 Fixture에 지정하는거 정말 좋은 것 같습니다!
안녕하세요 테오!
저번에 말씀해주신
주문 항목 목록은 비어있을 수 없다. 라는 비즈니스 제약조건이 존재하는데, Fixture에서 해당 규칙을 위반하는 객체를 생성해도 괜찮을까요?해당 부분은 도메인으로 옮기니 깔끔하게 해결되더라고요!최대한 일급 컬렉션을 사용하고, 서비스의 로직을 도메인으로 옮기려고 노력했습니다.
프로그래밍 요구 사항을 잘 지키려고 노력해봤는데, 사실 필드 갯수와 같은 부분은 지키기 어렵더라고요 😢
인수테스트를 작성하려고 했는데, 시간이 너무 많이 걸릴 것 같아 일단 작성하지 않았습니다. 😢 (레벨 4 너무 할 일이 많아 이슈)
차선의 선택으로 DTO를 기존의 도메인 필드 or 요청 필드를 그대로 사용하도록 리팩터링을 먼저하고, JPA 적용 이후로는 절대 건들지 않는 방향으로 리팩터링을 진행했습니다.
확실히 통합 테스트를 작성하다보니 TableServiceTest와 같은 부분은 많은 의존성을 가지고 있는 것을 볼 수 있습니다. ㅠㅠ
따라서 TableGroupService나 TableService같은 경우 의존을 끊어줘야할 것 같은 부분이 눈에 보이네용
3단계에서 의존성 분리를 진행하니 해당 부분은 따로 리팩터링을 진행하거나 하지 않았습니다.
졸린 상태로 코딩해서 이상한 부분도 많을 것 같지만.. 일단 테오의 리뷰를 믿고 PR날립니다..
2단계도 잘 부탁드립니다~~