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

KAN-80 (BE) 식당 - 좋아요 조회/변경 API 개발 #51

Merged
merged 6 commits into from
May 25, 2024
Merged

Conversation

one0955
Copy link
Collaborator

@one0955 one0955 commented May 21, 2024

테스트를 위해 최신 DB로 업데이트를 해야 하기때문에 mysql_batch pr(KAN-81)이 머지된 후 실행하고 테스트 해야 합니다.

Copy link
Member

@sinkyoungdeok sinkyoungdeok left a comment

Choose a reason for hiding this comment

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

리뷰쪽 한번 코드 읽어보시고 진행부탁드립니다.

@one0955 one0955 closed this May 23, 2024
@one0955 one0955 reopened this May 23, 2024
Comment on lines 24 to 25
// 세컨더리 생성자 추가 id가 null이어도 db엔 자동 숫자 생성됨
constructor(restaurantId: Long, email: Long) : this(null, restaurantId, email)
Copy link
Member

Choose a reason for hiding this comment

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

이거 추가할 필요 있나요? 없어도 되겠습니다.

Copy link
Member

Choose a reason for hiding this comment

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

아직 확인이 안된거죠?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

아 이거 그 뭐였더라 constructor 생성할 일 있었는 데 저거 없으니 id(PK 값)도 생성자에 넣어줘야 되더라고요. 근데 그건 자동 증가 값이라서.. 아무튼 저거 없으니 에러가 나서 넣었습니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

근데 이제 수정하면서 사용되는 곳이 사라졌네요.. 없애겠습니다 ㅎ

.limit(pageable.pageSize.toLong())
.fetch()

return restaurantIds.map { restaurantRepository.findDtoById(it)?.toDto() ?: throw NotFoundRestaurantException() }
Copy link
Member

Choose a reason for hiding this comment

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

이렇게 호출하지말고, 직접 쿼리로 작성해주세여.
repository 끼리는 호출하면 안됩니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LikeRestaurantService.kt

// 좋아요한 식당 아이디 리스트 반환
val restaurantIds: List<Long> = restaurantLikeRepository.findRestaurantLikesByUserId(userId, pageable)
// 기존에 있는 식당 조회 repository 재사용
val restaurantProjections: Page<RestaurantProjectionDto> = restaurantRepository.findDtoByIds(restaurantIds, userId, true, pageable)
// restaurantDto로 변환하여 리턴
return GetLikeRestaurantsResponse(restaurantProjections.map{it.toDto()})

RestaurantProjectionDto 조회 쿼리 있는데 또 만드는 건 비효율적이라 못하겠습니다. 이 정도가 제가 타협가능한 마지노선인데 괜찮으면 이대로 추가 PR올리겠습니다

Copy link
Member

Choose a reason for hiding this comment

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

말씀주신방법도 틀린방법은 아니지만 쿼리가 비효율적으로 한번더 호출되는게 성능상으로 비효율적입니다~
repository에서 아예 새롭게 쿼리를 작성하라고 말씀드린 것은 아니였고
restaurantRepository.findDtoByIds 이 쿼리에서 유저를 대상으로 좋아요를 한 데이터를 가져오는 점만 다를 것 같아요.
그럼 이부분에서 공통적인부분들은 함수로 빼서 같이 사용하면 될 것 같아요.
혹시 보시고 이해안되시면 @one0955 님이 말씀주신방법으로 반영해놓으시면 리팩토링하는건 제가 따로 해두겠습니다.

Copy link
Member

@sinkyoungdeok sinkyoungdeok left a comment

Choose a reason for hiding this comment

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

리뷰 준비되시면 comment로한번씩 언급부탁드립니다.

Comment on lines 24 to 25
// 세컨더리 생성자 추가 id가 null이어도 db엔 자동 숫자 생성됨
constructor(restaurantId: Long, email: Long) : this(null, restaurantId, email)
Copy link
Member

Choose a reason for hiding this comment

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

아직 확인이 안된거죠?

@sinkyoungdeok sinkyoungdeok merged commit 259f9fb into main May 25, 2024
3 checks passed
@sinkyoungdeok sinkyoungdeok deleted the KAN-80 branch May 25, 2024 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants