-
Notifications
You must be signed in to change notification settings - Fork 0
refactor [#451] 2차 스프린트 기간 코드리뷰 사항 반영 #469
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
base: develop
Are you sure you want to change the base?
Changes from all commits
fc57fa8
bfbb1ec
6df48d3
3b3f43c
9641caf
48cbdfc
b558c4a
259ef8c
c2e22e3
f619096
9f3f298
9fb1e15
b787225
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -3,8 +3,11 @@ | |||||||||||||||||
| import jakarta.validation.constraints.Max; | ||||||||||||||||||
| import jakarta.validation.constraints.Min; | ||||||||||||||||||
| import jakarta.validation.constraints.NotBlank; | ||||||||||||||||||
| import java.util.Arrays; | ||||||||||||||||||
| import java.util.List; | ||||||||||||||||||
| import java.util.Optional; | ||||||||||||||||||
| import java.util.Arrays; | ||||||||||||||||||
| import java.util.Objects; | ||||||||||||||||||
| import java.util.Collections; | ||||||||||||||||||
| import lombok.RequiredArgsConstructor; | ||||||||||||||||||
| import org.sopt.confeti.api.performance.dto.request.GetExpectedPerformanceRequest; | ||||||||||||||||||
| import org.sopt.confeti.api.performance.dto.response.ArtistPerformancesResponse; | ||||||||||||||||||
|
|
@@ -144,9 +147,14 @@ public ResponseEntity<BaseResponse<?>> searchAutoComplete( | |||||||||||||||||
| public ResponseEntity<BaseResponse<?>> getRecommendPerformanceId( | ||||||||||||||||||
| @UserId(require = false) Long userId | ||||||||||||||||||
| ) { | ||||||||||||||||||
| RecommendMusicsPerformanceDTO recommendMusicsDTO = performanceFacade.getRecommendPerformanceId(userId); | ||||||||||||||||||
| return ApiResponseUtil.success(SuccessMessage.SUCCESS, | ||||||||||||||||||
| RecommendMusicsPerformanceResponse.from(recommendMusicsDTO)); | ||||||||||||||||||
| Optional<RecommendMusicsPerformanceDTO> performanceDto = Objects.isNull(userId) | ||||||||||||||||||
| ? performanceFacade.getRecommendPerformanceIdByRand() | ||||||||||||||||||
| : performanceFacade.getRecommendPerformanceIdByUserId(userId); | ||||||||||||||||||
|
|
||||||||||||||||||
| return performanceDto | ||||||||||||||||||
| .map(performance -> | ||||||||||||||||||
| ApiResponseUtil.success(SuccessMessage.SUCCESS, RecommendMusicsPerformanceResponse.from(performance))) | ||||||||||||||||||
| .orElseGet(()-> ApiResponseUtil.success(SuccessMessage.SUCCESS, Collections.emptyMap())); | ||||||||||||||||||
|
Comment on lines
+154
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
이렇게 작성하는건 어떠신가요?
Comment on lines
+156
to
+157
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. orElse와 orElseGet의 차이를 인지하고 사용하신 부분이 좋네요 :) |
||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| @Permission(role = {Role.GENERAL}) | ||||||||||||||||||
|
|
||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,15 @@ | ||
| package org.sopt.confeti.api.performance.facade; | ||
|
|
||
| import java.time.LocalDate; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Set; | ||
| import java.util.HashMap; | ||
| import java.util.HashSet; | ||
| import java.util.Arrays; | ||
| import java.util.ArrayList; | ||
| import java.util.Optional; | ||
| import java.util.stream.Collectors; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
|
|
@@ -26,6 +27,7 @@ | |
| import org.sopt.confeti.api.performance.facade.dto.response.RecommendPerformancesDTO; | ||
| import org.sopt.confeti.api.performance.facade.dto.response.SearchACPerformancesDTO; | ||
| import org.sopt.confeti.api.performance.facade.dto.response.PerformanceIdsDTO; | ||
| import org.sopt.confeti.api.performance.vo.UserPerformanceRecordVO; | ||
| import org.sopt.confeti.domain.artist_favorite.ArtistFavorite; | ||
| import org.sopt.confeti.domain.artist_favorite.application.ArtistFavoriteService; | ||
| import org.sopt.confeti.domain.concert.Concert; | ||
|
|
@@ -36,6 +38,7 @@ | |
| import org.sopt.confeti.domain.festival.Festival; | ||
| import org.sopt.confeti.domain.festival.application.FestivalService; | ||
| import org.sopt.confeti.domain.festival_favorite.application.FestivalFavoriteService; | ||
| import org.sopt.confeti.domain.setlist.SetlistType; | ||
| import org.sopt.confeti.domain.setlist.application.SetlistService; | ||
| import org.sopt.confeti.domain.timetable_festival.application.TimetableFestivalService; | ||
| import org.sopt.confeti.domain.user.application.UserService; | ||
|
|
@@ -61,6 +64,8 @@ public class PerformanceFacade { | |
|
|
||
| private static final int RECENT_PERFORMANCES_SIZE = 7; | ||
| private static final int RECOMMEND_MUSIC_SIZE = 3; | ||
| private static final int MUSIC_FETCH_SIZE = 5; | ||
| private static final int SELECTED_ARTISTS_SIZE = 3; | ||
| private static final boolean PERSONALIZED = true; | ||
| private static final boolean UNPERSONALIZED = false; | ||
|
|
||
|
|
@@ -261,98 +266,89 @@ public SearchACPerformancesDTO searchACPerformances(String term, int limit, Perf | |
| ); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public RecommendMusicsPerformanceDTO getRecommendPerformanceId(final Long userId) { | ||
|
|
||
| Performance performance = performanceService.getPerformanceByUserFavorites(userId); | ||
| if (userId == null || performance == null) { | ||
| performance = performanceService.getPerformanceByRand(); | ||
| } | ||
|
|
||
| return RecommendMusicsPerformanceDTO.from(performance); | ||
| public Optional<RecommendMusicsPerformanceDTO> getRecommendPerformanceIdByRand() { | ||
| return performanceService.getPerformanceByRand() | ||
| .map(RecommendMusicsPerformanceDTO::from); | ||
| } | ||
|
|
||
| protected Set<String> setArtistsByRandom(Performance performance) { | ||
| List<PerformanceArtist> performanceArtists = performance.getArtists(); | ||
|
|
||
| if (performanceArtists.isEmpty()) { | ||
| return Collections.emptySet(); | ||
| } | ||
|
|
||
| List<String> artistList = performanceArtists.stream() | ||
| .map(PerformanceArtist::getArtistId).distinct().collect(Collectors.toList()); | ||
| Collections.shuffle(artistList); | ||
|
|
||
| int artistCount = Math.min(artistList.size(), 3); | ||
| return new HashSet<>(artistList.subList(0, artistCount)); | ||
| public Optional<RecommendMusicsPerformanceDTO> getRecommendPerformanceIdByUserId(final Long userId) { | ||
| return performanceService.getPerformanceByUserFavorites(userId) | ||
| .map(RecommendMusicsPerformanceDTO::from); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public RecommendMusicsDTO getNewRecommendMusics(long performanceId, List<String> musicIds) { | ||
| Performance performance = performanceService.getPerformanceById(performanceId); | ||
| Set<String> selectedArtistIds = setArtistsByRandom(performance); | ||
| Set<String> selectedArtistIds = selectRandomArtistIds(performance); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 무작위 아티스트 아이디를 추출할 때 애플리케이션 레벨, 데이터베이스 레벨 각각 어떤 장단점이 있었고 애플리케이션 레벨 구현을 선택하신 이유가 궁금합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 추가로 아티스트 아이디를 가져올 때 서비스에 요청하지 않고 트랜잭션 외부에서 공연 엔티티에서 추출하신 이유가 궁금합니다! |
||
| Set<String> existingMusicIds = (musicIds == null || musicIds.isEmpty()) | ||
| ? Collections.emptySet() | ||
| : musicIds.stream().flatMap(ids -> Arrays.stream(ids.split(","))) | ||
| .map(String::trim).collect(Collectors.toSet()); | ||
| : musicIds.stream() | ||
| .flatMap(ids -> Arrays.stream(ids.split(","))) | ||
| .map(String::trim) | ||
| .collect(Collectors.toSet()); | ||
|
|
||
| List<String> artistIdList = new ArrayList<>(selectedArtistIds); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 복수형 (-s, -List) 중 하나만 작성하시면 일관성 측면에서 더 좋을 것 같아요! |
||
| List<ConfetiMusic> recommendMusics = recommendMusicsByArtistCount(artistIdList, existingMusicIds); | ||
|
|
||
| return RecommendMusicsDTO.from(recommendMusics); | ||
| } | ||
|
|
||
| private List<ConfetiMusic> recommendMusicsByArtistCount(List<String> artistIdList, Set<String> existingMusicIds) { | ||
| int artistCount = artistIdList.size(); | ||
| if (artistCount == 1) { | ||
| return recommendForSingleArtist(artistIdList, existingMusicIds); | ||
| } | ||
| if (artistCount == 2) { | ||
| return recommendForTwoArtists(artistIdList, existingMusicIds); | ||
| protected Set<String> selectRandomArtistIds(Performance performance) { | ||
| List<PerformanceArtist> performanceArtists = performance.getArtists(); | ||
|
|
||
|
Comment on lines
+296
to
+298
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 별도의 메서드로 분리하신 부분 좋은 것 같아요 :) |
||
| if (performanceArtists.isEmpty()) { | ||
| return Collections.emptySet(); | ||
| } | ||
| return recommendForMultipleArtists(artistIdList, existingMusicIds); | ||
| } | ||
|
|
||
| private List<ConfetiMusic> recommendForSingleArtist(List<String> artistIdList, Set<String> existingMusicIds) { | ||
| return musicAPIHandler.getFilteredTopSongsByArtist( | ||
| artistIdList.getFirst(), RECOMMEND_MUSIC_SIZE, existingMusicIds | ||
| ); | ||
| } | ||
| List<String> artistList = performanceArtists.stream() | ||
| .map(PerformanceArtist::getArtistId).distinct().collect(Collectors.toList()); | ||
| Collections.shuffle(artistList); | ||
|
|
||
| private List<ConfetiMusic> recommendForTwoArtists(List<String> artistIdList, Set<String> existingMusicIds) { | ||
| List<ConfetiMusic> musics = new ArrayList<>(); | ||
| musics.addAll(musicAPIHandler.getFilteredTopSongsByArtist(artistIdList.getFirst(), 2, existingMusicIds)); | ||
| musics.addAll(musicAPIHandler.getFilteredTopSongsByArtist(artistIdList.getLast(), 1, existingMusicIds)); | ||
| return musics; | ||
| int artistCount = Math.min(artistList.size(), SELECTED_ARTISTS_SIZE); | ||
| return new HashSet<>(artistList.subList(0, artistCount)); | ||
| } | ||
|
|
||
| private List<ConfetiMusic> recommendForMultipleArtists(List<String> artistIdList, Set<String> existingMusicIds) { | ||
| private List<ConfetiMusic> recommendMusicsByArtistCount(List<String> artistIdList, Set<String> existingMusicIds) { | ||
| List<ConfetiMusic> musics = new ArrayList<>(); | ||
| for (String artistId : artistIdList) { | ||
| if (musics.size() >= RECOMMEND_MUSIC_SIZE) { | ||
| break; | ||
| Set<String> seenMusicIds = new HashSet<>(existingMusicIds); | ||
| int round = 0; | ||
|
|
||
| while (musics.size() < RECOMMEND_MUSIC_SIZE && round < MUSIC_FETCH_SIZE) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. round의 limit 값이랑 음악 조회 개수의 값이 같은 상수를 사용하는건가요? 제 의견은 매직 넘버를 상수화하신 것이라면, 이름을 따로 두어야 한다고 생각해요. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 동의합니다!!! MAX_FETCH_ATTEMPTS 같은 단어로 바꾸는 것도 좋을 것 같네요 ㅎㅎ |
||
| for (String artistId : artistIdList) { | ||
| if (musics.size() >= RECOMMEND_MUSIC_SIZE) break; | ||
|
|
||
| List<ConfetiMusic> topSongs = musicAPIHandler.getFilteredTopSongsByArtist( | ||
| artistId, MUSIC_FETCH_SIZE, seenMusicIds | ||
| ); | ||
|
|
||
| if (round < topSongs.size()) { | ||
| ConfetiMusic song = topSongs.get(round); | ||
| if (!seenMusicIds.contains(song.getId())) { | ||
| musics.add(song); | ||
| seenMusicIds.add(song.getId()); | ||
| } | ||
| } | ||
| } | ||
| musics.addAll(musicAPIHandler.getFilteredTopSongsByArtist(artistId, 1, existingMusicIds)); | ||
| round++; | ||
| } | ||
|
|
||
| return musics; | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public ConfetiRecordDTO getConfetiRecord(final long userId) { | ||
| validateExistUser(userId); | ||
|
|
||
| List<Long> timetableFestivalIds = timetableFestivalService.findFestivalIdsByUserId(userId); | ||
| List<Long> setListFestivalIds = setlistService.findFestivalIdsByUserId(userId); | ||
| List<Long> setListConcertIds = setlistService.findConcertIdsByUserId(userId); | ||
| List<Long> setListFestivalIds = setlistService.findMusicIdsByUserId(userId, SetlistType.FESTIVAL); | ||
| List<Long> setListConcertIds = setlistService.findMusicIdsByUserId(userId, SetlistType.CONCERT); | ||
|
|
||
| Set<Long> uniqueFestivalIds = new HashSet<>(); | ||
| uniqueFestivalIds.addAll(timetableFestivalIds); | ||
| uniqueFestivalIds.addAll(setListFestivalIds); | ||
|
|
||
| int totalCount = uniqueFestivalIds.size() + setListConcertIds.size(); | ||
| UserPerformanceRecordVO record = new UserPerformanceRecordVO( | ||
| timetableFestivalIds, | ||
| setListFestivalIds, | ||
| setListConcertIds | ||
| ); | ||
|
|
||
| return ConfetiRecordDTO.of(totalCount, timetableFestivalIds.size(), | ||
| setListFestivalIds.size() + setListConcertIds.size()); | ||
| return ConfetiRecordDTO.from(record); | ||
| } | ||
|
|
||
| protected void validateExistUser(final long userId) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,17 @@ | ||
| package org.sopt.confeti.api.performance.facade.dto.response; | ||
|
|
||
| import org.sopt.confeti.api.performance.vo.UserPerformanceRecordVO; | ||
|
|
||
| public record ConfetiRecordDTO( | ||
| long totalCount, | ||
| long timetableCount, | ||
| long setlistCount | ||
| ) { | ||
| public static ConfetiRecordDTO of(final long totalCount, final long timetableCount, final long setlistCount) { | ||
| public static ConfetiRecordDTO from(UserPerformanceRecordVO record) { | ||
| return new ConfetiRecordDTO( | ||
| totalCount, timetableCount, setlistCount | ||
| record.getTotalUniquePerformanceCount(), | ||
| record.getTimetableFestivalCount(), | ||
| record.getSetListPerformanceCount() | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| package org.sopt.confeti.api.performance.vo; | ||
|
|
||
| import java.util.HashSet; | ||
| import java.util.List; | ||
| import java.util.Set; | ||
|
|
||
| public record UserPerformanceRecordVO( | ||
| List<Long> timetableFestivalIds, | ||
| List<Long> setListFestivalIds, | ||
| List<Long> setListConcertIds | ||
| ) { | ||
| public int getTotalUniquePerformanceCount() { | ||
| Set<Long> uniqueFestivalIds = new HashSet<>(timetableFestivalIds); | ||
| uniqueFestivalIds.addAll(setListFestivalIds); | ||
| return uniqueFestivalIds.size() + setListConcertIds.size(); | ||
| } | ||
|
|
||
| public int getTimetableFestivalCount() { | ||
| return timetableFestivalIds.size(); | ||
| } | ||
|
|
||
| public int getSetListPerformanceCount() { | ||
| return setListFestivalIds.size() + setListConcertIds.size(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package org.sopt.confeti.api.user.controller; | ||
|
|
||
| import org.sopt.confeti.global.exception.ConfetiException; | ||
| import org.sopt.confeti.global.message.ErrorMessage; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum UserFavoriteSortType { | ||
| CREATED_AT("createdAt"), | ||
| ALPHABETICALLY("alphabetically"); | ||
|
|
||
| private final String value; | ||
|
|
||
| UserFavoriteSortType(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public static UserFavoriteSortType from(String value) { | ||
| return Arrays.stream(values()) | ||
| .filter(type -> type.value.equalsIgnoreCase(value)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new ConfetiException(ErrorMessage.BAD_REQUEST)); | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| package org.sopt.confeti.api.user.controller; | ||
|
|
||
| import org.sopt.confeti.global.exception.ConfetiException; | ||
| import org.sopt.confeti.global.message.ErrorMessage; | ||
|
|
||
| import java.util.Arrays; | ||
|
|
||
| public enum UserTimetableSortType { | ||
| OLDEST_FIRST("oldestFirst"), | ||
| CREATED_AT("createdAt"); | ||
|
|
||
| private final String value; | ||
|
|
||
| UserTimetableSortType(String value) { | ||
| this.value = value; | ||
| } | ||
|
|
||
| public static UserTimetableSortType from(String value) { | ||
| return Arrays.stream(values()) | ||
| .filter(type -> type.value.equalsIgnoreCase(value)) | ||
| .findFirst() | ||
| .orElseThrow(() -> new ConfetiException(ErrorMessage.BAD_REQUEST)); | ||
| } | ||
|
|
||
| public String getValue() { | ||
| return value; | ||
| } | ||
| } |
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.
삼항 연산자를 통해 직관적으로 파악할 수 있네요! 좋습니다