feat: 식당 검색 필터링 옵션에 거리 가까운 순과 좋아요 순을 추가한다. #206
Conversation
| @@ -21,8 +21,8 @@ public class BookmarkService { | |||
| private final RestaurantRepository restaurantRepository; | |||
| private final BookmarkRepository bookmarkRepository; | |||
|
|
|||
| public BookmarkService(MemberRepository memberRepository, RestaurantRepository restaurantRepository, | |||
| BookmarkRepository bookmarkRepository) { | |||
| public BookmarkService(final MemberRepository memberRepository, final RestaurantRepository restaurantRepository, | |||
There was a problem hiding this comment.
final 처리 좋네요 👍
카키가 말씀해주신 전체 로직 final 처리 관련 부분은 백호도 의견이 같다면 처리 해봅시다!
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.distance, r.name asc | ||
| """), | ||
| ORDER_BY_BOOKMARK_COUNT_DESC("BOOKMARK", """ |
There was a problem hiding this comment.
저 안그래도 이것도 말씀드리고 싶은 부분이었는데요
현재 대부분의 쿼리문들이 문자열을 합친 형태로 구성되어있어서
작성에 불편함이 있습니다.
일관성을 위해서 일단 제가 맡은 부분 작업 할 때도 그렇게 코드를 작성하긴 했는데,
이러한 부분들 전부 편의를 위해서 Text Block으로 리팩토링하는 것은 어떻게 생각하시나요?
There was a problem hiding this comment.
17에서 지원해주는 방식대로 변경해주셔도 좋아요
There was a problem hiding this comment.
전체적으로 개선한다면 한 번 회의해서 리팩토링 범위를 정해 final, Text Block 등 일관성을 위한 리팩토링도 좋겠네요 !
한번 시간잡아서 얘기해보시죠 😁
| ORDER_BY_RATING_DESC("RATING", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.reviewRatingAverage desc | ||
| """), | ||
| ORDER_BY_NAME_ASC("SPELL", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.name | ||
| """), | ||
| ORDER_BY_ID_DESC("DEFAULT", """ | ||
| select r from Restaurant r | ||
| where (r.campusId = :campusId) and (:categoryId is null or r.categoryId = :categoryId) | ||
| order by r.id desc | ||
| """), | ||
| ORDER_BY_DISTANCE_ASC("DISTANCE", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.distance, r.name asc | ||
| """), | ||
| ORDER_BY_BOOKMARK_COUNT_DESC("BOOKMARK", """ | ||
| select r from Restaurant r | ||
| left join Bookmark b on b.restaurant = r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| group by r | ||
| order by count(b) desc, r.name asc | ||
| """), |
There was a problem hiding this comment.
RestaurantFindQuery를 단순 문자열로 찾아오는 부분이 조금 아쉽긴 하네요..
사용하는 부분에서는 매직 리터럴에 가까워보여서 코드에 대한 이해도가 부족한 상황에서는 "엇.. 이게뭐지" 싶습니다 🥲
이렇게 관리하는 큰 이유가 없다면 key를 Enum으로 관리하는 것은 어떨까요??
There was a problem hiding this comment.
enum값은 말그대로 enum 각 값의 형태를 나타내는 값이기 때문에 가능하면 외부인자를 통한 변환에 사용하지는 않으셨으면 해요. 말그대로 이 enum 값 그자체를 대표하는 값이어서 이 값을 토대로 외부의 값과 일치하는지 확인하는 용도로는 어색해요.
사실 구조자체가 여러 상태의 필터를 가진 호출에 대해서 하나의 api로 만들어 코드량을 줄이기위함으로 만든거긴한데 일반적인 구조는 아니긴해요.
There was a problem hiding this comment.
처음보는 구조라 의아하긴 했었는데 일단 정해져있는 구조로 정렬 상수를 추가했어요 😀
오리의 말씀대로라면 지금 구조는 상수를 관리하는 enum의 상수 값이 외부 인자에 영향을 받기 때문에 어색하므로
이를 분리하지 않고 레포지토리 계층에서 @Query 어노테이션으로 쿼리를 실행하도록 변경했으면 좋겠다는 의도가 맞을까요 ??
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.distance, r.name asc | ||
| """), | ||
| ORDER_BY_BOOKMARK_COUNT_DESC("BOOKMARK", """ |
There was a problem hiding this comment.
17에서 지원해주는 방식대로 변경해주셔도 좋아요
| ORDER_BY_RATING_DESC("RATING", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.reviewRatingAverage desc | ||
| """), | ||
| ORDER_BY_NAME_ASC("SPELL", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.name | ||
| """), | ||
| ORDER_BY_ID_DESC("DEFAULT", """ | ||
| select r from Restaurant r | ||
| where (r.campusId = :campusId) and (:categoryId is null or r.categoryId = :categoryId) | ||
| order by r.id desc | ||
| """), | ||
| ORDER_BY_DISTANCE_ASC("DISTANCE", """ | ||
| select r from Restaurant r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| order by r.distance, r.name asc | ||
| """), | ||
| ORDER_BY_BOOKMARK_COUNT_DESC("BOOKMARK", """ | ||
| select r from Restaurant r | ||
| left join Bookmark b on b.restaurant = r | ||
| where r.campusId = :campusId and r.categoryId = :categoryId | ||
| group by r | ||
| order by count(b) desc, r.name asc | ||
| """), |
There was a problem hiding this comment.
enum값은 말그대로 enum 각 값의 형태를 나타내는 값이기 때문에 가능하면 외부인자를 통한 변환에 사용하지는 않으셨으면 해요. 말그대로 이 enum 값 그자체를 대표하는 값이어서 이 값을 토대로 외부의 값과 일치하는지 확인하는 용도로는 어색해요.
사실 구조자체가 여러 상태의 필터를 가진 호출에 대해서 하나의 api로 만들어 코드량을 줄이기위함으로 만든거긴한데 일반적인 구조는 아니긴해요.
| void 캠퍼스id와_카테고리id가_일치하는_식당을_가까운_거리순으로_페이징해서_반환한다() { | ||
| Restaurant restaurant1 = createTestRestaurant(1L, 1L, "식당1", "주소1", 5); | ||
| Restaurant restaurant2 = createTestRestaurant(1L, 1L, "식당2", "주소2", 7); | ||
| Restaurant restaurant3 = createTestRestaurant(1L, 1L, "식당3", "주소3", 6); | ||
| restaurantRepository.saveAll(List.of(restaurant1, restaurant2, restaurant3)); | ||
|
|
||
| String query = ORDER_BY_DISTANCE_ASC.getQuery(); | ||
| Slice<Restaurant> page = restaurantQueryRepository.findPageByCampusIdAndCategoryId(query, 1L, 1L, | ||
| PageRequest.of(0, 3)); | ||
|
|
||
| assertThat(page).containsExactly(restaurant1, restaurant3, restaurant2); | ||
| } | ||
|
|
||
| @Test | ||
| void 캠퍼스id와_카테고리id가_일치하는_식당을_찜수가_많은순으로_페이징해서_반환한다() { |
There was a problem hiding this comment.
거리가 일치하거나 찜순이 일치하는 경우에 후순위로 이름으로 필터링되는 것도 테스트네이밍에 포함되면 좋을 것 같아요.
There was a problem hiding this comment.
앗 네이밍을 통해 후순위 정렬 조건이 있다는게 파악이 안되네요 😅
캠퍼스id와_카테고리id가_일치하는_식당을_가까운_거리순_가나다순으로_페이징해서_반환한다()와 같이
후순위 정렬 조건이 파악되도록 거리, 찜 수 정렬 테스트에 네이밍 적용 하였습니다 !
|
issue: #205
as-is
별점 순과가나다 순정렬만으로 원하는 조건을 맛집을 파악하기 힘들다.to-be