[레거시 코드 리팩터링 - 1단계] 더즈(이동규) 미션 제출합니다. #221
Merged
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
jojogreen91
reviewed
Oct 25, 2022
| entity.setId(resultSet.getLong(KEY_COLUMN_NAME)); | ||
| entity.setCreatedDate(resultSet.getObject("created_date", LocalDateTime.class)); | ||
| return entity; | ||
| return new TableGroup( |
There was a problem hiding this comment.
레거시 코드 도메인을 수정하면서 정말 많은 부분이 바뀌는게 느껴지네요 ㅋㅋㅋ
jojogreen91
reviewed
Oct 25, 2022
|
|
||
| import kitchenpos.domain.Menu; | ||
|
|
||
| public class MenuResponse { |
There was a problem hiding this comment.
이야...dto 가 엄청나가 많아졌네요. 일일이 확인하면서 만드는게 힘들었을텐데 수고하셨습니다.
jojogreen91
reviewed
Oct 25, 2022
|
|
||
| if (Objects.equals(OrderStatus.COMPLETION.name(), savedOrder.getOrderStatus())) { | ||
| throw new IllegalArgumentException(); | ||
| throw new IllegalArgumentException("완료 주문은 상태를 변경할 수 없습니다."); |
There was a problem hiding this comment.
개인적으로 레거시를 많이 고치는걸 고민을 해봐야한다는 입장인데 예외 메시지 추가는 꼭 필요해 보이네요
jojogreen91
reviewed
Oct 25, 2022
| ); | ||
| } | ||
|
|
||
| @DisplayName("메뉴 그룹 목록을 조회한다..") |
jojogreen91
reviewed
Oct 25, 2022
| private static final String MENU_NAME = "앙념 두마리 치킨"; | ||
|
|
||
| private final List<MenuProductRequest> menuProductRequests = List.of( | ||
| new MenuProductRequest(Product.후라이드, 2), |
There was a problem hiding this comment.
이거 픽스쳐 좋긴한데 스태틱 임포트 까지 하니까 진짜 도메인 클래스를 사용한건지 깜짝 놀라게 되네요.
There was a problem hiding this comment.
그럴 수 있겠네요! ProductId.후라이드처럼 ID 값이라는 걸 명시하는 게 좋겠네요
jojogreen91
reviewed
Oct 25, 2022
| .containsOnly(Product.후라이드, Product.양념치킨), | ||
| () -> assertThat(menuProducts) | ||
| .extracting("quantity") | ||
| .containsOnly(Product.후라이드, Product.양념치킨) |
jojogreen91
reviewed
Oct 25, 2022
| @Test | ||
| void create() throws Exception { | ||
| // given | ||
| MenuResponse menuResponse = new MenuResponse( |
There was a problem hiding this comment.
dto 생성에 대해서 좀 더 깔끔하게 할 방법이 없을까는 고민해볼만 하네요. 저도 지금은 딱히 더 좋은 방법이 생각나진 않는거 같아요.
jojogreen91
approved these changes
Oct 25, 2022
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.
안녕하세요 조조그린!! 리뷰어로 만나니 새롭네요.
스모디 코드만 리뷰 받다가 미션 코드 리뷰도 받고 조조그린의 코드 리뷰를 많이 받을 수 있어서 좋은 것 같습니다.
테스트에서 @nested 사용
같은 메서드의 여러 흐름을 테스트 한다는 것을 알아보기 쉽게 하기 위해 @nested를 적극 활용하였습니다. 이렇게 하니 공통적인 given 설정도 가능하고 가독성도 좋아진 것 같은데 조조그린은 어떻게 생각하시나요?
PlatformTransactionManager로 롤백 처리@nested의 부작용이 중첩 클래스 내부에는
@Transactional롤백이 먹히지 않는다는 것이었어요. 그래서 모든 Service 레이어 테스트 클래스들이 상속받는 부모 클래스인@ServiceTest를 만들어서 직접 롤백을 하도록 했어요.실제 비즈니스 코드에서는 격리를 위해 다른 방법을 쓰겠지만 현재는 트랜잭션 롤백으로 간단히 격리를 구현하고 싶어서 위처럼 구현하였습니다.
프로덕션 코드는?
프로덕션 코드는 DTO를 사용하도록 변경한 것과 예외 메시지를 구체화한 것밖에 건드린 게 없어요. 2단계부터 본격적인 프로덕션 리팩터링을 진행하려고 합니다.