Refactor/#803 행사 및 함께해요 관련 api 명세 수정 #808

amaran-th · 2023-10-30T12:29:05Z

#️⃣ 연관된 이슈

close : #803

📝 작업 내용

행사 및 함께해요 조회 API 명세 수정 및 자잘한 변경사항 반영

API 명세 순서 수정
Event Service 리팩토링

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

예상 소요 시간 : 10/30
실제 소요 시간 : 10/30

#803

- EventService를 Command와 Query로 분리 - AllImagesOfContent 일급컬렉션 구현 #803

- MemberReferenceResponse dto를 포함하도록 구현 #803

hong-sile

일단 간단하게 한 번 훑어봤어요 RC 날릴겡

hong-sile · 2023-10-31T04:41:05Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

 import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;


이 클래스 이름을 EvnetResponse로 바꾸는 건 어때요? Detail보단, 그게 나을 것 같아서요

반영했습니당

hong-sile · 2023-10-31T04:49:17Z

backend/emm-sale/src/main/java/com/emmsale/scrap/application/ScrapQueryService.java

-    for (Image image : images) {
-      imageUrlPerEventId.put(image.getContentId(), image.getName());
+
+    Map<Long, AllImagesOfContent> imageUrlsPerEventId = new HashMap<>();


stream으로 풀어볼수도 있지 않을까요?

전에 어디선가 코드리뷰 받으면서 알게된 건데 stream.forEach()로 원소를 순회하게 되면, 도중에 예외가 발생했을 때 순회가 중단되지 않고 순회를 마친 후 예외가 던져진다고 하더라구요.
그래서 저는 반복문 내에 레파지토리를 조회하는 로직이 포함된 경우 for문으로 풀어서 작성하고 있습니다.

그 stream.forEach()로 원소순회 시 예외 발생하면 순회를 마친 후 예외가 던져진다.
는 아마 내가 알려줬던 것 같은데, 기억하고 있네

나는 반복문 내에 Repository에서 조회 로직이 필요하도록 짰다면 다른 방식을 한번 고민해볼것 같아.
지금 그러면 쿼리가 event 수만큼 나가고 있는 거잖아.
내 생각엔 findAllByEventIds(eventIds)를 하고, 그 다음에 grouping을 할 수도 있을 것 같은데, 어떻게 생각해?
그러면 쿼리가 훨씬 줄 것 같아서

아아 그런 방법이 있었네
바로 반영했어
하는김에 중복 코드들을 ImageQueryService를 만들어서 옮겼습니당

hong-sile · 2023-10-31T04:49:31Z

backend/emm-sale/src/main/java/com/emmsale/scrap/application/ScrapQueryService.java

-    for (Image image : images) {
-      imageUrlPerEventId.put(image.getContentId(), image.getName());
+
+    Map<Long, AllImagesOfContent> imageUrlsPerEventId = new HashMap<>();


final 붙여주세용

반영했습니다!

hong-sile · 2023-10-31T04:54:43Z

backend/emm-sale/src/main/java/com/emmsale/event/application/EventQueryService.java

-    final List<String> informationImageUrls = extractInformationImages(imageUrls);
+        .collect(toList()));
+    final String thumbnailImageUrl = images.extractThumbnailImage();
+    final List<String> informationImageUrls = images.extractInformationImages();
    return EventDetailResponse.from(event, thumbnailImageUrl, informationImageUrls);


아예 파라미터로 Images를 넘겨도 좋을 것 같네요

좋은 아이디어 감사합니다~~ 반영했습니다!

hong-sile · 2023-10-31T04:55:38Z

backend/emm-sale/src/main/java/com/emmsale/image/domain/AllImagesOfContent.java

+    return images.stream()
+        .filter(Image::isThumbnail)
+        .findFirst()
+        .orElseThrow()


orElseThrow에서 Exception을 지정하고 테스트를 짜보면 좋을 것 같아

헐 커스텀 예외 만들려고 생각했었는데 잊어버렸나보다...
알려줘서 고마워 바로 반영할게

hong-sile · 2023-10-31T04:56:49Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

+      final Map<Long, AllImagesOfContent> imagesPerEventId) {
+    return events.stream()
+        .map(event -> {
+          final AllImagesOfContent allImageUrls = imagesPerEventId.get(event.getId());


이정도 사이즈의 메서드라면 별도로 분리하는건 어떨까?
그리고 Collect할 때 UnModifiableList로 해줄 수 있겠니

반영했습니당

hong-sile · 2023-10-31T04:57:53Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

+  ) {
+    return groupByEventStatus.values().stream()
+        .map(events -> makeEventResponsesByStatus(events, imagesPerEventId))
+        .reduce(new ArrayList<>(), (combinedEvents, eventsToAdd) -> {


이정도면 아마 reduce(new ArrayList<>(), List::addAll)로 할 수 있지 않나?

아 그거 처음에 그렇게 하려고 했는데 addAll의 반환 타입이 boolean이라 안되는 걸로 알고있어용

hong-sile · 2023-10-31T04:58:16Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

@@ -65,4 +70,28 @@ public static EventDetailResponse from(
        event.getEventMode().getValue()
    );
  }
+
+  public static List<EventDetailResponse> makeEventResponsesByStatus(final List<Event> events,


여기 변환로직이 조금 복잡한데 천천히 바꿔보자 솔직히 안 읽혀

오키오키

#803

amaran-th

@hong-sile
리뷰 반영했습니당~~

amaran-th · 2023-10-31T07:49:15Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

 import java.util.List;
+import java.util.Map;
+import java.util.stream.Collectors;


반영했습니당

amaran-th · 2023-10-31T07:54:28Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

+      final Map<Long, AllImagesOfContent> imagesPerEventId) {
+    return events.stream()
+        .map(event -> {
+          final AllImagesOfContent allImageUrls = imagesPerEventId.get(event.getId());


반영했습니당

amaran-th · 2023-10-31T07:57:55Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

+  ) {
+    return groupByEventStatus.values().stream()
+        .map(events -> makeEventResponsesByStatus(events, imagesPerEventId))
+        .reduce(new ArrayList<>(), (combinedEvents, eventsToAdd) -> {


아 그거 처음에 그렇게 하려고 했는데 addAll의 반환 타입이 boolean이라 안되는 걸로 알고있어용

amaran-th · 2023-10-31T07:59:08Z

backend/emm-sale/src/main/java/com/emmsale/image/domain/AllImagesOfContent.java

+    return images.stream()
+        .filter(Image::isThumbnail)
+        .findFirst()
+        .orElseThrow()


헐 커스텀 예외 만들려고 생각했었는데 잊어버렸나보다...
알려줘서 고마워 바로 반영할게

amaran-th · 2023-10-31T07:59:19Z

backend/emm-sale/src/main/java/com/emmsale/event/application/dto/EventDetailResponse.java

@@ -65,4 +70,28 @@ public static EventDetailResponse from(
        event.getEventMode().getValue()
    );
  }
+
+  public static List<EventDetailResponse> makeEventResponsesByStatus(final List<Event> events,


오키오키

amaran-th · 2023-10-31T08:02:26Z

backend/emm-sale/src/main/java/com/emmsale/event/application/EventQueryService.java

-    final List<String> informationImageUrls = extractInformationImages(imageUrls);
+        .collect(toList()));
+    final String thumbnailImageUrl = images.extractThumbnailImage();
+    final List<String> informationImageUrls = images.extractInformationImages();
    return EventDetailResponse.from(event, thumbnailImageUrl, informationImageUrls);


좋은 아이디어 감사합니다~~ 반영했습니다!

amaran-th · 2023-10-31T08:28:08Z

backend/emm-sale/src/main/java/com/emmsale/scrap/application/ScrapQueryService.java

-    for (Image image : images) {
-      imageUrlPerEventId.put(image.getContentId(), image.getName());
+
+    Map<Long, AllImagesOfContent> imageUrlsPerEventId = new HashMap<>();


전에 어디선가 코드리뷰 받으면서 알게된 건데 stream.forEach()로 원소를 순회하게 되면, 도중에 예외가 발생했을 때 순회가 중단되지 않고 순회를 마친 후 예외가 던져진다고 하더라구요.
그래서 저는 반복문 내에 레파지토리를 조회하는 로직이 포함된 경우 for문으로 풀어서 작성하고 있습니다.

amaran-th · 2023-10-31T08:28:15Z

backend/emm-sale/src/main/java/com/emmsale/scrap/application/ScrapQueryService.java

-    for (Image image : images) {
-      imageUrlPerEventId.put(image.getContentId(), image.getName());
+
+    Map<Long, AllImagesOfContent> imageUrlsPerEventId = new HashMap<>();


반영했습니다!

amaran-th

@hong-sile

amaran-th

@hong-sile

- 단건 조회, 다건 조회 API에 적용 #803

hong-sile

일단 이번 목표는 명세를 수정하는 거니, 더 리팩터링 관련해선 리뷰를 안 남길게요. 수고했슴다

hong-sile · 2023-11-02T04:18:03Z

backend/emm-sale/src/main/java/com/emmsale/scrap/application/ScrapQueryService.java

-    for (Image image : images) {
-      imageUrlPerEventId.put(image.getContentId(), image.getName());
+
+    Map<Long, AllImagesOfContent> imageUrlsPerEventId = new HashMap<>();


그 stream.forEach()로 원소순회 시 예외 발생하면 순회를 마친 후 예외가 던져진다.
는 아마 내가 알려줬던 것 같은데, 기억하고 있네

나는 반복문 내에 Repository에서 조회 로직이 필요하도록 짰다면 다른 방식을 한번 고민해볼것 같아.
지금 그러면 쿼리가 event 수만큼 나가고 있는 거잖아.
내 생각엔 findAllByEventIds(eventIds)를 하고, 그 다음에 grouping을 할 수도 있을 것 같은데, 어떻게 생각해?
그러면 쿼리가 훨씬 줄 것 같아서

hong-sile · 2023-11-02T04:21:25Z

backend/emm-sale/src/main/java/com/emmsale/event/application/EventCommandService.java

+      return new ArrayList<>();
+    }
+
+    return tags.stream()


여기 작업하는 부분이 아니긴 하지만, 이런 것도 엄청 쿼리가 많이 나가는 부분이잖아 In절 쿼리로 한번에 조회하도록 할 수있을 것 같은데?

tagRepository.findByNames(tagNames)

반영했어~~!

hong-sile · 2023-11-02T04:23:19Z

backend/emm-sale/src/main/java/com/emmsale/event/application/EventCommandService.java

-    final String thumbnailImageUrl = savedImages.extractThumbnailImage();
-    final List<String> informationImageUrls = savedImages.extractInformationImages();
-    return EventResponse.from(event, thumbnailImageUrl, informationImageUrls);
+    return EventResponse.from(event, savedImages);


코드리뷰 해줬던 부분을 반영하고, 하나의 커밋으로 합쳐서 커밋했네.
앞으로는 쪼개서 커밋해보도록 하자

넵...!👀

- 스크랩 행사 목록 조회 시 행사마다 image 조회 쿼리가 나가는 대신 행사들의 이미지를 모두 불러온 후 그룹핑되도록 수정 - 코드 중복을 없애기 위한 ImageQueryService 추가 #803

#803

amaran-th added 4 commits October 30, 2023 19:28

refactor: Event 다건 조회 명세를 단건 조회 명세와 동일하게 수정

2bdede3

#803

refactor: API 문서의 API 순서 조정

cc120f6

#803

refactor: EventService 리팩토링

0fdaea5

- EventService를 Command와 Query로 분리 - AllImagesOfContent 일급컬렉션 구현 #803

refactor: 함께해요 요청 조회 API 리팩토링

c33ed38

- MemberReferenceResponse dto를 포함하도록 구현 #803

amaran-th added Backend 백엔드 관련 이슈 리팩터링 테스트 코드의 검증 값이 변환하지 않고 코드 변경 High Priority 리뷰 우선순위가 높은 PR labels Oct 30, 2023

amaran-th added this to the 레벨 5 milestone Oct 30, 2023

amaran-th self-assigned this Oct 30, 2023

hong-sile requested changes Oct 31, 2023

View reviewed changes

amaran-th added 2 commits October 31, 2023 16:48

refactor: EventDetailResponse를 EventResponse로 변경

64b4e62

#803

refactor: 코드리뷰 반영

80a3b75

#803

amaran-th commented Oct 31, 2023

View reviewed changes

amaran-th requested a review from hong-sile October 31, 2023 10:01

refactor: 함께해요 게시글 반환 형식 변경

e7caef1

- 단건 조회, 다건 조회 API에 적용 #803

hong-sile approved these changes Nov 2, 2023

View reviewed changes

amaran-th added 4 commits November 2, 2023 15:10

refactor: 행사 목록 조회 시 tag 조회로 인해 발생하는 N+1 해결

bdee3ce

#803

refactor: 누락된 final 추가

b9ca8c9

#803

refactor: QueryService의 @transactional에 readOnly 속성 추가

6602615

#803

amaran-th merged commit 2bf14c4 into backend-main Nov 2, 2023
1 check passed

Refactor/#803 행사 및 함께해요 관련 api 명세 수정 #808

Refactor/#803 행사 및 함께해요 관련 api 명세 수정 #808

Conversation

amaran-th commented Oct 30, 2023

#️⃣ 연관된 이슈

📝 작업 내용

예상 소요 시간 및 실제 소요 시간 (일 / 시간 / 분)

hong-sile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

amaran-th left a comment

Choose a reason for hiding this comment

hong-sile left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment