[BE] 약속 추천 기능 리팩토링 #292

hw0603 · 2024-08-22T07:19:18Z

작업 내용

'약속 추천'을 새롭게 정의했습니다.
추천 로직의 수행 시간을 단축했습니다.
- M1 Pro 로컬 IDLE 상태에서 15명 추천 연산 기준 220s → 0.5s 미만
추상화 할 수 있는 로직들을 추상화하여 새로운 구현체를 쉽게 만들 수 있는 구조로 변경했습니다.

기존 약속 추천 로직의 개선 필요성

참가자가 N명인 약속에 대해, 기존에 구현된 약속 추천 로직은 다음과 같이 동작했습니다.

N명의 참여자 정보를 DB에서 로드한 뒤 필터링 기준으로 전달된 참여자 M(<=N)명에 대한 정보만을 골라 냄
M 명의 참여자들의 모든 조합에 대해 조합 별 추천 로직 수행
조합 별 추천 로직 결과를 정렬 기준에 맞게 각각 정렬하고 반환

‘조합 별 추천 로직’은 아래와 같이 동작합니다.

전달된 조합(subGroup)에 포함된 참여자들의 모든 일정 정보를 DB에서 조회
스트림으로 (날짜-시간슬롯) 별 참여가 가능한 참여자들의 Map 구축
구축한 Map을 시간 오름차순으로 순회하며 subGroup 내의 모든 참여자가 참여할 수 있는 (날짜, 시간슬롯) 구함
연속된 시간슬롯들을 병합

해당 로직은 참여자가 많아질수록 수행 시간이 기하급수적으로 늘어난다는 문제가 있었습니다.

조합 별 추천 로직 이 한 번 실행될 때 마다, 쿼리가 하나씩 실행되는 구조였기 때문인데요, 해당 쿼리 자체도 IN 연산자를 사용하기 때문에 무거운 쿼리인데다가 그 쿼리가 조합의 총 개수인 2^N 번 수행되는 구조였습니다.

실제로 첨부한 사진의 로그를 보면, 11명에 대한 추천 일정 응답에 52분(…)이 소요된 것을 확인할 수 있습니다.

(IDLE 상태에서 이 정도가 걸린 것은 아니고, 다수의 참여자에 대한 반복적인 요청 때문에 큐잉 딜레이가 생겼던 것으로 보입니다. 로컬 IDLE 상태에서 실행 시 10명 계산 시 약 40초, 15명 계산 시 약 220초가 필요했습니다.)

제가 생각했던 개선 포인트들은 다음과 같습니다.

참여자 조합 생성 로직을 O(N*(2^N)) 에서 O(nCr) 로 개선
DB에서는 데이터를 읽어오기만 하고, 모든 집계를 Stream으로 처리하던 부분을 집계 쿼리(group by 등)를 활용하여 개선
2^N 번의 조합 별 추천 로직 수행에서, 중복으로 계산되는 부분을 캐싱해 두었다가 재사용

첫 번째와 두 번째를 개선했음에도 수행 시간은 크게 줄어들지 않았습니다. (사실 세 번째가 가장 큰 병목임을 알고 있긴 했어요🥲) ref

기존 구조에서 캐싱을 추가하기에는 구현 코드의 복잡도가 크게 증가하는 것에 비해, 캐싱을 제대로 적용한다고 해도 이론상 계산해야 하는 최소 연산 횟수 자체가 크기 때문에 근본적인 해결책이 될 수 없다고 생각하여 실제로 구현해 보지는 않았어요. (서버가 뻗는 N 값의 임계치를 조금 높일 수 있을 뿐이라 생각했습니다.)

새로운 추천 기준의 정의

위와 같은 이유로, 추천 시간 이라는 것을 새롭게, 그리고 명확하게 정의하고 사용자에게는 상위 몇 개의 일정만 노출하는 편이 낫겠다는 생각을 하게 됐습니다.

최대 2^N 개의 추천일정들이 계산될 텐데, 그걸 다 사용자들에게 보여주는 것은 사실 의미가 없겠죠?

따라서 이번 스프린트에서 구현할 추천 일정을 다음과 같이 정의했습니다.

위에서 언급했던 것과 같이 참가자가 N 명인 약속에서 M(<=N) 명에 대한 추천을 요청했을 경우에,

N == M 인 경우. 즉, 전체 참여자에 대한 조회인 경우
- N명이 참여할 수 있는 시간대, N-1명이 참여할 수 있는 일정, … 을 구하여 상위 10개를 응답합니다.
N > M 인 경우. 즉, 일부 참여자에 대한 조회인 경우
- M 명이 모두 참여할 수 있는 연속된 일정 중 최대 상위 5개를 응답합니다.

이때, N==M 인 경우에 중복된 일정을 다시 추천하지는 않습니다.

따라서 다음과 같은 일정 정보가 있고, 빠른 시간 순으로 추천 요청이 오면

10시: A, B, C, D
11시: A, B, C
13시: A, B, C, D
14시: A, B, C, D

다음과 같이 응답합니다.

[(10~10시, ABCD), (13~14시, ABCD), (11~11시, ABC)]

즉, (A, B, C) 는 모두 10시~11시에 참여할 수 있지만 (10시 ~ 11시, ABC) 를 별도로 추천해주지 않습니다. 해당 정보가 궁금했다면 사용자는 ABC를 필터링 조건으로 걸고 조회할 것이라 판단했습니다.

코드 구조

커스텀 쿼리의 결과를 반환받기 위한 레코드 DateAndTimeslot 을 추가했습니다.

기존에 존재하던 TimeslotInterval과는 별개로, 날짜와 함께 구간을 관리해야 할 소요가 생겨 DateTimeInterval을 추가했습니다.

일정 추천은 다음과 같이 동작합니다.

Timeslot 단위로 분리되어 있으며, 시간 오름차순 정렬된 참가자의 일정을 모두 DB 조회
연속적으로 이어 붙일 수 있는 구간들을 병합
추천 기준에 맞게 정렬
상위 N개를 뽑아서 반환

// 추상 클래스 ScheduleRecommender의 메서드

public List<CandidateSchedule> recommend(AttendeeGroup group, String recommendType) {
    List<CandidateSchedule> mergedCandidateSchedules = calcCandidateSchedules(group);
    sortSchedules(mergedCandidateSchedules, recommendType);
    return mergedCandidateSchedules.stream()
            .limit(getMaxRecommendCount())
            .toList();
}

private List<CandidateSchedule> calcCandidateSchedules(AttendeeGroup group) {
    List<CandidateSchedule> intersectedDateTimes = extractProperSortedDiscreteScheduleOf(group);
    return CandidateSchedule.mergeContinuous(intersectedDateTimes, this::isContinuous);
}

abstract List<CandidateSchedule> extractProperSortedDiscreteScheduleOf(AttendeeGroup group);

abstract boolean isContinuous(CandidateSchedule current, CandidateSchedule next);

private void sortSchedules(List<CandidateSchedule> mergedCandidateSchedules, String recommendType) {
    RecommendedScheduleSortStandard sortStandard = RecommendedScheduleSortStandard.from(recommendType);
    CandidateScheduleSorter sorter = sortStandard.getSorter();
    sorter.sort(mergedCandidateSchedules);
}

abstract long getMaxRecommendCount();

ScheduleRecommender를 상속받은 구현체들은 모두 recommend() 라는 하나의 public 메서드를 가지고, 추상 클래스 내에서 위에서 언급한 흐름을 정의해 두었습니다.

구현체별로

DB를 조회하는 상세 구현
각 시간대의 연속성을 판단하는 기준
응답할 상위 N개의 일정 개수

가 달라져야 하므로, 추상 메서드로 정의하여 구현을 미뤄 두었습니다.

예를 들어, 전체 참여자에 대한 추천 일정을 계산하는 TotalScheduleRecommender 에서는 물리적으로 연속된 시간이라도 해당 시간대에 참여할 수 있는 AttendeeGroup의 참여자 구성이 다르다면 병합시킬 수 없습니다. 반면 FilteredScheduleRecommender에서는 필터로 전달된 참여자들에 대해서만 연산하므로 시간대가 연속이라면 항상 병합시킬 수 있습니다. 이에 따라 isContinuous() 의 구현이 달라져야 합니다.

현재 ScheduleRecommender 타입의 Bean 후보가 유일하지 않으므로, 일반적인 형태로는 오토와이어링 받을 수 없습니다. ScheduleRecommenderFactory 내부에 Bean 후보들을 모두 담고 있는 Map을 구축하고, 사용 위치에서 팩토리에 빈 반환을 요청하면 팩토리에서 연산을 수행하여 문맥별로 적절한 빈을 반환하도록 구현해 두었습니다.

~~Sorter 들에도 유사한 구조가 적용됩니다.~~ (현재는 팩토리 패턴을 제거하고 Enum에서 일괄적으로 관리하는 것으로 변경됨)

특이 사항

⭐️ 추가로 개선/구현해야 하는 부분

AttendeeGroup을 필터링 할 때 전체 탐색을 진행하고 있는데, AttendeeGroup도 AvailableDates 와 같이 내부 자료구조를 Set으로 사용하면 좋을 것 같습니다.
현재 ScheduleRecommenderTest의 일부 메서드에 @Transactional 어노테이션이 붙어 있습니다. 테스트 코드에서 @Transactional 사용 시 고려해야 할 사항이 많음은 알고 있지만, TotalScheduleRecommender를 트랜잭션 없이 테스트할 수 있는 방법을 아직 찾지 못했습니다.
FilteredScheduleRecommender 를 테스트 할 때는 어노테이션 없이도 잘 동작합니다.
이는 해당 추천기의 내부 구현이 @Query 를 통해 JPQL로 데이터를 가져오고 있는데, 현재 지연 로딩으로 데이터를 가져오지 않고 있기 때문으로 보입니다.
필터링 조건으로 전달된 참여자들을 ‘무조건 포함’하면서, 나머지 참여자들이 많이 참여할 수 있는 기준으로 추천하는 기능도 필요해 보입니다.
‘길게 만나도 싶어요’ 의 추천 로직을 조금 변형하여, ‘최소 T 시간은 만나야 해요’ 와 같은 최소시간 보장 추천 로직도 구현이 필요합니다.
‘전체 참여자에 대한 추천’ 은 자주 조회될 가능성이 높으므로, 매번 계산하기보다는 참여자들의 일정에 변동사항이 생길 때 미리 한 번 계산해 두고, 추천일정 조회 시 계산된(DB에 저장된) 데이터를 단순 조회하여 반환하는 구조로 개선해도 좋을 것 같아요.

도움이 필요한 부분

현재 TotalScheduleRecommenderTest 수행 시 @Transactional 이 없으면 LazyInitializationException 이 발생합니다.
findAllByAttendeeIn() 에서 Schedule을 가져올 때 프록시 객체로 가져오는데, 테스트 코드의 단언문이 트랜잭션 밖에 있으므로 attendee를 조회할 수 없기 때문입니다.

이를 적절히 테스트 하기 위해 테스트 코드를 어떻게 작성해야 할지 다른 분들의 조언이 필요합니다.

프로덕션 코드에서는 ScheduleService.recommendSchedules() 에 붙은 @Transactional 여부에 관계없이 Schedule 내의 Attendee를 지연 로딩하지 않고 가져오는 반면, 테스트 코드에서는 @Transactional이 붙은 경우에만 지연 로딩하지 않고 가져오는 것 같습니다. 어떻게 현재처럼 동작하는 것인지 이해가 잘 되지 않네요😅

다 적고 나서 깨달았습니다…. 참여자 조회는 Service에서 하는데, 지금 테스트하는 것은 Service 내에서 참여자 조회 후 호출하는 추천기 로직이니.. 캐시 히트가 발생하지 못해서 프록시 객체로 남아 있는게 당연하겠네요ㅎㅎ; 테스트 코드에서 @Transactional 명시 시 실제 객체를 가져올 수 있었던 이유도 @BeforeEach 로 정의된 setUp() 에서 테스트 데이터를 삽입하는 로직과 같은 트랜잭션으로 묶여 캐시에서 가져올 수 있었던 거구요. 캐시에서 가져온 ‘정확히 동일한’ 객체를 단순 비교하고 있어서 테스트가 통과하는데, em.clear() 이후에는 값이 같은 새로운 객체가 만들어지므로 테스트에 실패할 것 같아요. 이 테스트는 확실히 다시 짜야 하겠어요

리뷰 요구사항 (선택)

github-actions · 2024-08-22T07:20:43Z

Test Results

137 tests 137 ✅ 9s ⏱️
30 suites 0 💤
30 files 0 ❌

Results for commit 683aba7.

♻️ This comment has been updated with latest results.

ehBeak

페드로 추천 기능 리팩토링하시느라 고생 많으셨습니다🥹🥹
깊은 고민이 여기까지 느껴지네요..하하..

아까 직접 말했던 것처럼, RecommendedScheduleSortStandard에서 type과 CandidateScheduleSorter를 연관 지어서 정의할 수 있을 것 같아요. 괜찮다면 이후 반영해 보면 좋을 것 같습니다~
그 외에는 코멘트 정도만 남겼습니다. 확인 부탁드려요~~

아직 ScheduleRecommender 부분을 리뷰하지 못했습니다. 집 도착하면 바로 이어서 할게요~ (이미 리뷰한 건 집 가기 전에 미리 올리는 게 좋을 것 같아서 먼저 보내요!)

ehBeak · 2024-08-22T12:13:56Z

backend/src/main/java/kr/momo/domain/schedule/ScheduleRepository.java

+    @Query("""
+                SELECT
+                    new kr.momo.domain.schedule.DateAndTimeslot(ad.date, s.timeslot)
+                FROM Schedule s
+                JOIN s.availableDate ad
+                WHERE s.attendee IN :essentialAttendees
+                GROUP BY ad.date, s.timeslot
+                HAVING COUNT(s.attendee.id) = :groupSize
+                ORDER BY ad.date ASC, s.timeslot ASC
+            """)
+    List<DateAndTimeslot> findAllDateAndTimeslotByEssentialAttendees(
+            @Param("essentialAttendees") List<Attendee> essentialAttendees, @Param("groupSize") int groupSize
+    );


Suggested change

@Query("""

SELECT

new kr.momo.domain.schedule.DateAndTimeslot(ad.date, s.timeslot)

FROM Schedule s

JOIN s.availableDate ad

WHERE s.attendee IN :essentialAttendees

GROUP BY ad.date, s.timeslot

HAVING COUNT(s.attendee.id) = :groupSize

ORDER BY ad.date ASC, s.timeslot ASC

""")

List<DateAndTimeslot> findAllDateAndTimeslotByEssentialAttendees(

@Param("essentialAttendees") List<Attendee> essentialAttendees, @Param("groupSize") int groupSize

);

@Query("""

SELECT

new kr.momo.domain.schedule.DateAndTimeslot(ad.date, s.timeslot)

FROM Schedule s

JOIN s.availableDate ad

WHERE s.attendee IN :essentialAttendees

GROUP BY ad.date, s.timeslot

HAVING COUNT(s.attendee.id) = :#{#essentialAttendees.size()}

ORDER BY ad.date ASC, s.timeslot ASC

""")

List<DateAndTimeslot> findAllDateAndTimeslotByEssentialAttendees(

@Param("essentialAttendees") List<Attendee> essentialAttendees

);

groupSize가 해당 메서드의 파라미터 중 하나인, essentialAttendees의 size()인데요.
groupSize는 essentialAttendees매개변수로도 알 수 있습니다. 때문에 groupSize라는 파라미터를 빼고 SpEL표현식을 사용하여 essentialAttendees 리스트의 크기를 동적으로 쿼리 내에서 계산할 수 있습니다😎

안 그래도 어떻게 크기를 받아올 방법이 없나 계속 찾아봤었는데 SpEL을 여기서도 쓸 수 있군요ㅋㅋㅋ 너무 좋습니다👍

ehBeak · 2024-08-22T12:56:07Z

backend/src/main/java/kr/momo/service/schedule/recommend/TotalScheduleRecommender.java

+    }
+
+    @Override
+    List<CandidateSchedule> extractProperSortedDiscreteScheduleOf(AttendeeGroup group) {


protected 접근 제어자를 명시하지 않은 이유가 있을까요?

IDE에서 단축키로 메서드 구현 기능을 사용했더니 누락됐네요😅 추가했습니다!

ehBeak · 2024-08-22T12:58:37Z

backend/src/main/java/kr/momo/service/schedule/recommend/TotalScheduleRecommender.java

+                .collect(Collectors.groupingBy(Schedule::dateTimeInterval, Collectors.mapping(
+                                Schedule::getAttendee,
+                                Collectors.collectingAndThen(Collectors.toList(), AttendeeGroup::new)
+                        ))
+                );


Suggested change

.collect(Collectors.groupingBy(Schedule::dateTimeInterval, Collectors.mapping(

Schedule::getAttendee,

Collectors.collectingAndThen(Collectors.toList(), AttendeeGroup::new)

))

);

.collect(groupingBy(Schedule::dateTimeInterval, mapping(

Schedule::getAttendee,

collectingAndThen(toList(), AttendeeGroup::new)

))

);

Collectors클래스를 static import로 두면 코드가 더 깔끔해질 것 같아요👍

초기 팀 컨벤션을 정할 때 static import는 테스트 메서드에서만 사용하기로 했던 걸로 알고 있어요!

Collectors는 예외로 두기로 했어요..ㅎㅎ

ehBeak · 2024-08-22T13:10:47Z

backend/src/main/java/kr/momo/domain/schedule/recommend/CandidateSchedule.java

+
+    public static List<CandidateSchedule> mergeContinuous(
+            List<CandidateSchedule> sortedSchedules,
+            BiPredicate<CandidateSchedule, CandidateSchedule> isContinuous


CandidateSchedule이 연속되는지 확인하는 로직을 넘겨 받는데, 넘겨 받는 로직을 FilteredRecommender에 isContinuous()라는 함수로 따로 둔 이유가 있을까요?
제가 느끼기엔, 각 CandidateSchedule이 연속되는 것을 확인하는 것이니까 CandidateSchedule 도메인 내부에 static 함수로 있는 것이 더 자연스럽게 느껴지는데요..! 어떤 의도로 분리하셨는지 궁금합니다 :)

안 그래도 구현하면서 조금 설명이 필요한 부분들이 있을 것 같아 PR 본문을 상세하게 써야겠다고 생각했었는데, 어쩌다 보니 시간 관계상 본문을 제대로 작성하기 전에 리뷰 요청을 드리게 됐네요😅

아까 대면으로 이야기했던 것 처럼 현재 추상 클래스 ScheduleRecommender를 상속받는 구현체가 총 두 가지 존재합니다. 이들은 각각 일정의 '연속성'을 판단하는 기준이 조금 다른데요, 때문에 추상 메서드로 정의해 둔 뒤 구현체에게 상세 로직을 위임해 두었습니다.

좀 더 자세히 설명드리자면,

TotalScheduleRecommender

하나의 약속 내에 존재하는 모든 참여자들을 대상으로 추천 연산을 수행합니다. N명의 참가자가 있을 때, 참여자의 부분집합(N-1명, N-2`명...) 내의 참여자들이 참여 가능한 일정을 반환합니다.

FilteredScheduleRecommender

필터링된 일정 추천에 사용됩니다. 전체 참여자 중, 전달된 참여자들이 모두 참여할 수 있는 일정을 반환합니다.

따라서, 전체 참여자에 대한 추천 연산과는 달리 연속성 판별 시 AttendeeGroup의 동일성을 추가적으로 검사해 주어야 합니다.

현재는 두 구현체가 존재하지만 이후 '필터링 된 참여자들을 꼭 포함하되, 나머지 참여자들이 많이 참여할 수 있는 순'으로 추천하는 기능도 추가될 것으로 보입니다. 해당 기능 구현 시 정렬이나 연속성 판별 기준이 또 달라질 수 있을 것 같아 공통 부분만 추상화해 두고 나머지 부분들은 구현을 하위 객체로 미뤄 두었습니다🙂

ehBeak · 2024-08-22T13:13:18Z

backend/src/main/java/kr/momo/domain/schedule/recommend/CandidateScheduleSorter.java

+public interface CandidateScheduleSorter {
+
+    void sort(List<CandidateSchedule> unorderedSchedules);
+}


해당 인터페이스는 함수가 더 추가될 일이 있을까요?
사소하지만.. 없다면, @FunctionalInterface을 추가하여 함수형 인터페이스임을 명시적으로 표시하면 좋을 것 같습니다.

Suggested change

public interface CandidateScheduleSorter {

void sort(List<CandidateSchedule> unorderedSchedules);

}

@FunctionalInterface

public interface CandidateScheduleSorter {

void sort(List<CandidateSchedule> unorderedSchedules);

}

추가했습니다!

ehBeak · 2024-08-22T13:27:04Z

backend/src/main/java/kr/momo/service/schedule/dto/RecommendedScheduleResponse.java


 public record RecommendedScheduleResponse(
+        int rank,


rank라는 네이밍이 뭔가 순위 같은 느낌이라, order 정도가 어떨까요? 싫음 말고요ㅎ

ㅋㅋㅋㅋㅋ 순위를 표현하기 위해 프론트랑 협의 후 rank로 사용하였습니다.
'추천 순위'라고도 볼 수 있지 않을까요?

ikjo39

안녕하세요 페드로!
추천 로직 대단하네요. 엄청 개선된게 보여요.
추상화와 객체 분리로 적절히 되어 보입니다!

배키의 리뷰를 반영한다면 크게 수정할 부분은 없을 것 같아요.
몇 가지 궁금점과 의견만 남겨 두었으니 확인해주시면 감사하겠습니다.
import문 정리도 한번 해주시길 부탁드리겠습니다 😁

ikjo39 · 2024-08-22T15:00:24Z

backend/src/main/java/kr/momo/domain/timeslot/Timeslot.java

@@ -58,7 +58,7 @@ public enum Timeslot {
    TIME_2300(LocalTime.of(23, 0)),
    TIME_2330(LocalTime.of(23, 30));

-    private static final long DURATION_IN_MINUTE;
+    public static final long DURATION_IN_MINUTE;


사소하지만 상수랑 필드 공백 한칸 추가가 빠져있네요!!

ikjo39 · 2024-08-22T15:09:10Z

backend/src/main/java/kr/momo/service/schedule/recommend/TotalScheduleRecommender.java

+        return groupedAttendeesByDateTimeInterval.entrySet().stream()
+                .map(entry -> new CandidateSchedule(entry.getKey(), entry.getValue()))
+                .sorted(Comparator.comparing(CandidateSchedule::startDateTime))
+                .toList();
+    }


keySet()과 entrySet()에 대해 궁금해여 찾아보니 map에 모든 key로 value를 연산해야 하는 경우 entrySet()이 훨씬 성능이 좋다고 하네요!
리뷰하다 알게된 점 남기고 갑니다!
참고자료 - stackoverflow

entrySet() 자주 애용해 주세요~

ikjo39 · 2024-08-22T15:15:22Z

backend/src/main/java/kr/momo/service/schedule/recommend/TotalScheduleRecommender.java

+    @Override
+    List<CandidateSchedule> extractProperSortedDiscreteScheduleOf(AttendeeGroup group) {
+        return findAllScheduleAvailableByEachAttendee(group);
+    }


이 부분의 경우 메서드를 처리 없이 바로 반환하는데 메서드를 분리하신 이유가 궁금해요!

추상 메서드명은 extractProperSortedDiscreteScheduleOf 로 작명했는데요, 말 그대로 하위 클래스에서 알아서 자신의 책임에 맞게 '적절한' 일정의 리스트를 구해서 반환해야 합니다.

위 코멘트에서 언급했던 것과 같이, 현재 구현체별로 추천하는 로직이 조금 달라요. 다시 말해서 구현체별로 '적절한' 일정이라고 판단하는 기준이 다른 건데요, 그래서 단순히 추상 메서드의 이름을 그대로 사용하고 @Override 한 메서드에 모든 구현을 놓으면 코드의 의미를 따라가기 힘들 것이라 생각했습니다.

따라서 각 구현체별로 서로 다른 이름의 메서드를 별도로 정의하고, 인자를 포워딩 시켜 주는 방식을 택했습니다. (주석으로 구현을 적어 두는 방식보다는 낫다고 생각했어요🤔)

저도 좀 고민이 됐던 부분인데, 다른 분들의 의견도 궁금합니다!
@ehBeak @seokmyungham @seunghye218

저는 괜찮은 방식이라고 생각하고, 하위 메서드 분리만 조금 더 잘하면 좋을 것 같아요.

개인적으로 extractProperSortedDiscreteScheduleOf 이름이 너무 어려워서 더 헷갈렸는데요 ㅋㅋ
로직이 복잡한 만큼 시그니처는 짧고 직관적으로 가는게 좋을 것 같아요.
getSortedDiscreteSchedule() 정도면 충분하지 않을까 싶습니다.

하위에 findAllScheduleAvailableByEachAttendee도 마찬가지로 findSchedulesForEachAttendee 면 좋을 것 같고.. 개인적인 바람입니다 ㅋㅋ

ikjo39 · 2024-08-22T16:04:19Z

backend/src/main/java/kr/momo/domain/schedule/recommend/CandidateScheduleSorterFactory.java

+    public CandidateScheduleSorter getSorterOf(RecommendedScheduleSortStandard standard) {
+        if (standard == RecommendedScheduleSortStandard.EARLIEST_ORDER) {
+            return candidates.get(fetchBeanName(EarliestFirstSorter.class));
+        }
+        if (standard == RecommendedScheduleSortStandard.LONG_TERM_ORDER) {
+            return candidates.get(fetchBeanName(LongTermFirstSorter.class));
+        }
+        throw new IllegalArgumentException("잘못된 정렬 기준입니다.");
+    }


현재는 RecommendedScheduleSortStandard 1개에 CandidateScheduleSorter 가 일대일 대응으로 존재하는 것 같아요!
또한 1개의 CandidateScheduleSorter가 추가될 때마다 if문도 계속 추가될 것이 우려되네요!

RecommendedScheduleSortStandard Enum의 필드로 CandidateScheduleSorter를 추가해보는건 어떨까요?

배키도 비슷한 제안을 해 줬었는데 다온도 같은 생각이시네요!

Recommender 팩토리를 구현하고 난 뒤라 그런지 자연스럽게 팩토리로 만들었던 것 같은데, 이미 정렬 기준에 대한 enum이 존재하는 상황이니 한 클래스에서 같이 관리해 주는게 깔끔할 것 같아요.

예외 처리도 한 번에 할 수 있겠네요. 반영하겠습니다😎

ehBeak

페드로 추가 코멘트 남깁니다!
고생 많으셨어요 :) 💯💯💯

ehBeak · 2024-08-22T15:56:38Z

backend/src/main/java/kr/momo/service/schedule/recommend/FilteredScheduleRecommender.java

+    }
+
+    @Override
+    long getMaxRecommendCount() {


이 부분도 protected 접근 지정자를 따로 두지 않은 이유가 궁금합니다!

추가했습니다ㅎㅎ

ehBeak · 2024-08-22T16:33:16Z

backend/src/main/java/kr/momo/service/schedule/recommend/ScheduleRecommenderFactory.java

+    public ScheduleRecommender getRecommenderOf(AttendeeGroup attendeeGroup, AttendeeGroup filteredGroup) {
+        if (filteredGroup.containsAll(attendeeGroup)) {
+            return candidates.get(fetchBeanName(TotalScheduleRecommender.class));
+        }
+        return candidates.get(fetchBeanName(FilteredScheduleRecommender.class));
+    }
+
+    private String fetchBeanName(Class<?> clazz) {
+        char[] className = clazz.getSimpleName().toCharArray();
+        className[0] = Character.toLowerCase(className[0]);
+        return new String(className);
+    }


로직을 보니, ScheduleRecommender를 상속한 2개의 빈을 map(candidates)에 담아두고 이후에 필요할 때마다 클래스 이름으로 조회하여 반환하는 것 같아요. 조금 더 좋은 방법이 없을까 고민해 봤는데, 딱히 좋은 방안이 떠오르지는 않네요.

클래스명을 가져와서 조회하는 것이 좀 그렇다면, 아래와 같이 ApplicationContext를 사용하는 법도 있을 것 같아요!
(그런데 ApplicationContext를 이렇게 써도 되는 건지는 모르겠네요..😅)

@Component @RequiredArgsConstructor public class ScheduleRecommenderFactory { private final ApplicationContext applicationContext; public ScheduleRecommender getRecommenderOf(AttendeeGroup attendeeGroup, AttendeeGroup filteredGroup) { if (filteredGroup.containsAll(attendeeGroup)) { return applicationContext.getBean(TotalScheduleRecommender.class); } return applicationContext.getBean(FilteredScheduleRecommender.class); } }

보통 Map 이나 List를 활용하여 초기 부트 시에 모든 빈들을 다 받아 두고 사용하는 현재와 같은 방식과, 제안해 주신 방법처럼 ApplicationContext에서 가져오는 방법 두 가지를 모두 사용하는 것 같아요.

.getBean()은 동적으로 빈을 조회할 때 주로 사용하고, (스프링 내장 캐싱이 있긴 하지만) 스프링 컨테이너 내에서 빈을 매번 찾아오는 방식이라 성능 면에서 조금 느릴 수 있다고 해요.

반면 빈 후보를 오토와이어링 받아 두고 사용하는 방식은 초기 로딩 시 약간의 오버헤드가 발생하지만, HashMap 특성 상 이후 조회는 매우 빠르게 수행할 수 있습니다.

현재의 팩토리는 고정된 타입의 빈들에 대한 반복적 접근이 필요한 경우로 볼 수 있으므로, 현재 구현을 유지하는 게 좋을 것 같네요🙂 이후 변경이 필요해진다면 ApplicationContext를 사용하는 방법도 고려해 보겠습니다!

seokmyungham

페드로 고생하셨습니다 ㅋㅋㅋㅋ 🔥🔥
놀랍네요 😮

…추천하는 것으로 변경 - 모든 참여자가 참가 가능한 스케줄을 JPQL을 활용해 한 번에 가져오도록 변경

…구하는 메서드명 변경 - findAllDateTimeAvailableByEveryAttendee()

…erval 정보를 활용하도록 변경 - AttendeeGroup 정보를 `findAllDateTimeAvailableByEveryAttendee()` 에서 설정하도록 변경

…수행하도록 변경

…ule에서 수행하도록 변경

- Enum에서 Comparator 구현 분리

…절성 테스트 추가

- 참여자 수 오름차순 -> 내림차순

- 마지막 타임슬롯에 해당하는 DateAndTimeslotTest의 변환 시 endDate가 다음 날짜로 지정되는지 테스트

… 관리하도록 변경

hw0603 · 2024-08-22T21:11:52Z

cc. @ehBeak @seunghye218

hw0603 added 🐈‍⬛ 백엔드 백엔드 관련 이슈에요 :) 🚀 기능 기능을 개발해요 :) 🛠️ 픽스 버그를 해결했어요 :) ♻️ 리팩터링 코드를 깎아요 :) 🏡 집 가고 싶음 솔직히 집에 가고 싶어요 :( labels Aug 22, 2024

hw0603 added this to the 4차 데모데이 milestone Aug 22, 2024

hw0603 self-assigned this Aug 22, 2024

hw0603 marked this pull request as ready for review August 22, 2024 09:56

hw0603 requested review from ehBeak, ikjo39, seokmyungham and seunghye218 as code owners August 22, 2024 09:56

ehBeak reviewed Aug 22, 2024

View reviewed changes

ikjo39 approved these changes Aug 22, 2024

View reviewed changes

ehBeak reviewed Aug 22, 2024

View reviewed changes

seokmyungham approved these changes Aug 22, 2024

View reviewed changes

hw0603 added 12 commits August 23, 2024 05:53

refactor(ScheduleService): 필터링된 일정 추천 시 필터에 포함된 사람들이 모두 참여할 수 있는 일정을 …

f746219

…추천하는 것으로 변경 - 모든 참여자가 참가 가능한 스케줄을 JPQL을 활용해 한 번에 가져오도록 변경

refactor(AttendeeGroup): 더 이상 사용되지 않는 조합 관련 로직과 테스트 제거

2dd1916

refactor(RecommendedScheduleGenerator): 일정 추천 로직 담당 클래스 분리 후 추상화

4d3e4bb

refactor(TotalRecommendedScheduleGenerator): 전체 참여자에 대한 추천 일정 계산 로직 분리

3aa8da0

refactor(FilteredRecommendedScheduleGenerator): 전달된 모든 참여자가 가능한 시간대를 …

3fe7c3c

…구하는 메서드명 변경 - findAllDateTimeAvailableByEveryAttendee()

refactor(FilteredRecommendedScheduleGenerator): 내부 연산에서 시점 정보가 아닌 Int…

59460ff

…erval 정보를 활용하도록 변경 - AttendeeGroup 정보를 `findAllDateTimeAvailableByEveryAttendee()` 에서 설정하도록 변경

refactor(FilteredRecommendedScheduleGenerator): 연속적인 시간을 병합하는 로직을 직접 …

a0720c5

…수행하도록 변경

refactor(RecommendedScheduleGenerator): 각 구현체를 유사한 구조로 변경

62a41f4

refactor(CandidateSchedule): 추천 스케줄 생성기의 연속된 일정 병합 로직을 CandidateSched…

50f70cb

…ule에서 수행하도록 변경

refactor(RecommendedScheduleSortStandard): 추천 일정의 정렬 기준을 정의하는 enum 이름 변경

e60579b

- Enum에서 Comparator 구현 분리

feat(RecommendedScheduleSorter): 추천 일정 정렬을 담당하는 객체 구현

eda7595

refactor(RecommendedScheduleGenerator): 정렬기가 추천 일정을 정렬하도록 위임

1fb679b

hw0603 added 25 commits August 23, 2024 06:00

feat(ScheduleService): 추천 일정 응답 시 rank 정보를 함께 응답하는 기능 구현

f336151

test(AttendeeGroupTest): 그룹 간의 포함 관계 확인 메서드 테스트 추가

b6ebc6c

refactor(ScheduleRecommender): 일정 연속성 확인 메서드에서 불연속성이 아닌 연속성을 확인하도록 변경

9e2d8ab

refactor(CandidateSchedule): 연속 일정 병합 메서드명 변경

20ca7b2

test(CandidateScheduleTest): 연속된 추천 일정 병합 테스트 추가

1150fcf

test(CandidateScheduleSorterFactoryTest): Sorter Factory가 반환한 Bean의 적…

49b4597

…절성 테스트 추가

refactor(ScheduleRecommenderFactory): 테스트 메서드명 변경

ad6a2c4

test(EarliestFirstSorterTest): 이른 시간 순 정렬기 테스트 추가

1a364ff

refactor(CandidateScheduleTest): 테스트 픽스쳐를 사용하도록 변경

7f7b46e

fix(EarliestFirstSorterTest): @DisplayName 설명 수정

cb40825

- 참여자 수 오름차순 -> 내림차순

test(LongTermFirstSorterTest): 긴 일정 기간 순 정렬기 테스트 추가

8c8856a

test(DateAndTimeSlotTest): DateTimeInterval로의 변환 테스트 추가

f674ce0

- 마지막 타임슬롯에 해당하는 DateAndTimeslotTest의 변환 시 endDate가 다음 날짜로 지정되는지 테스트

refactor(ScheduleRepository): 사용하지 않는 메서드 삭제

699ac82

test(ScheduleRepositoryTest): 참여자들이 모두 참여할 수 있는 시간대 조회 쿼리 테스트 추가

46a0e7b

style(RecommendedScheduleResponse): 코드 스타일 적용

317cc16

comment(ScheduleRepositoryTest): 잘못 작성된 주석 정보 수정

bc7cdc7

test(ScheduleRecommenderTest): 일정 추천기 별 테스트 코드 추가

7cecfa6

docs: 추천 일정 조회 API 문서화

d3902e2

refactor(ScheduleRepository): SpEL을 활용하여 그룹 크기를 동적으로 계산하도록 변경

9b46b0f

refactor(TotalScheduleRecommender): 누락된 메서드 접근 제어자 추가

8fd344c

refactor(CandidateScheduleSorter): @FunctionalInterface 어노테이션 명시

b56d94a

style(Timeslot): 코드 스타일 적용

4097369

refactor(FilteredScheduleRecommender): 누락된 메서드 접근 제어자 추가

0845562

refactor(RecommendedScheduleSortStandard): SorterFactory를 제거하고 Enum에서…

b6016c5

… 관리하도록 변경

fix(ScheduleRepositoryTest): 병합 충돌 해결 후 잘못 지정된 행 수정

683aba7

hw0603 force-pushed the refactor/190-schedule-recommendation branch from a37d53a to 683aba7 Compare August 22, 2024 21:07

hw0603 merged commit 817b655 into develop Aug 22, 2024
6 checks passed

ehBeak deleted the refactor/190-schedule-recommendation branch August 22, 2024 22:08

hw0603 mentioned this pull request Aug 26, 2024

[BE] 하루의 마지막 타임슬롯이 포함된 일정 등록 시 오류가 발생해요 :( #88

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BE] 약속 추천 기능 리팩토링 #292

[BE] 약속 추천 기능 리팩토링 #292

hw0603 commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

ehBeak left a comment

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024 •

edited

Loading

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ikjo39 left a comment •

edited

Loading

ikjo39 Aug 22, 2024

ikjo39 Aug 22, 2024

hw0603 Aug 22, 2024

ikjo39 Aug 22, 2024

hw0603 Aug 22, 2024

seokmyungham Aug 22, 2024 •

edited

Loading

ikjo39 Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak left a comment

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

ehBeak Aug 22, 2024

hw0603 Aug 22, 2024

seokmyungham left a comment

hw0603 commented Aug 22, 2024

[BE] 약속 추천 기능 리팩토링 #292

[BE] 약속 추천 기능 리팩토링 #292

Conversation

hw0603 commented Aug 22, 2024 • edited Loading

관련 이슈

작업 내용

기존 약속 추천 로직의 개선 필요성

새로운 추천 기준의 정의

코드 구조

특이 사항

⭐️ 추가로 개선/구현해야 하는 부분

도움이 필요한 부분

리뷰 요구사항 (선택)

github-actions bot commented Aug 22, 2024 • edited Loading

Test Results

ehBeak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehBeak Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikjo39 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehBeak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokmyungham left a comment

Choose a reason for hiding this comment

hw0603 commented Aug 22, 2024

hw0603 commented Aug 22, 2024 •

edited

Loading

github-actions bot commented Aug 22, 2024 •

edited

Loading

ehBeak Aug 22, 2024 •

edited

Loading

ikjo39 left a comment •

edited

Loading

seokmyungham Aug 22, 2024 •

edited

Loading