-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DayLog의 Item들 순서 변경 기능 구현 #129
Conversation
private void changeOrdinalOfItemsByOrderedItemIds(final List<Long> orderedItemIds) { | ||
int ordinal = 1; | ||
|
||
for (final Long itemId : orderedItemIds) { | ||
final Item item = itemRepository.findById(itemId) | ||
.orElseThrow(() -> new BadRequestException(NOT_FOUND_TRIP_ITEM_ID)); | ||
item.changeOrdinal(ordinal++); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분 때문에 DayLogService
가 ItemRepository
를 주입받고 있습니다.
ItemRepository
에 의존하는 게 싫다면 DayLog.getItem()
을 사용하는 방법이 있긴 한데,
Stream으로 해당 id를 가지는 객체를 찾거나(O(n)) 몇 줄의 코드를 더 추가해서 탐색 시간을 줄여야 할 것 같은데... 어떻게 생각하시나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
홍고 멋져요 🍊
=== DayLog 아이템들 순서 수정 (PATCH /trips/:tripId/daylogs/:dayLogId/order) | ||
|
||
==== 요청 | ||
include::{snippets}/day-log-controller-test/update-ordinal-of-items/http-request.adoc[] | ||
include::{snippets}/day-log-controller-test/update-ordinal-of-items/path-parameters.adoc[] | ||
include::{snippets}/day-log-controller-test/update-ordinal-of-items/request-fields.adoc[] | ||
|
||
==== 응답 | ||
include::{snippets}/day-log-controller-test/update-day-log-title/http-response.adoc[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestDocs 💯
requestFields( | ||
fieldWithPath("itemIds") | ||
.type(JsonFieldType.ARRAY) | ||
.description("정렬된 아이템의 IDs") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
새로운 순서의 아이템 IDs 어떤가요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
옼케이
//given | ||
final ItemsOrdinalUpdateRequest request = new ItemsOrdinalUpdateRequest(List.of(3L,2L,1L)); | ||
|
||
doNothing().when(dayLogService).updateOrdinalOfItems( any(), any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
알수없는 공백
updateOrdinalOfItems( any(), any());
dayLogService.updateOrdinalOfItems(1L, request); | ||
|
||
// then | ||
List<Item> items = DAYLOG_FOR_ITEM_FIXTURE.getItems(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final 가능?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
항상 꾸준한 홍고생 응원합니다.
간단한 리뷰 남겼어요. 한 번씩 봐주세용
public void updateOrdinalOfItems(final Long dayLogId, | ||
final ItemsOrdinalUpdateRequest itemsOrdinalUpdateRequest) { | ||
final DayLog dayLog = dayLogRepository.findById(dayLogId) | ||
.orElseThrow(() -> new BadRequestException(NOT_FOUND_DAY_LOG_ID)); | ||
final List<Item> items = dayLog.getItems(); | ||
|
||
final List<Long> orderedItemIds = itemsOrdinalUpdateRequest.getItemIds(); | ||
validateOrderedItemIds(items, orderedItemIds); | ||
changeOrdinalOfItemsByOrderedItemIds(orderedItemIds); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void validateOrderedItemIds(final List<Item> items, final List<Long> orderedItemIds) { | ||
for (final Item item : items) { | ||
if (!orderedItemIds.contains(item.getId())) { | ||
throw new BadRequestException(INVALID_ORDERED_ITEM_IDS); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private void validateOrderedItemIds(final List<Item> items, final List<Long> orderedItemIds) { | |
for (final Item item : items) { | |
if (!orderedItemIds.contains(item.getId())) { | |
throw new BadRequestException(INVALID_ORDERED_ITEM_IDS); | |
} | |
} | |
} | |
private void validateOrderedItemIds(final List<Item> items, final List<Long> orderedItemIds) { | |
items.stream().filter(item -> !orderedItemIds.contains(item.getId())).forEach(item -> { | |
throw new BadRequestException(INVALID_ORDERED_ITEM_IDS); | |
}); | |
} |
어떨까요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어 이미 바꼈네
final ItemsOrdinalUpdateRequest request = new ItemsOrdinalUpdateRequest(List.of(3L,2L,1L)); | ||
|
||
doNothing().when(dayLogService).updateOrdinalOfItems( any(), any()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final ItemsOrdinalUpdateRequest request = new ItemsOrdinalUpdateRequest(List.of(3L,2L,1L)); | |
doNothing().when(dayLogService).updateOrdinalOfItems( any(), any()); | |
//given | |
final ItemsOrdinalUpdateRequest request = new ItemsOrdinalUpdateRequest(List.of(3L, 2L, 1L)); | |
doNothing().when(dayLogService).updateOrdinalOfItems(any(), any()); |
코드 정렬을 생활화 하자
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다 홍고
|
||
@PatchMapping("/order") | ||
public ResponseEntity<Void> updateOrdinalOfItems( | ||
@PathVariable final Long tripId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 안쓰이는거 같아요!
final Set<Long> itemIds = items.stream() | ||
.map(Item::getId) | ||
.collect(Collectors.toSet()); | ||
final Set<Long> orderedItemIdsSet = new HashSet<>(orderedItemIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분이 검증을 하는 것이 orderedItemIds
가 가진 요소들이 정확히 itemIds가 가지고 있는지 판별하는 것이라고 생각하는데 set으로 받아주는 것 보다 sort해서 정확히 같은지 비교하는 방법은 어떤가요?
final Set<Long> itemIds = items.stream() | |
.map(Item::getId) | |
.collect(Collectors.toSet()); | |
final Set<Long> orderedItemIdsSet = new HashSet<>(orderedItemIds); | |
final List<Long> itemIds = items.stream() | |
.map(item->item.getId()) | |
.sorted() | |
.collect(Collectors.toList()); | |
final List<Long> sortedOrderItemIds = orderedItemIds.stream().sorted().collect(Collectors.toList()); | |
if(!itemIds.equals(sortedOrderItemIds)){ | |
throw new BadRequestException(INVALID_ORDERED_ITEM_IDS); | |
} |
📄 Summary
DayLog의 Item들 순서를 변경하는 기능 구현했습니다.
🙋🏻 More
DayLogService
의updateItemOrdinals
메소드에서ItemFixture
에 위치한DayLog
객체를 사용하고 있습니다. 새로운DayLog
와Item
들을 생성하면 코드가 너무 길어져서ItemFixture
에 있는DayLog
객체를 사용했는데 이 부분 괜찮나요? 새로운 객체들을 만들어서 테스트해보길 원하신다면 코멘트 달아주세요.의견있으시면 말해주세요!
batch update를 사용해 성능을 높여보려고 했는데, batch update를 쓰려면 Item객체의 ID 생성 전략을 spring 코드에서 직접 할당해줘야하는 전략으로 바꾸거나, JDBC를 사용해야 한다고 하더군요...
그래서 일단은 업데이트해야하는 Item의 개수만큼 쿼리를 날리는 방식으로 구현했습니다... 나중에 시간이 된다면 성능 향상을 도전해보겠습니다. (하지만 DayLog 하나당 Item 최대 20개? 나쁘지 않을지도?)