Feat/search place by tag1 #16

SonMinGyu · 2023-08-16T07:14:32Z

작업 내용들 PR 드립니다.
이것저것 프론트에서 요청한 사항들 작업했습니다.

리뷰 조회에 멤버 프로필 사진 응답 추가
태그로 장소검색 기능 추가
리뷰 이미지 응답 시 이미지id가 아닌 이미지를 넘겨주도록 수정
placeDto에 태그 class 추가
이미지 없이 리뷰 등록 가능하게 수정
멤버의 리뷰 데이터에 장소 이름 추가
리뷰, 장소 생성 시 전달해준 이미지 id가 존재하는 이미지 인지 확인 로직 구현
확인하고 코드리뷰 부탁드립니다!

- 리뷰 작성 시 이미지 필수였던 조건을 필수 아닌 조건으로 수정 - 리뷰 작성 시 이미지가 존재하는지 확인하는 로직 추가

github-actions · 2023-08-16T07:15:52Z

Unit Test Results

152 tests 152 ✔️ 17s ⏱️
  46 suites     0 💤
  46 files     0 ❌

Results for commit fc7fa46.

♻️ This comment has been updated with latest results.

yxxnghwan

민규님 고생하셨습니다! 리뷰 확인 부탁드려요~

yxxnghwan · 2023-08-16T13:58:11Z

src/main/java/org/prography/kagongsillok/common/utils/CustomListUtils.java

@@ -35,6 +35,9 @@ public static <T> boolean isEmpty(final List<T> list) {
    }

    public static <T> String joiningToString(final List<T> list, final String delimiter) {
+        if (list.size() < 1) {


매직넘버 상수로 추출해서 의미를 표현해주는건 어떨까요?

yxxnghwan · 2023-08-16T13:59:00Z

src/main/java/org/prography/kagongsillok/common/utils/CustomListUtils.java

@@ -35,6 +35,9 @@ public static <T> boolean isEmpty(final List<T> list) {
    }

    public static <T> String joiningToString(final List<T> list, final String delimiter) {
+        if (list.size() < 1) {
+            return null;


null을 리턴하기보단, 빈 스트링을 주는게 메소드의 의도에 더 맞는 것 같습니다! 메소드명을 보면 반드시 string 타입이 반환될 것 같아서요!

yxxnghwan · 2023-08-16T14:01:51Z

src/main/java/org/prography/kagongsillok/place/application/PlaceService.java

+    private void checkExistImage(final List<Long> imageIds) {
+        if (imageRepository.isExistIdIn(imageIds)) {
+            throw new NotFoundImageException(imageIds);
+        }
+    }


imageIds로 조회해서 존재할 경우 예외가 터지는 걸까요? 그렇게 읽히네요.

yxxnghwan · 2023-08-16T14:02:42Z

src/main/java/org/prography/kagongsillok/place/application/dto/PlaceDto.java

+    private List<Image> images;
    private String phone;
    private List<LinkDto> links;
    private List<BusinessHourDto> businessHours;
+    private List<ReviewTag> reviewTags;


dto에서 다른 엔티티를 직접 들고 있네요. 같이 dto로 변환해서 드는게 좋지 않을까요?

yxxnghwan · 2023-08-16T14:04:31Z

src/main/java/org/prography/kagongsillok/record/application/StudyRecordService.java

+    private List<StudyRecordDto> getStudyRecordDtos(final List<StudyRecord> studyRecords) {
+        return studyRecords.stream()
+                .map(studyRecord -> StudyRecordDto.of(studyRecord, getImages(studyRecord)))
+                .collect(Collectors.toList());
+    }


List<StudyRedcord> -> List<StudyRecordDto>로 변환하는 코드네요.
StudyRecordDto의 정적 팩토리로 만드는건 어떨까요?

map 안에서 imageRepository를 통해 각 studyRecord의 이미지를 불러오는 로직이 있는데 어떤식으로 변경하는게 좋을까요?

미리 studyRecords를 가지고 imageRepository에 한번에 조회해서 조회된 결과를 가지고 mapping 함수 내부를 작성할 수 있을 것 같아요!

yxxnghwan · 2023-08-16T14:09:04Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

+        if (member.getIsDeleted()) {
+            throw new NotFoundReviewException(memberId);
+        }


멤버가 삭제된 경우 리뷰도 삭제되어야할까요?
회원이 리뷰를 작성한 이후에 회원이 탈퇴했더라도 리뷰는 남아있어야할 것 같아요!

yxxnghwan · 2023-08-16T14:10:26Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

+        List<ReviewDto> reviewDtos = new ArrayList<>();
+        for (Review review : reviews) {
+            final Member member = memberRepository.findById(review.getMemberId())
+                    .orElseThrow(() -> new NotFoundMemberException(review.getMemberId()));
+            final Place place = placeRepository.findById(review.getPlaceId())
+                    .orElseThrow(() -> new NotFoundPlaceException(review.getPlaceId()));
+            reviewDtos.add(ReviewDto.of(review, member, getImages(review), place));
+        }
+
+        return reviewDtos;


n + 1 이 발생하는 코드네요. member와 place의 id를 모아 in절로 조회하면 n + 1을 회피할 수 있어요. 아니면 아예 조인해서 조회하는 방법도 있습니다.

yxxnghwan · 2023-08-16T14:11:54Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

+                    Member member = members.get(review.getMemberId());
+                    if (member == null) {
+                        member = Member.builder()
+                                .nickname("알 수 없음")
+                                .email("[email protected]")
+                                .build();
+                    }


정적 팩토리 메소드와 Map.getOrElse 등을 활용해서 코드 개선할 수 있을 것 같아요

yxxnghwan · 2023-08-16T14:13:22Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

+    private List<Image> getImages(final Review review) {
+        return imageRepository.findByIdIn(review.getImageIds());
+    }


메소드로 감춘 이유가 있을까요? 오히려 더 가독성이 떨어지는 메소드 네이밍인 것같아요.
그냥 풀어쓰는게 더 파악이 쉬워보입니다.

yxxnghwan

민규님 추가 리뷰 남겼습니다 반영 부탁드려요!

yxxnghwan · 2023-08-19T14:59:07Z

src/main/java/org/prography/kagongsillok/common/utils/CustomListUtils.java

@@ -9,6 +9,8 @@
 @UtilityClass
 public class CustomListUtils {

+    private static final int minimumImageSize = 1;


상수의 네이밍은 보통 전체 대문자에 _를 사용해서 단어를 구분합니다!

yxxnghwan · 2023-08-19T15:01:08Z

src/main/java/org/prography/kagongsillok/place/application/PlaceService.java

+        if (imageRepository.isNotExistIdIn(imageIds)) {
            throw new NotFoundImageException(imageIds);
        }


없는 이미지 아이디가 하나라도 있을 경우 에러가 발생하는걸까요? 없을 경우 빈 이미지객체를 내려주는 방식도 가능할 것 같아서요.
예를 들면 100건의 이미지를 조회했을 경우 99건만 있고 단 한건 없을 경우 99건은 찾은 이미지를 내려주고 없는 한 건에 대해서만 빈 이미지를 내려주는게 사용성에서 더 낫지않나 생각됩니다. 지금 코드의 경우 단 한건이라도 없을 경우 에러를 응답받을 것 같아요

해당 함수는 존재하지 않은 imageId가 place에 mapping되는 것을 막기 위해 imageIds를 조회해보는 함수입니다.
그래서 일단 return 값이 boolean이고요 실제 imageId를 참고해서 image를 가져올 때 image가 존재하지 않으면 findByIdIn으로 인해 자동으로 빈 객체가 return 될 것 같습니다.
조회 시에 없으면 빈 객체가 나오고 있으니 생성 전에 image가 존재하는지 검증하는 로직이 필요 없을까요?

매핑시 검증을 위해 사용되는 로직이군요. 이건 트레이드 오프일 것 같은데, 실제 이미지가 존재하는지 안하는지를 검증 로직에 포함시키면 place 관련 테스트를 짜기 위한 협력 이미지가 늘어날 것 같아서 이 부분이 불편할 것 같고 대신 생성시 좀 더 데이터 정합성에 초점을 맞출 수 있을 것 같아요. 민규님이 판단하여 의견이랑 이유 말씀주시고 소신껏 가시죠!

데이터 정합성이 서비스 관점에서 생각했을 때 좀 더 중요할 것 같아서 이대로 가겠습니다!

yxxnghwan · 2023-08-19T15:04:17Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

    public List<ReviewDto> getAllReviewsByMemberId(final Long memberId) {
        final List<Review> reviews = reviewRepository.findAllByMemberId(memberId);

-        List<ReviewDto> reviewDtos = new ArrayList<>();
-        for (Review review : reviews) {
-            final Member member = memberRepository.findById(review.getMemberId())
-                    .orElseThrow(() -> new NotFoundMemberException(review.getMemberId()));
-            final Place place = placeRepository.findById(review.getPlaceId())
-                    .orElseThrow(() -> new NotFoundPlaceException(review.getPlaceId()));
-            reviewDtos.add(ReviewDto.of(review, member, getImages(review), place));
+        if (reviews.isEmpty()) {
+            return List.of();
        }

-        return reviewDtos;
+        final List<Long> memberIds = reviews.stream()
+                .map(Review::getMemberId)
+                .collect(Collectors.toList());
+
+        final List<Long> placeIds = reviews.stream()
+                .map(Review::getPlaceId)
+                .collect(Collectors.toList());
+
+        final List<Long> imageIds = reviews.stream()
+                .map(Review::getImageIds)
+                .flatMap(List::stream)
+                .collect(Collectors.toList());
+
+        final Map<Long, Member> memberMap = memberRepository.findByIdIn(memberIds);
+        final Map<Long, Place> placeMap = placeRepository.findByIdInToMap(placeIds);
+        final Map<Long, Image> imageMap = imageRepository.findByIdInToMap(imageIds);
+        final Map<Long, List<Long>> reviewIdImageIdsMap = reviews
+                .stream()
+                .collect(Collectors.toMap(Review::getId, Review::getImageIds));
+
+        List<ReviewDto> reviewDtos = new ArrayList<>();
+
+        return reviews.stream()
+                .map(review -> ReviewDto.of(
+                        review,
+                        memberMap.get(review.getMemberId()),
+                        getMappedImages(reviewIdImageIdsMap.get(review.getId()), imageMap),
+                        placeMap.get(review.getPlaceId())
+                ))
+                .collect(Collectors.toList());
    }


메소드 라인이 너무 긴 것 같은데 관심사와 목적에 따라 충분히 메소드 분리 가능해보입니다.
리팩토링 부탁드려요.

yxxnghwan

민규님 고생하셨습니다! 리뷰 확인 부탁드려요!

yxxnghwan · 2023-08-21T15:55:29Z

src/main/java/org/prography/kagongsillok/record/application/StudyRecordService.java

        return studyRecords.stream()
-                .map(studyRecord -> StudyRecordDto.of(studyRecord, getImages(studyRecord)))
+                .map(studyRecord -> studyRecord.getImageIds())


메소드 참조 사용하는건 어떨까요?

yxxnghwan · 2023-08-21T15:56:10Z

src/main/java/org/prography/kagongsillok/record/application/StudyRecordService.java

+    private Map<Long, Image> getImageMap(final List<Long> imageIds) {
+        return imageRepository.findByIdIn(imageIds)
+                .stream()
+                .collect(Collectors.toMap(Image::getId, image -> image));
+    }


해당 메소드는 ImageRepository 에 있으면 좋을 것 같아요!

yxxnghwan · 2023-08-21T15:57:15Z

src/main/java/org/prography/kagongsillok/record/application/StudyRecordService.java

+                .stream()
+                .collect(Collectors.toMap(Image::getId, image -> image));


Function.identity() 사용할 수 있을 것 같아요

yxxnghwan · 2023-08-21T15:59:35Z

src/main/java/org/prography/kagongsillok/review/application/ReviewService.java

@@ -4,6 +4,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.stream.Collectors;
+import kotlin.Function;


어떤 import일까요?

yxxnghwan

민규님 ddl-auto설정 다시 부탁드릴게요~!

yxxnghwan · 2023-08-22T17:17:15Z

src/main/resources/application-develop.yml

    hibernate:
-      ddl-auto: create
+      ddl-auto: update



운영환경에 올리면서 변경하게 된 걸까요?
update의 경우 잘 사용하지 않는 옵션입니다. 서비스 운영에서 문제가 되는 코드를 배포하는 경우는 허다한데, 그럴 경우 운영중인 원본 DB 테이블에 치명적인 영향을 끼칠 수 있어서 그래요.
운영이라면 좀 더 보수적으로 접근해서 none이나 validate 옵션을 주로 사용하고 있어요.

넵 확인해서 수정했습니다!

yxxnghwan

수고하셨습니다 민규님!

SonMinGyu added 10 commits August 16, 2023 15:18

refactor: 리뷰 조회 응답에 멤버 profile 추가

f0b51e7

feat: 태그로 장소 검색하는 기능 구현

bd92297

test: 태그들로 장소 조회 인수 테스트 구현

d5e61f2

refactor: 장소 응답에 태그 보내도록 리팩토링

abfb4f8

refactor: 장소 조회 응답에 관련된 태그 같이 보내도록 리팩토링

7f77798

refactor: place 관련 review 조회 시 reviewTag join 추가

921a1e7

refactor: review 조회 시 이미지 응답하도록 리팩토링

9a7bc2f

refactor: 리뷰 작성 시 이미지 필수 조건 수정

42fcea7

- 리뷰 작성 시 이미지 필수였던 조건을 필수 아닌 조건으로 수정 - 리뷰 작성 시 이미지가 존재하는지 확인하는 로직 추가

feat: 장소 등록 시 이미지 존재여부 판단 로직 추가

c3abf66

refactor: 리뷰 응답에 장소 이름 추가

8429465

SonMinGyu requested review from doorBW and yxxnghwan August 16, 2023 07:14

SonMinGyu added 7 commits August 16, 2023 17:11

test: ReviewAcceptanceTest 수정

b737711

test: test오류 수정

f761488

test: StudyRecordAcceptanceTest 수정

56b81b3

feat: StudyRecord 이미지 존재 여부 검증 로직 추가

81d0203

test: PlaceAcceptanceTest 수정

191bb97

test: StudyRecordServiceTest 수정

d1c00a2

refactor: studyrecord 응답 시 image 보내주도록 리팩토링

d0c9a62

yxxnghwan requested changes Aug 16, 2023

View reviewed changes

refactor: 코드 리뷰 사항 수정

c6db02d

yxxnghwan requested changes Aug 19, 2023

View reviewed changes

refactor: 코드 리뷰 내용 반영

e26c46e

yxxnghwan requested changes Aug 21, 2023

View reviewed changes

SonMinGyu added 2 commits August 22, 2023 21:45

refactor: 코드 리뷰 사항 반영

04dafa0

chore: jpa hibernate ddl-auto 속성 변경

5c53d26

yxxnghwan requested changes Aug 22, 2023

View reviewed changes

chore: ddl-auto 수정

fc7fa46

yxxnghwan approved these changes Aug 23, 2023

View reviewed changes

SonMinGyu merged commit 473697e into develop Aug 24, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/search place by tag1 #16

Feat/search place by tag1 #16

SonMinGyu commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 •

edited

Loading

yxxnghwan left a comment

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

SonMinGyu Aug 17, 2023

yxxnghwan Aug 19, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan Aug 16, 2023

yxxnghwan left a comment

yxxnghwan Aug 19, 2023

yxxnghwan Aug 19, 2023

SonMinGyu Aug 21, 2023

yxxnghwan Aug 21, 2023

SonMinGyu Aug 23, 2023

yxxnghwan Aug 19, 2023

yxxnghwan left a comment

yxxnghwan Aug 21, 2023

yxxnghwan Aug 21, 2023

yxxnghwan Aug 21, 2023

yxxnghwan Aug 21, 2023

yxxnghwan left a comment

yxxnghwan Aug 22, 2023

SonMinGyu Aug 23, 2023

yxxnghwan left a comment

		.stream()
		.collect(Collectors.toMap(Image::getId, image -> image));

Feat/search place by tag1 #16

Feat/search place by tag1 #16

Conversation

SonMinGyu commented Aug 16, 2023

github-actions bot commented Aug 16, 2023 • edited Loading

Unit Test Results

yxxnghwan 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

yxxnghwan 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

yxxnghwan 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

yxxnghwan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yxxnghwan left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 16, 2023 •

edited

Loading