[레거시 코드 리팩토링 미션 - 1단계] 우르(김현우) 미션 제출합니다. #493
Conversation
There was a problem hiding this comment.
안녕하세요 우르! 미션에서 뵙는 건 처음이네요~
전체적으로 코드가 완벽해서 오히려 제가 더 많이 배워가는 기분이네요ㅎㅎ;
코드를 보며 궁금했던 점과 제안드리고 싶은 점들을 코멘트로 남겨보았습니다.
‘~~~한 부분에 대해서도 테스트를 작성해달라’는 코멘트가 많은데, 혹시나 누락하신 테스트가 있을까봐 남긴 것이니 꼭 필요한 테스트가 아니라고 생각하신다면 스킵하셔도 괜찮습니다!
그리고 혹시 미션 리뷰와 관련해서 원하시는 점이 있으시면 언제든 편하게 말씀 부탁드려요~!
| - id, tableGroupId, numberOfGuests, empty(boolean) | ||
| 2. 모든 주문 테이블들을 조회할 수 있다. | ||
| 3. 특정 주문 테이블을 empty 상태로 변경할 수 있다. | ||
| 4. 특정 주문 테이블의 손님의 명 수를 변경할 수 있다. |
There was a problem hiding this comment.
요구사항에 예외 케이스에 대한 내용은 없네요...!
저의 경우 서비스의 요구사항엔 예측 가능한 예외 상황에 대한 제약 사항도 포함된다고 생각하는 편인데, 우르는 예외 케이스에 대한 요구사항을 작성하는 것에 대해 어떻게 생각하시나요??
| @@ -2,3 +2,4 @@ logging.level.org.hibernate.type.descriptor.sql.BasicBinder=TRACE | |||
| spring.h2.console.enabled=true | |||
| spring.jpa.properties.hibernate.format_sql=true | |||
| spring.jpa.show-sql=true | |||
| spring.flyway.enabled=false | |||
There was a problem hiding this comment.
gradle에 flyway 의존성이 추가되어 있었네요....!
미션 코드들을 굉장히 꼼꼼하게 확인하셨군요👍👍
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.boot.test.context.SpringBootTest; | ||
|
|
||
| class MenuGroupServiceTest extends ServiceIntegrationTest { |
| menu.setMenuGroupId(3L); | ||
| menu.setName("menu1MenuGroup3"); | ||
| menu.setPrice(BigDecimal.valueOf(100000000)); | ||
| menu.setMenuProducts(menuProductDao.findAll()); |
There was a problem hiding this comment.
정상 케이스의 Menu 객체를 필드 혹은 Fixture로 만들고
menu.setPrice()로 가격 값을 수정해주면 어떨까요?? 중복되는 부분을 줄일 수 있을 것 같습니다!
| delete from product; | ||
| delete from order_table; | ||
| delete from table_group; | ||
| delete from menu_group; |
There was a problem hiding this comment.
질문) truncate가 아닌 delete로 테이블을 삭제하신 이유는 data.sql에서 정의해주신 ALTER문의 변경사항을 유지하기 위해서인가요?
There was a problem hiding this comment.
h2 는 제약조건이 있으면 truncate가 안된다고 하더라구요 그래서 delete 사용했습니다.
| () -> assertEquals(0, savedProduct.getPrice().compareTo(product.getPrice())), | ||
| () -> assertEquals(savedProduct.getName(), product.getName()) | ||
| ); | ||
| } |
There was a problem hiding this comment.
상품 가격이 0원 미만인 경우에 대한 검증 테스트도 있으면 좋을 것 같아요!
| assertAll( | ||
| () -> assertNotNull(savedTableGroup.getId()), | ||
| () -> assertNotNull(savedTableGroup.getCreatedDate()) | ||
| ); |
There was a problem hiding this comment.
테이블 그룹이 등록된 이후 테이블 그룹에 속한 주문 테이블들은 모두 비어있지 않은 상태가 되는데, 이 부분에 대해서도 검증 로직이 추가되면 좋을 것 같아요!
| tableGroupService.ungroup(tableGroupId); | ||
|
|
||
| //then | ||
| assertThat(orderTableDao.findAllByTableGroupId(tableGroupId)).isEmpty(); |
There was a problem hiding this comment.
단체 테이블을 삭제할 때 단체 테이블에 속한 주문 테이블의 주문들은 모두 계산이 완료된 COMPLETION 상태여야 하는데(혹은 주문이 아예 없거나), 이 제약사항에 대해서도 검증할 수 있으면 좋을 것 같아요!
| //when & then | ||
| assertThatThrownBy(() -> tableService.changeEmpty(orderTableId, orderTable)) | ||
| .isInstanceOf(IllegalArgumentException.class); | ||
| } |
There was a problem hiding this comment.
만약 주문테이블이 특정 테이블 그룹에 속해있을 경우에도 empty 여부를 변경할 수 없는데, 이 제약 사항에 대해서도 테스트가 있으면 좋을 것 같습니다!
There was a problem hiding this comment.
if (Objects.nonNull(savedOrderTable.getTableGroupId())) {
throw new IllegalArgumentException();
}이 부분인가요??
특정 테이블 그룹에 속해 있지 않으면 안된다는거 아닌가여,,??
There was a problem hiding this comment.
orderTable의 tableGroupId가 null이어야 검증을 통과하니 테이블 그룹에 속해있지 않아야 합니다!
2단계에서 알맞게 구현해주셨네요~!
| } | ||
|
|
||
| @Test | ||
| @DisplayName("changeNumberOfGuests() : 주문 테이블의 인원 수를 수정할 수 있다.") |
There was a problem hiding this comment.
DisplayName이 수정되지 않은 것 같아요!
그리고 변경하려는 손님 수가 0명 이하인 경우에도 검증을 하면 좋을 것 같아요!
안녕하세요 아마란스!!
저는 XXXService에 대해서만 통합 테스트를 진행했습니다.
이번 리뷰 잘 부탁드립니다!!