[레거시 코드 리팩터링 - 1단계] 허브(방대의) 미션 제출합니다. #465
Conversation
README.md
Outdated
| - 메뉴의 가격은 0원 이상이어야 한다. | ||
| - 존재하는 메뉴 그룹 번호를 입력받아야 한다. | ||
| - 메뉴 상품 목록은 비어있을 수 없다. |
There was a problem hiding this comment.
메뉴 가격이 0원이면서 상품 목록이 비어있는 경우는 없을까요?
물론 메뉴 가격이 0원인 경우는 없겠지만, 비즈니스 로직 상 제약이 없기 떄문에 충분히 가능한 시나리오라고 여겨져요!
허브는 어떻게 생각하시나요?
There was a problem hiding this comment.
오 그러네요 상품 목록이 비어있지만 메뉴 가격이 0원인 경우에는 등록할 수 있네요!
해당 부분도 충분히 가능한 시나리오라고 생각됩니다.
해당 부분을 제거하고, 각각의 메뉴 상품의 경우 상품이 존재해야 한다. 로 변경했습니다!
|
|
||
| ## 테이블 | ||
|
|
||
| <img width="1011" alt="image" src="https://github.com/greeng00se/greeng00se.github.io/assets/58586537/1c2a352e-bed7-4c0f-89ed-8d6d31487b9c"> |
| } | ||
|
|
||
| final OrderTable savedOrderTable = orderTableDao.findById(orderTableId) | ||
| .orElseThrow(IllegalArgumentException::new); | ||
| .orElseThrow(() -> new IllegalArgumentException("주문 테이블을 찾을 수 없습니다.")); |
There was a problem hiding this comment.
예외 상황에 대해서는 비즈니스적인 의미를 담을 수 있도록 모두 메세지를 추가해주셨네요 👍
아마 허브만의 판단 기준이 있었을테지만, 궁금해서 하나 여쭤봅니다!
해당 서비스 클래스를 레거시로 바라본다면 변경 포인트가 생겨도 괜찮을까요?
변경이 되어도 괜찮다면 이유는 무엇일까요? 🤔
There was a problem hiding this comment.
현재 레거시 코드에서 예외 메시지만 수정을 했는데요!
예외를 구분하기 어렵다면, 테스트 작성 및 리팩터링 단계에서 어떤 예외가 발생했는지 명확히 구분이 어려울 거라고 생각했습니다!
따라서 레거시 코드를 및 로직을 최대한 수정하지 않고, 리팩터링 전 예외 메시지만 추가하도록 수정했습니다.
변경 포인트가 기존의 비즈니스 로직이 변경되는 부분이 아니고, 단순히 예외 메시지를 추가하는 정도라면 생산성 측면에서 괜찮다고 생각하는데 어떻게 생각하시나요?
There was a problem hiding this comment.
저는 추후 실무에서 레거시 코드를 접했을 때, 불가피한 이유로 코드 수정이 불가능한 상황이 있을 수도 있겠다고 생각했습니다! 물론 현재는 프로덕션 코드의 수정이 가능하고 제약이 없는 상황인데요.
실무 상황에서는 정책적인 이슈 등으로 인해 쉽게 건들 수 없는 경우도 있을 것 같다고 생각했어요! 또한, 빌드 파일이 바뀌어서는 안되는 경우도 있지 않을까요? 다시 빌드하기 어려운 환경도 있을 것 같아요.
다만 이 부분은 제가 실무를 경험해본 것이 아니라 추측일 뿐입니다! 실제로는 그런 상황이 없을수도 있겠죠 😅
제 개인적인 견해이니 참고만 하시면 좋을 것 같습니다!
There was a problem hiding this comment.
오.. 코드를 아예 수정하지 않는다는 제약 조건을 설정하고 진행해봐도 재밌겠네요 👍
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) | ||
| @TestExecutionListeners( | ||
| value = DatabaseCleaner.class, | ||
| mergeMode = TestExecutionListeners.MergeMode.MERGE_WITH_DEFAULTS | ||
| ) | ||
| @SpringBootTest | ||
| public @interface ServiceTest { | ||
| } |
| public static Order 주문(Long orderTableId, OrderStatus orderStatus) { | ||
| Order order = new Order(); | ||
| order.setOrderTableId(orderTableId); | ||
| order.setOrderedTime(LocalDateTime.now()); | ||
| order.setOrderStatus(orderStatus.name()); | ||
| return order; | ||
| } |
There was a problem hiding this comment.
주문 항목 목록은 비어있을 수 없다. 라는 비즈니스 제약조건이 존재하는데, Fixture에서 해당 규칙을 위반하는 객체를 생성해도 괜찮을까요?
인자 타입에 따라 두 가지 메소드가 오버로딩되어 있는 것 같은데, 실수로 '주문 생성' 기능을 테스트하는데 위 메소드를 사용하는 경우에는 Fixture로 인해 실패할 것 같아요. (물론 사용하는 사람이 잘 사용하면 문제는 없겠지만요) 테스트의 성공 / 실패 여부가 Fixture까지 도달할 수 있을 것 같아요.
따라서 개인적으로 저는 1. 비즈니스적으로 무결한 Fixture를 만들어두거나, 2. 모든 필드 설정을 호출자가 해주는 Fixture 메소드를 만드는 편이 낫지 않을까 싶은데
허브는 어떻게 생각하시는지 궁금하네요 ㅎㅎ 정답은 없으니 편하게 답변해주시기 바랍니다! 저는 허브의 생각이 궁금할 뿐 😌
There was a problem hiding this comment.
오오 꼼꼼하게 봐주셨네요. 저도 이 부분 테스트 작성을 하면서 주문 항목의 경우 주문의 Id를 가지고 있기 때문에 애매했었던 것 같습니다.
비즈니스적으로 무결한 Fixture는 리팩터링을 진행하면서 도메인에 검증 로직을 추가하여 사용할 수 있을 것 같아요.
말씀해주신 2번의 경우에도 현재 상황에서는 빈 주문 항목 목록을 넣어줄 것 같아요.
이 부분은 2단계 진행하면서 고민을 조금 더 해봐도 괜찮을까용?
There was a problem hiding this comment.
말씀해주신대로 비즈니스 제약조건들을 서비스에서 도메인 객체로 옮기면 자연스레 해결될 일이네요!
저도 고민되는 부분이라 2단계 진행하면서 같이 고민해보시죠 💯
| class MenuGroupServiceTest { | ||
|
|
||
| @Autowired | ||
| private MenuGroupService sut; |
There was a problem hiding this comment.
하나의 테스트에서 테스트하고자 하는 주요 대상이 되는 Unit을 SUT(System Under Test)라고 부른다.
👍👍
| Order savedOrder = sut.create(order); | ||
|
|
||
| // then | ||
| Order findOrder = orderDao.findById(savedOrder.getId()).get(); |
There was a problem hiding this comment.
sut.create(order)로 반환된 savedOrder 변수를 사용하지 않고 DAO를 통해 한번 더 조회를 하는 이유가 있을까요?
저는 create 메소드에 대해 테스트하는 것이라면 반환값에 대한 검증을 수행해야 한다고 생각합니다! 정말 이상한 일이긴 하지만 1. 데이터는 제대로 DB에 기록되었지만 2. 반환값은 이상한 값이 나가는 경우는 없을까요?
DAO에서 정상적인 값을 찾았다고 해서 서비스 메소드가 정상적인 값을 반환한다는 보장이 없지 않을까..? 이런 생각이 들었습니다!
There was a problem hiding this comment.
현재 테스트 대역을 사용하지 하지 않고 SpringBootTest를 사용하고 있어 저장의 경우 데이터베이스에 정상적으로 저장되는 부분이 중요하다고 생각해서 db에 값을 조회하여 정상적으로 저장이 되었는지 확인을 했습니다!
테오가 말씀해주신 것 처럼 해당 메서드의 경우 반환값도 검증이 필요하다고 생각해요! 둘 다 검증하도록 수정했습니다! 👍
| .extracting(OrderTable::isEmpty) | ||
| .containsExactly(false, false); |
안녕하세요 테오 😄
1단계에서는 읽기 쉬운 요구사항 그리고 BO에 대한 테스트를 꼼꼼히 작성하는 것에 중점을 두고 미션을 진행했습니다.
처음부터 요구사항을 잘 정리하고 시작해보려 했지만 쉽지 않았고, 테스트를 진행하면서 문서도 점진적으로 수정해보았습니다!
레거시 코드 리팩터링 미션동안 잘 부탁드립니다 🙇