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] Search, Recommend 도메인 추가 #6

Merged
merged 9 commits into from
Oct 18, 2024
Merged

[Feat] Search, Recommend 도메인 추가 #6

merged 9 commits into from
Oct 18, 2024

Conversation

baeksom
Copy link
Member

@baeksom baeksom commented Oct 16, 2024

사용자 검색 조회 및 조회수 기반 장소 추천 기능 구현

  1. 사용자 검색 조회
  • 로그인한 사용자가 있을 경우: 검색한 장소를 Search 테이블에 사용자와 함께 저장합니다.
  • 로그인한 사용자가 없을 경우: 검색한 장소의 조회수만 증가시킵니다.
  • 처음 검색된 장소일 경우: Location 테이블에 해당 장소를 등록합니다.
  • 이미 검색 기록이 있는 장소일 경우: 해당 장소의 조회수를 증가시킵니다.
  1. 조회수 기반 장소 추천 기능
  • 사용자의 현재 위치(경도, 위도)를 기반으로 반경 1km 이내의 장소를 검색하여,
  • 조회수 순으로 상위 3개 장소를 출력합니다.

@baeksom baeksom self-assigned this Oct 16, 2024
@baeksom baeksom requested a review from SongJaeHoonn October 16, 2024 06:13
@Getter
@Builder
@AllArgsConstructor
@RequiredArgsConstructor
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dto에 AllArgsConstructorRequiredArgsConstructor를 사용하시는 이유가 있나요?


public static SearchResponseDto toSearchResponseDto(Location location) {
return SearchResponseDto.builder()
.location(location.getLocation())
Copy link
Collaborator

Choose a reason for hiding this comment

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

클래스명 Location과 문자열 형태의 변수명인 location이 같아서 헷갈릴 여지가 있을 것 같아요
name이나 locationName으로 변경하는건 어떠신가요??

locationRepository.save(location);
} else {
// 기존 장소 검색 시
location = locationRepository.findByLocation(locationName).get();
Copy link
Collaborator

Choose a reason for hiding this comment

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

findByLocation() 을 조건문에서도 사용하고, else문 내에서도 사용하는데, 이렇게 되면 테이블 스캔을 두 번 하게 되어 비효율적일 것 같습니다.
처음 한 번만 조회 후 그 결과를 계속 사용하는 방식이 나아보여요


if(userService.findByKakaoId(userService.getCurrentUserId()) != null) {
// 현재 로그인한 사용자
User user = userService.findByKakaoId(userService.getCurrentUserId());
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분도 메서드가 중복 호출되어 비효율적이라고 생각합니다

// 기존 장소 검색 시
location = locationRepository.findByLocation(locationName).get();
// 검색수 증가
location.incrementViews();
Copy link
Collaborator

@SongJaeHoonn SongJaeHoonn Oct 17, 2024

Choose a reason for hiding this comment

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

트랜잭션이 동시다발적으로 일어나면 동시성 문제때문에 검색 수가 정확히 반영이 안될 수도 있을 것 같은데 어떻게 생각하시나요??

Copy link
Member Author

@baeksom baeksom Oct 17, 2024

Choose a reason for hiding this comment

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

findByName 메서드에 대해서 "@lock(LockModeType.PESSIMISTIC_WRITE)"으로 락 걸어주는거 어떤가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

사실 서비스가 작아서 괜찮을 것 같긴 하지만 말씀해주신대로 비관적 lock 걸어서 처리해도 좋을 것 같습니다

@baeksom baeksom requested a review from SongJaeHoonn October 17, 2024 06:20
@SongJaeHoonn
Copy link
Collaborator

수고하셨습니다!

@baeksom baeksom merged commit c873430 into main Oct 18, 2024
1 check passed
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