refactor [#451] 2차 스프린트 기간 코드리뷰 사항 반영

[Ivoryeee]

🔗 Issue Number

closes #451

🔙 Previous PR

• #(이전 PR 번호)

📌 To-do

• @EntityListeners 적용 및 사용되지 않는 createdAt 파라미터 제거
• SortType Enum으로 수정
• null 값 삭제 및 존재하지 않는 값에 대해 emptyMap 반환되도록 수정
• 음악 추천 로직 분기 제거 및 공통 로직으로 통합
• 내 공연 관람 기록 조회 API 에서 VO 도입
• import 구문 와일드카드 제거

✨Description

@EntityListeners 적용 및 사용되지 않는 createdAt 파라미터 제거

필요한 생성자 파라미터 createdAt 제거하고 @EntityListeners(AuditingEntityListener.class) 어노테이션을 추가해 @CreatedDate가 선언된 필드에 생성 시점의 시간이 자동으로 주입되도록 설정했습니다.

SortType Enum으로 수정

기존에 문자열로 사용하던 sortBy 파라미터를 UserFavoriteSortType, UserTimetableSortType와 같은 enum으로 변경하여 타입 안정성을 높였습니다.
기존에는 문자열 기반 비교로 인해 오타나 유효하지 않은 값에 대한 검증이 Controller나 Service 단에서 별도로 필요했지만, enum을 도입함으로써 잘못된 입력 값에 대한 검증 책임을 enum 내부의 from() 메서드로 위임하였습니다.

public enum UserFavoriteSortType {Add commentMore actions
    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;
    }

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;
    }
}

null 값 삭제 및 존재하지 않는 값에 대해 emptyMap 반환되도록 수정

기존에는 null을 직접 비교하거나 반환값으로 사용하는 방식이 있어 NPE의 가능성이 존재했습니다. 이를 방지하고 코드의 안정성과 가독성을 높이기 위해 리팩토링을 적용했습니다.

1. Objects.isNull()을 사용해 명시적인 null 비교를 제거하고 더 읽기 쉬운 방식으로 변경하였습니다.
2. Optional을 적용해 파사드 계층에서 반환값으로 Optional 형식을 사용하여 null 처리 로직을 명확히 하고 불필요한 null 체크를 제거했습니다.
3. 컨트롤러 계층에서는 Optional.orElseGet()을 통해 null인 경우 빈 객체 형태인 Collections.emptyMap()을 반환하도록 했습니다.

// 파서드 계층 
   public Optional<UpcomingPerformanceDTO> getUpcomingPerformance(final long userId) {Add commentMore actions

        validateExistUser(userId);

        Performance performance = performanceService.getUpcomingPerformanceByUserId(userId);
        if (Objects.isNull(performance)) {
            return Optional.empty();
        }
        return Optional.of(UpcomingPerformanceDTO.from(performance));
    }

// 컨트롤러 계층 
    public ResponseEntity<BaseResponse<?>> getUpcomingPerformance(
            @UserId Long userId
    ) {
        Optional<UpcomingPerformanceDTO> upcomingPerformanceDTO = userFavoriteFacade.getUpcomingPerformance(userId);
        return upcomingPerformanceDTO
                .map(performanceDTO ->
                        ApiResponseUtil.success(SuccessMessage.SUCCESS, UpcomingPerformanceResponse.of(performanceDTO, s3FileHandler)))
                .orElseGet(() -> ApiResponseUtil.success(SuccessMessage.SUCCESS, Collections.emptyMap()));
    }

음악 추천 로직 분기 제거 및 공통 로직으로 통합

기존 음악 추천 로직은 아티스트 수에 따라 분기 처리가 하드코딩되어 있어 유지보수와 재사용이 어려운 구조였습니다. 따라서, 아티스트 수에 따라 나뉘던 조건문을 제거하고 아티스트 수에 상관없이 동일한 방식으로 추천이 동작되도록 수정했습니다.

1.각 아티스트별 상위 곡 리스트(MUSIC_FETCH_SIZE만큼)를 가져옴
2. round 인덱스를 기준으로 곡을 골라 추천 리스트에 추가
3. 중복된 곡은 건너뛰고, 추천 곡이 원하는 개수만큼(RECOMMEND_MUSIC_SIZE) 모이면 종료
4. 최대 round는 MUSIC_FETCH_SIZE로 제한

 private List<ConfetiMusic> recommendMusicsByArtistCount(List<String> artistIdList, Set<String> existingMusicIds) {
    List<ConfetiMusic> musics = new ArrayList<>();
    Set<String> seenMusicIds = new HashSet<>(existingMusicIds);

    // 각 아티스트별 추천 곡 리스트를 round 인덱스로 순차적으로 탐색
    int round = 0;

    // 추천 곡이 원하는 개수(RECOMMEND_MUSIC_SIZE)만큼 모일 때까지 반복
    // 단, 무한 루프 방지를 위해 round가 MUSIC_FETCH_SIZE 이상이 되면 중단
    while (musics.size() < RECOMMEND_MUSIC_SIZE && round < MUSIC_FETCH_SIZE) {
        for (String artistId : artistIdList) {
            if (musics.size() >= RECOMMEND_MUSIC_SIZE) break;

            // 각 아티스트의 필터링된 상위 곡 리스트를 가져옴
            List<ConfetiMusic> topSongs = musicAPIHandler.getFilteredTopSongsByArtist(
                    artistId, MUSIC_FETCH_SIZE, seenMusicIds
            );

            // 현재 round 인덱스에 해당하는 곡이 있다면 추천 리스트에 추가
            if (round < topSongs.size()) {
                ConfetiMusic song = topSongs.get(round);
                if (!seenMusicIds.contains(song.getId())) {
                    musics.add(song);
                    seenMusicIds.add(song.getId());
                }
            }
        }
        round++;
    }

    return musics;
}

내 공연 관람 기록 조회 API 에서 VO 도입

기존에는 Facade 계층에서 직접 공연 관람 기록 데이터를 조합하고 DTO로 변환하는 로직이 포함되어 있어 계층 간 책임이 모호하고 가독성이 저하되는 문제가 있었습니다. 따라서 VO를 도입해 해당 부분을 개선했습니다.

// Facade
        UserPerformanceRecordVO record = new UserPerformanceRecordVO(
                timetableFestivalIds,
                setListFestivalIds,
                setListConcertIds
        );

        return ConfetiRecordDTO.from(record);

// VO
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();
    }
}

import 구문 와일드카드 제거

Java 스타일 가이드의 권고에 따라 와일드카드 import 대신 실제로 사용하는 클래스와 어노테이션을 명시적으로 import 해 코드의 의도가 명확히 드러나는 방식으로 수정했습니다.

[ch1hyun]

정말 고생하셨어요!
정확한 리뷰를 위해서 텍스트 상으로는 힘들 것 같고 다음주 회의 때 해당 PR을 리뷰하는 시간을 가질게요.

일단 일부만 리뷰해두었습니다!

[ch1hyun]

삼항 연산자를 통해 직관적으로 파악할 수 있네요! 좋습니다

[ch1hyun]

Suggested change

	return performanceDto
	.map(performance ->
	ApiResponseUtil.success(SuccessMessage.SUCCESS, RecommendMusicsPerformanceResponse.from(performance)))
	.orElseGet(()-> ApiResponseUtil.success(SuccessMessage.SUCCESS, Collections.emptyMap()));
	return ApiResponseUtil.success(SuccessMessage.SUCCESS,
	performanceDto.map(RecommendMusicsPerformanceResponse::from)
	.orElseGet(Collections.emptyMap())
	);

이렇게 작성하는건 어떠신가요?
중복된 코드를 줄일 수 있을 것 같아요!

[ch1hyun]

무작위 아티스트 아이디를 추출할 때 애플리케이션 레벨, 데이터베이스 레벨 각각 어떤 장단점이 있었고 애플리케이션 레벨 구현을 선택하신 이유가 궁금합니다!

[ch1hyun]

추가로 아티스트 아이디를 가져올 때 서비스에 요청하지 않고 트랜잭션 외부에서 공연 엔티티에서 추출하신 이유가 궁금합니다!

[ch1hyun]

복수형 (-s, -List) 중 하나만 작성하시면 일관성 측면에서 더 좋을 것 같아요!

[ch1hyun]

round의 limit 값이랑 음악 조회 개수의 값이 같은 상수를 사용하는건가요?
따로 작성하지 않으신 이유가 궁금합니다.

제 의견은 매직 넘버를 상수화하신 것이라면, 이름을 따로 두어야 한다고 생각해요.
매직 넘버를 상수화하는 것은 이름으로 해당 상수 값이 어떤걸 의미하는지 직관적으로 나타낼 때 사용하기 때문이에요.

[junggyo1020]

동의합니다!!! MAX_FETCH_ATTEMPTS 같은 단어로 바꾸는 것도 좋을 것 같네요 ㅎㅎ

[junggyo1020]

변경사항이 많아 힘드셨을텐데 고생하셨습니다!!

[junggyo1020]

orElse와 orElseGet의 차이를 인지하고 사용하신 부분이 좋네요 :)

[junggyo1020]

별도의 메서드로 분리하신 부분 좋은 것 같아요 :)

[junggyo1020]

동의합니다!!! MAX_FETCH_ATTEMPTS 같은 단어로 바꾸는 것도 좋을 것 같네요 ㅎㅎ

[junggyo1020]

기존에 List.sort()를 사용하고 있었군요! JPA 쿼리메서드로 변경하면서 확실히 처리속도가 개선될 것같네요 👍

[junggyo1020]

기존에는 제외할 ID를 고려하여 넉넉하게 5배수를 가져온 후 필터링했던 것으로 보여요! 이 로직을 변경한 특별한 이유가 있으신가요? 예를 들어, getTopSongsByArtistId 내부의 캐싱 전략이 변경되었거나, API 응답 속도에 문제가 있었을까요? 이 변경으로 인해 필터링 후 최종적으로 반환되는 음악의 개수가 limit보다 적어질 가능성이 커졌는데, 의도된 동작인지 궁금합니다!