-
Notifications
You must be signed in to change notification settings - Fork 0
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
Update recommendations ASC 방식으로 개선 #15
Conversation
Order("restaurant_recommendation_request_id DESC"). | ||
Order("restaurant_recommendation_request_id ASC"). |
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.
p2; 이거 restaurant_recommendation_request_id가 아니라 restaurant_recommendation_id로 페이지네이션해야해요🙏 모델 구조랑 로직 구조를 먼저 이해해보시는 건 어떨까요?
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.
Cursor를 받아올때 recommendation 객체의 top 10개를 가져와야하는 상황에서
그 객체를 오름차순으로 바꿔야 하는걸로 이해했는데, 제가 이해한게 맞을까요??@mokhs00
파악한 부분
이슈 내용 파악한 부분
궁금한점
수정한 부분
|
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.
pre-approve
@@ -46,7 +46,7 @@ func (r *restaurantRecommendationRepository) FindAllByRestaurantRecommendationRe | |||
RestaurantRecommendationRequestID: restaurantRecommendationRequestID, | |||
}, | |||
whereConditions...). | |||
Order("restaurant_recommendation_request_id DESC"). | |||
Order("restaurant_recommendation_id DESC"). |
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.
이건 ASC로 해야해용 추천 데이터가 생기는 구조랑 페이지네이션을 어떻게 엮을지 생각해보면 이해하실 수 있을 겁니당
- 추천 요청(restaurant_recommendation_request)에 매핑되는 추천 데이터(restaurant_recommendation)를 처음에 10 개 생성하고 이후 추천 데이터가 부족하면 추가로 생성하는 식인데, 이러면 페이지네이션할 때 desc로 할 수가 없어용
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.
추가로 위에 이 부분도 restaurant_recommendation_request_id가 아니라 restaurant_recommendation_id를 사용하고 부등호도 알맞게 수정해두어야 합니당
// as-is
whereConditions = append(whereConditions, "restaurant_recommendation_request_id < ?", *cursorRestaurantRecommendationID)
// to-be
whereConditions = append(whereConditions, "restaurant_recommendation_id > ?", *cursorRestaurantRecommendationID)
제가 너무 날림으로 코드를 읽은 것 같습니다. 제가 이해한 내용이 맞는지 다시 한번 확인해 주실 수 있을까요? @mokhs00 requestRestaurantRecommendation해당 api에서 사용자 요청이 들어오면 다음 행동을 합니다.
질문 14번 단계 에서 우리가 수정해야 하는 부분이 보이는 것 같습니다. 지금은 사용자의 위도, 경도에 상관없이 항상 10개의 값을 result := r.db.
Order("recommendation_score DESC").
Limit(limit).
Find(&existingRestaurants) 이런 식으로 받아오는데, 이 로직을 고도화 해야 할것 같은데, 이것이 issue #8 인것 같은데 맞나요? listRecommendedRestaurants본격적으로 이 PR에서 수정해야할 API 부분입니다.
질문 2
Order("restaurant_recommendation_id ASC").
whereConditions = append(whereConditions, "restaurant_recommendation_id > ?", *cursorRestaurantRecommendationID) 결과는 다음과 같고요. 추가로 하고 싶은 말오늘 JPA 강의를 들으면서 서버 코드에 대한 이해력이 늘어나고 나서 다시 이 코드를 보니까 어느정도 읽을 수 있는 것 같습니다. |
@falconlee236 ㅎㅎ 잘하고 싶어서 그러시는 거니까 괜찮습니당 죄송 -> 사랑 질문 답변 드리면,,
넵넵 위에 이해하신 내용이 맞아용
넵넵 맞습니당 위치 기반 검색이 들어가야해용 인덱스 관련해서도 찾아봐야합니당
넵넵 이해하신 게 맞아요 로직 플로우는 아래 처럼 됩니당
|
…_request_id to recommendation_id
최종적으로 수정했고 여기에 이모지 남겨주시면 바로 merge 하겠습니다 |
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.
LGTM 👍
Summary
ListRecommendedRestaurants
함수애 있는nextCursor
변수 선언 부분에 있는recommendations
부분을 오름차순으로 변경했습니다.Description
변경사항
restaurant_recommendation_service.go
파일의 171번째 줄에 있는 nextCursor 기능을 변경했습니다.recommendations
변수에만 영향을 받는데, 이 변수는이 부분에서 값을 가져오기 때문에
FindAllByRestaurantRecommendationRequestID
이 함수만 변경하면 된다고 판단했습니다.FindAllByRestaurantRecommendationRequestID
해당 함수에서 order by를 DESC에서 ASC로 변경했습니다.궁금한점
이 파일 하나만 바꾸는게 맞는지 잘 모르겠네요.. 예를 들어서 음식점 이미지도 findbyid 함수가 있는데 이 함수는 nextcursor랑 아무 연관이 없으니까 orderby를 추가할 필요는 없는것 같아서 수정 안했습니다.
ListRecommendedRestaurants
함수에 있는 다른 find 함수도 정렬이 필요한지 궁금합니다.