Skip to content
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

feat #10: 맛집 목록 조회 음식 종류 필터링, 페이징 구현과 MatzipRaw 실제 클래스로 전환 #39

Merged
merged 13 commits into from
Nov 9, 2023

Conversation

mojh7
Copy link
Contributor

@mojh7 mojh7 commented Nov 7, 2023

📃 설명

🔨 작업 내용

  1. TempMatzipRaw -> MatzipRaw 전환
    • 테스트용 더미 데이터 Matzip.sql에 MatzipRaw 더미 데이터 설정
    • 기존 테스트 통과되도록 수정
  2. 필터링 구현
    • 현재 데이터 수집 중인 음식점 종류(카페, 일식, 중식)에 대해 필터링 할 수 있게 category query parameter 추가
    • category 미 입력시 default 값으로 음식점 종류 상관없이 모든 음식점 조회(=따로 필터링 x)
  3. 페이징 구현
    • 과제 처음 설명상 거리 계산을 서버에서 하다보니 기존의 jpa 페이징 기능은 사용 못하고 일정 거리안에 포함됐는지 확인 후 수 작업으로 페이징 구현
    • TODO: 아직 제대로 동작하는지 검증이랑 테스트 작성 못했습니다. 이후에 하겠습니다
  4. 유효성 검증 관련 ErrorCode만 추가
    • TODO: 다음 PR에 request prameter에 대한 유효성 검사 추가하겠습니다

💬 기타 사항

  • subList gc 관련
  • 마지막 날은 문서화 작업을 주로 하는게 좋을 것 같아 추가로 필요한 작업은 나중에 해보겠습니다

mojh7 added 2 commits November 7, 2023 12:10
 - matzip.sql에 MatzipRaw 실제 필드에 맞춰서 더미 데이터 설정
 - MatzipRaw 필드가 많아서 matzip.sql 불러올 때 에러 해결겸 MatzipRaw unique key 검증 테스트 추가
 - matzip.sql 불러오는 테스트들 여러 개 같이 실행하니 에러 생겨서 @testinstance(TestInstance.Lifecycle.PER_CLASS) 설정
 - 위도, 경도 데이터 Location 클래스로 교체
@github-actions github-actions bot added the ☁️ feature 새로운 기능 추가 label Nov 7, 2023
Copy link

github-actions bot commented Nov 7, 2023

Auto add labels 📑

@mojh7 mojh7 self-assigned this Nov 7, 2023
mojh7 added 5 commits November 7, 2023 16:50
 - 지금은 따로 설정 하는 것은 없고 테스트 때 인식을 못해서 JPAQueryFactory bean 설정만
 - 서버에서 거리 계산과 상관없이 db에서 데이터 가져올 때 구분해도 되고 query dsl 써보고 싶어서 따로 MatzipRepositoryImpl 만들어서 필터링 구현
 - 생성자에 Builder 선언하는 방식으로 하니 저 방법 말고는 딱히 디폴트 값 설정 방법 못 찾음
 - 참고: db에는 중식 = '중국식' 카페는 '까페'로 되어있음
 - 거리순, 평점순 조회에서 카페, 일식, 중식 필터링 테스트
 - 정렬(type), 음식집 종류(category) 미 입력시 테스트
 - 다음 로직에서 페이징 구현할 때 파라미터가 3->4개가 되는데 람다는 (a, b) -> a + b 같은 적은 개수의 파라미터를 받고 간단한 로직을 짤 때 적합한 듯 하여 수정
 - 맛집 목록 조회 로직 담당하는 MatzipListRetrieveType 람다에서 추상 메서드로 변경
 - MatzipListRetrieveType가 사실 거리순, 평점순이라 다른 조회 api였으면 sort정도인데 너무 많은 역할을 하는 것 같아 추후 좋은 방법 있으면 적절히 분리 예정
 - db말고 서버app에서 거리 계산 하는 방식이라 수작업으로 페이징 구현
 - subList gc 이슈 있어서 새로 복사본 반환하는 방법으로 처리
@mojh7
Copy link
Contributor Author

mojh7 commented Nov 7, 2023

subList index 참고

@Test
void subListTest() {
    List<Integer> list = Arrays.asList(1, 2, 3, 4, 5);

    List<Integer> subList1 = list.subList(0, 4);
    List<Integer> subList2 = list.subList(0, 5);
    // List<Integer> subList3 = list.subList(0, 6); // IndexOutOfBoundsException

    List<Integer> subList4 = list.subList(5, 5);
    // List<Integer> subList5 = list.subList(5, 6); // IndexOutOfBoundsException
    // List<Integer> subList6 = list.subList(6, 7); // IndexOutOfBoundsException
    // List<Integer> subList7 = list.subList(10, 3); // IllegalArgumentException: fromIndex(10) > toIndex(3)

    System.out.println(subList1.size()); // 4
    System.out.println(subList2.size()); // 5
    // System.out.println(subList3.size());
    System.out.println(subList4.size()); // 0
    // System.out.println(subList5.size());
    // System.out.println(subList6.size());
    // System.out.println(subList7.size());
}

@mojh7 mojh7 marked this pull request as ready for review November 7, 2023 15:43
mojh7 added 2 commits November 8, 2023 09:26
 - member랑 중복되지만 따로 가져 가기로함
 - NumberFormatException도 custom 예외 메시지 나가도록 수정
Copy link
Contributor

@JoonheeJeong JoonheeJeong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

장현님 코드 잘봤습니다! 제가 몇가지 사소한 코멘트들만 남겨놨습니다!

Comment on lines 21 to 48
DISTANCE() {
@Override
public List<MatzipSummaryReponse> retrieve(List<Matzip> matzipList, Location requestLocation, double range,
int page, int pageSize) {
List<MatzipSummaryReponse> matzips
= matzipList.stream()
.map(m -> MatzipSummaryReponse.of(m, m.calcDistanceFrom(requestLocation)))
.filter(m -> m.getDistance() <= range)
.sorted(Comparator.comparingDouble(MatzipSummaryReponse::getDistance))
.collect(Collectors.toList());
return paging(matzips, page, pageSize);
}
},
// 평점 높은순
AVG_RATING() {
@Override
public List<MatzipSummaryReponse> retrieve(List<Matzip> matzipList, Location requestLocation, double range,
int page, int pageSize) {
List<MatzipSummaryReponse> matzips
= matzipList.stream()
.map(m -> MatzipSummaryReponse.of(m, m.calcDistanceFrom(requestLocation)))
.filter(m -> m.getDistance() <= range)
.sorted(Comparator.comparingDouble(MatzipSummaryReponse::getAvgRating)
.reversed())
.collect(Collectors.toList());
return paging(matzips, page, pageSize);
}
};
Copy link
Contributor

@JoonheeJeong JoonheeJeong Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 요렇게 빼낸 것이 가독성이 더 좋아보이네요. 👍🏼

  2. 작은 팁인데, JDK16부터는 .collect(Collectors.toList()) -> .toList()로 쓰면 더 짧게 쓸 수 있습니다~~ 혹시 아시는데 레거시 방식대로 쓰시는 거면 이유를 알 수 있을까요!?

  3. 요건 추가적인 제안인데, 27, 41번줄에 MatzipSummaryReponse DTO 생성할 때, 생성하는 정적 팩토리 메서드 of의 인자를 Matzip, double에서 Matzip, Location으로 바꿔 받고 내부에서 calcDistanceFrom을 호출하는 방향으로 리팩토링하면 코드 중복을 줄이고 응집도를 더 높일 수 있을 것 같다는 생각인데, 혹시 어떻게 생각하시는지요~? 장현님 생각이 궁금합니다!

  4. ㅋㅋ 아 그리고 쓰다보니까 발견한 건데, MatzipSummaryResponse가 맞는데, Re's'ponse에 s가 빠져있네요! 요거는 수정해주시면 좋을 것 같습니다.

Copy link
Contributor Author

@mojh7 mojh7 Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 👍

  2. 딱히 이유는 없었고 저 프로젝트 설정할 때 주로 자바 8 아님 11써서 바뀐 줄도 몰랐었습니다 ㅎㅎ toList로 바꾸겠습니다

  3. 평소에 dto에 비즈니스 로직이 담기는 것을 지양하고 기껏해야 타입 캐스팅 정도만 하거나 entity의 일부 필드만 response dto에 반영하는 작업 정도만 해왔습니다.
    하지만 말은 이렇게 하고 제가 작성한 것을 다시 보니 평점 = 총점 / 개수 작업도 제 평소 생각대로 할 거였으면 Service 레이어에서 평점 계산 다 하고 평점 자체를 넘겼어야 했네요. ㅎㅎ
    평점도 이미 dto에서 계산하고 있으니 거리 정보도 리뷰 내용대로 수정하겠습니다.

  4. 이건 발견 안 해주셨으면 영원히 못 찾았습니다...

Copy link
Contributor

@JoonheeJeong JoonheeJeong Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그러네요. 생각해보니 비즈니스 로직이 서비스 레이어에 모여있는 게 아름답긴 할텐데 ㅋㅋ

그래도 검색 로직을 각 검색타입 인스턴스별로 분할해서 종속시키려면 지금 구조도 괜찮은 것 같아요!

Comment on lines +65 to +68
// subList(0, 15) -> index 0부터 14까지
// subList는 기존 list의 view일 뿐이라 기존 list가 gc 대상이 되지않는 이슈가 있다고 함
// 그래서 새로운 복사본 생성해서 반환하도록 설정
return new ArrayList<>(matzips.subList(fromIndex, toIndex));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 되게 심화적인 부분도 고려하셨네요 .. 👍🏼

그런데 요게 이해가 안 되는데, 서브리스트를 쓰면 기존 리스트가 참조 대상이기 때문에 임시적으로 gc 대상에서 제외될 수 있다는 건 이해가 되는데, 결국은 요 메소드 호출하는 서비스 흐름이 끝나면 새롭게 생성하든 안 하든 서브리스트나 기존리스트나 gc가 제거하지 않을까요?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이건 저도 준희님과 같이 생각했습니다. 다른 API도 생각해보면 스레드에서 모든 로직이 끝나고 gc가 그 흔적들을 지울텐데 이곳도 마찬가지가 아닌가 생각이듭니다.

// min <= x && x <= max와 같음
return MIN_RANGE.compareTo(range) <= 0 && range.compareTo(MAX_RANGE) <= 0;
}
}
Copy link
Contributor

@JoonheeJeong JoonheeJeong Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요기 빈 줄 추가해주시면 좋을 것 같아요!
코드는 잘 봤습니다!

@@ -0,0 +1,3 @@
matzip:
page:
size: 20
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요기 설정파일도 빈 줄 추가 부탁드립니다 ㅎㅎ

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

와 더미데이터 만드시느라 진짜 고생많으셨습니다 ㅜㅜ

Copy link
Contributor Author

@mojh7 mojh7 Nov 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gpt로 어느 정도 모양새 다듬어서 넣어서 작업량을 줄이긴 했는데

만약 컬럼이 28개이면 어떤 필드는 값이 26개밖에 안 들어있거나 하더군요.

뭐가 빠졌나 보고 다 추가한 것 같아서 데이터 이상 없어 보이는데? 했다가 막상 돌리면

에러 발생해서 차근차근 찾아보니 이제는 MatzipRaw에 고유하게 있는 createdAt, updatedAt, unique key

이런 추가 필드들 작성 안 해줘서 애먹었네요.

그리고 제가 했던 작업이랑은 크게 상관은 없지만 데이터가 분명 같은 양식인데도 막상 보면

출력하는 형식이 다르더군요?

LICENSG_DE (인허가일자) 데이터가 일식은 2021-07-21인데 중식이랑 카페는 20110526 ...☹️☹️☹️

회의 때 준희님 작업한 내용 애로사항들이 좀 더 와닿았습니다.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

에고 ㅜㅜ 고생하셨어요 ㅜㅜ

mojh7 added 2 commits November 8, 2023 12:32
 - MatzipSummaryResponse에서 Reponse 오타 수정
 - MatzipSummaryResponse 거리 계산 dto 내부에서 호출 하도록 수정
 - 일부 파일 마지막 빈 줄 추가
 - 안 쓰는 import나 코드 제거
Copy link
Contributor

@OldRabbit736 OldRabbit736 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

장현님 코드 잘 봤습니다. 고생 많으셨습니다. 👍


private BooleanExpression isEqualCategory(MatzipListRetrieveCategory category) {
if (category == null || category == MatzipListRetrieveCategory.ALL) {
return matzipRaw.sanittnBizcondNm.isNotNull();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNotNull 이라는 BooleanExpression 이 있다는 것 처음 알았습니다. 유용해보이네요! 👍

this.type = type;
this.type = type == null ? MatzipListRetrieveType.DISTANCE : type;
this.category = category == null ? MatzipListRetrieveCategory.ALL : category;
this.page = page == null ? 1 : page;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 default 값을 DTO에서 이렇게 바로 할 수도 있군요. 간편하고 좋네요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

클래스에 @Builder 선언하는거면 @Builder.Default 으로 디폴트 값 선언되는데
저희 지금 생성자에 @Builder 선언하는 것은 저렇게 하는 것 말고는 다른 방법을 모르겠더라구요 ㅎㅎ

Comment on lines +21 to +33
DISTANCE() {
@Override
public List<MatzipSummaryResponse> retrieve(List<Matzip> matzipList, Location requestLocation, double range,
int page, int pageSize) {
List<MatzipSummaryResponse> matzips
= matzipList.stream()
.map(m -> MatzipSummaryResponse.of(m, requestLocation))
.filter(m -> m.getDistance() <= range)
.sorted(Comparator.comparingDouble(MatzipSummaryResponse::getDistance))
.toList();
return paging(matzips, page, pageSize);
}
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TriFunction 말고 다른 것으로 대체하셨군요. 이것이 익명 클래스인가요?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 TriFunction은 ( 파라미터, 파라미터2 ) <- 파라미터 개수 늘어나면 TriFunction도 계속 바꿔줘야되고
람다는 결국 파라미터도 적게 받고 결국 간단한 식들 (o1, o2) -> o1 - o2; 에 어울렸지 않나 싶습니다
그래서 바꾼거고 익명 클래스 인 것 같긴 한데 확신이 안들어서 gpt한테 물어보니 맞다고 합니다

네, 이것은 Java에서의 익명 클래스(Anonymous Class)입니다. 익명 클래스는 클래스를 이름없이 정의하고 인스턴스화하는 방법 중 하나입니다.

위의 코드에서 MatzipListRetrieveType 열거형은 추상 메소드 retrieve를 구현하기 위해 익명 클래스로 정의되어 있습니다. 각 열거 상수(DISTANCE와 AVG_RATING)는 해당 추상 메소드를 구현하고, 각각의 열거 상수가 retrieve 메소드를 구체화하고 있습니다.

이렇게 익명 클래스를 사용하면 열거 상수마다 다르게 동작하는 메소드를 구현할 수 있으며, 다형성을 활용하여 MatzipListRetrieveType에 대해 각기 다른 동작을 정의할 수 있습니다.

Comment on lines +65 to +68
// subList(0, 15) -> index 0부터 14까지
// subList는 기존 list의 view일 뿐이라 기존 list가 gc 대상이 되지않는 이슈가 있다고 함
// 그래서 새로운 복사본 생성해서 반환하도록 설정
return new ArrayList<>(matzips.subList(fromIndex, toIndex));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오 이건 저도 준희님과 같이 생각했습니다. 다른 API도 생각해보면 스레드에서 모든 로직이 끝나고 gc가 그 흔적들을 지울텐데 이곳도 마찬가지가 아닌가 생각이듭니다.

…matzip-list-filter-page

# Conflicts:
#	src/main/java/com/wanted/teamr/tastyfinder/api/exception/ErrorCode.java
#	src/main/java/com/wanted/teamr/tastyfinder/api/matzip/domain/Matzip.java
@mojh7 mojh7 merged commit 9b1c015 into develop Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☁️ feature 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 맛집 목록 조회 음식 종류 필터링, 페이징 구현과 MatzipRaw 실제 클래스로 전환
3 participants