Item 수정 요청시 isPlaceUpdated로 판단 로직 추가 #162
Conversation
📝 Jacoco Test Coverage
|
| private final Boolean isPlaceUpdated; | ||
|
|
||
| private final PlaceRequest place; | ||
|
|
||
| private final ExpenseRequest expense; |
There was a problem hiding this comment.
우리 어노테이션 없는 변수들 사이는 공백 없이 가기로 했던 것 같은데? 근데 어라 싶어서 찾아보니 다른 파일들도 다 이렇게 되어있고? 저는 이제 뭐가 뭔지 모르겠구요? 돌이키기엔 이미 늦었을지도?
There was a problem hiding this comment.
맞아요 늦었어요 .. 나중에 기능 구현 다 완료되면 전체적으로 컨벤션 리팩토링 해야될듯
| private void validateRatingFormat(final Double value) { | ||
| if (value % 0.5 != 0) { | ||
| throw new BadRequestException(INVALID_RATING); | ||
| } | ||
| } |
There was a problem hiding this comment.
0.5 매직넘버 상수화 해주세요 라고 하려 했는데 ItemRequest도 코드가 이거랑 똑같구요?
근데 또 도메인에 Item에서는 private static final double RATING_DECIMAL_UNIT = 0.5; 으로 상수 잘 적용되 있구요?
이것도 뭐가뭔지 모르겠네요?
통일..해주시면..감사하겠다는 저의 작은 바람..
There was a problem hiding this comment.
원래 있던 ItemRequest에 isPlaceUpdated만 추가한거라 그대로 복붙했는데
다시 보면서 생각이 든게 굳이 dto에서 검증로직을 돌아야 할까? 라는 생각이 들었어요.. 이건 얘기를 다시 해봐요..!
| public ResponseEntity<Void> updateItem( | ||
| @PathVariable final Long tripId, | ||
| @PathVariable final Long itemId, | ||
| @RequestBody final ItemRequest itemRequest) { | ||
| itemService.update(tripId, itemId, itemRequest); | ||
| @RequestBody final ItemUpdateRequest itemUpdateRequest | ||
| ) { | ||
| itemService.update(tripId, itemId, itemUpdateRequest); | ||
| return ResponseEntity.noContent().build(); | ||
| } |
There was a problem hiding this comment.
여기 코드에는 문제가 없지만 제가 사소하게 궁금한게 있습니다 디노..
ItemController는 수정되었는데 왜 ItemControllerTest는 수정된 커밋이 없을까요..?
ItemRequest가 ItemUpdateRequest로 수정되었는데...?
테스트 수정을 안했으면 RestDocs가 터져야 정상인데..?
안터졌으면 그냥 처음부터 이 API에 RestDocs를 적용을 안한건데 설마 그건 아니었겠죠..?
아니 근데 다시 생각해보니 RestDocs가 아니라 그냥 테스트가 터질텐데?
커밋이 누락된거겠죠..?
There was a problem hiding this comment.
오우 항상 pr 보내기 전에 테스트 쫙 돌려보고 하는데 다 통과해서 놓쳤나보네요.. 변경했습니다.
근데 보니까 테스트 안터지는게 맞아요.
문제 드립니다 왜 안터질까요~~?
There was a problem hiding this comment.
왜냐면!? !!!? NotNull이 아니기 때문에 ???????
There was a problem hiding this comment.
근데 이건 제가 isPlaceUpdated에 NotNull 조건 안붙여서 통과한 거예요.. 수정하겠습니다..
| dayLog, | ||
| getExpenseByItemRequest(itemRequest), | ||
| getExpenseByItemRequest(itemRequest.getExpense()), | ||
| getImagesByItemRequest(itemRequest) | ||
| ); | ||
| validateAlreadyDeleted(item); |
There was a problem hiding this comment.
이 로직들 이제 없어도 되지 않나요 ?!?!?
없어도 된다면 다른 메소드들에서도 삭제 부탁드리겠습니담
| getNewItemOrdinal(tripId), | ||
| itemRequest.getRating(), | ||
| itemRequest.getMemo(), | ||
| getPlaceByItemRequest(itemRequest), | ||
| getPlaceByItemRequest(itemRequest.getPlace()), | ||
| dayLog, | ||
| getExpenseByItemRequest(itemRequest), | ||
| getExpenseByItemRequest(itemRequest.getExpense()), | ||
| getImagesByItemRequest(itemRequest) |
There was a problem hiding this comment.
여기 네이밍들 뒤에 ByItemRequest가 꼭 붙어야하나 ?!??!?!
메소드 네임만 조금 더 길어진 느낌적인 느낌 ~ 의견입니다 ~
There was a problem hiding this comment.
네이밍 make~ 로 바꿨는데 괜찮을까요..?
| @@ -106,11 +113,11 @@ private Place createPlaceByPlaceRequest(final PlaceRequest placeRequest) { | |||
| ); | |||
There was a problem hiding this comment.
이거 메소드 하나로 가도 좋을거 같아요
get, create 둘다 비슷한 의미라 오히려 한뎁스만 더 들어가는 느낌!
나중에 로직 추가되고 분리해도 될듯 ~~ 의견입니다!
There was a problem hiding this comment.
음 제가 작성한 코드가 아니라 섣불리 고치기가 어렵네요 ..
로직 추가되면 그 때 다시 한 번 보겠습니다..!
| private Expense getExpenseByItemRequest(final ItemRequest itemRequest) { | ||
| if (itemRequest.getExpense() == null) { | ||
| private Expense getExpenseByItemRequest(final ExpenseRequest expenseRequest) { | ||
| if (expenseRequest == null) { | ||
| return null; | ||
| } | ||
| return createExpenseByExpenseRequest(itemRequest.getExpense()); | ||
| return createExpenseByExpenseRequest(expenseRequest); |
📄 Summary
🙋🏻 More