Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,60 @@ public class CourseRankingService {
private static final EnumSet<Category> LIBERAL_CATEGORIES =
EnumSet.complementOf(MAJOR_CATEGORIES);

// ✅ 교양 랭킹에서 제외할 키워드
private static final List<String> LIBERAL_EXCLUDE_KEYWORDS =
List.of("공동체리더십훈련", "채플");

public CourseRankingService(CourseRankingRepository repository) {
this.repository = repository;
}

public RankingResponse getCourseRanking() {
return new RankingResponse(
loadTop10(MAJOR_CATEGORIES),
loadTop10(LIBERAL_CATEGORIES)
loadTop10(MAJOR_CATEGORIES, false),
loadTop10(LIBERAL_CATEGORIES, true)
);
}
Comment on lines 35 to 40

Choose a reason for hiding this comment

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

medium

스타일 가이드(규칙 66)에 따라 읽기 전용(read-only) 트랜잭션에는 @Transactional(readOnly = true)를 명시하는 것이 좋습니다. 이 어노테이션은 JPA에게 성능 최적화를 할 수 있다는 힌트를 주어 데이터베이스 부하를 줄일 수 있습니다. getCourseRanking 메서드는 데이터를 조회만 하므로 이 어노테이션을 추가하는 것을 권장합니다. FQCN(정규화된 클래스 이름)을 사용하면 import 문을 추가하지 않고도 적용할 수 있습니다.

Suggested change
public RankingResponse getCourseRanking() {
return new RankingResponse(
loadTop10(MAJOR_CATEGORIES),
loadTop10(LIBERAL_CATEGORIES)
loadTop10(MAJOR_CATEGORIES, false),
loadTop10(LIBERAL_CATEGORIES, true)
);
}
@org.springframework.transaction.annotation.Transactional(readOnly = true)
public RankingResponse getCourseRanking() {
return new RankingResponse(
loadTop10(MAJOR_CATEGORIES, false),
loadTop10(LIBERAL_CATEGORIES, true)
);
}
References
  1. 읽기 전용 쿼리는 @transactional(readOnly = true) 명시. (link)


private List<RankingItem> loadTop10(Set<Category> categories) {
var rows = repository.findTopCoursesByCategories(
private List<RankingItem> loadTop10(Set<Category> categories, boolean applyLiberalFilter) {
int fetchSize = applyLiberalFilter ? 30 : 10;

Choose a reason for hiding this comment

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

medium

30이라는 숫자가 '매직 넘버'로 사용되었습니다. 이 값이 왜 선택되었는지 코드만으로는 명확하지 않습니다. 가독성과 유지보수성을 높이기 위해 이 값을 LIBERAL_RANKING_FETCH_SIZE와 같은 의미 있는 이름의 상수로 추출하고, 값의 의도를 설명하는 주석을 추가하는 것을 권장합니다. 이렇게 하면 나중에 제외 키워드가 늘어나는 등의 변화에 대응하기 용이해집니다.


var fetchedRows = repository.findTopCoursesByCategories(
categories,
PageRequest.of(0, 10)
PageRequest.of(0, fetchSize)
);

return IntStream.range(0, rows.size())
// ✅ 여기서 최종 리스트를 한 번만 결정
final List<? extends CourseRankingRepository.CourseCountRow> finalRows;

if (applyLiberalFilter) {
finalRows = fetchedRows.stream()
.filter(r -> !containsAny(r.getName(), LIBERAL_EXCLUDE_KEYWORDS))
.limit(10)
.toList();
} else {
finalRows = fetchedRows.stream()
.limit(10)
.toList();
}
Comment on lines +51 to +62

Choose a reason for hiding this comment

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

low

if/else 블록에서 스트림을 생성하고 .limit(10).toList()를 호출하는 부분이 중복됩니다. 코드를 조금 더 간결하게 리팩토링할 수 있습니다. 스트림을 먼저 생성하고, 조건에 따라 필터 연산을 추가한 뒤, 마지막에 limittoList를 한 번만 호출하는 방식이 더 깔끔합니다.

        Stream<? extends CourseRankingRepository.CourseCountRow> stream = fetchedRows.stream();
        if (applyLiberalFilter) {
            stream = stream.filter(r -> !containsAny(r.getName(), LIBERAL_EXCLUDE_KEYWORDS));
        }
        final List<? extends CourseRankingRepository.CourseCountRow> finalRows = stream.limit(10).toList();


// ✅ finalRows는 effectively final → 람다에서 안전
return IntStream.range(0, finalRows.size())
.mapToObj(i -> new RankingItem(
i + 1,
rows.get(i).getName(),
rows.get(i).getTakenCount(),
finalRows.get(i).getName(),
finalRows.get(i).getTakenCount(),
0
))
.toList();
}


private boolean containsAny(String text, List<String> keywords) {
if (text == null) return false;
for (String kw : keywords) {
if (text.contains(kw)) return true;
}
return false;
}
Comment on lines +76 to +82

Choose a reason for hiding this comment

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

low

containsAny 메서드는 Java 스트림 API의 anyMatch를 사용하면 더 간결하고 표현력 있게 작성할 수 있습니다. 가독성 향상에 도움이 됩니다.

    private boolean containsAny(String text, List<String> keywords) {
        if (text == null) {
            return false;
        }
        return keywords.stream().anyMatch(text::contains);
    }

}
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,23 @@ void setUp() {
}

@Test
void getCourseRanking_returnsMajorAndLiberalTop10_withRanksStartingAt1() {
// given: repo가 2번 호출되므로 thenReturn을 2개 준비
var majorRows = List.of(
void getCourseRanking_returnsMajorAndLiberalRankings_withFilteringAndRanksStartingAt1() {
// given: repo가 2번 호출되므로 thenReturn을 2개 준비(전공 -> 교양)
List<CourseRankingRepository.CourseCountRow> majorRows = List.of(
row("OS", 12),
row("DB", 10),
row("DS", 7)
);

var liberalRows = List.of(
// 교양: "채플", "공동체리더십훈련" 포함 과목은 제외되어야 함
List<CourseRankingRepository.CourseCountRow> liberalRows = List.of(
row("채플", 30),
row("실용영어", 20)
row("공동체리더십훈련(1)", 25),
row("실용영어", 20),
row("철학의이해", 18),
row("채플 실습", 17), // "채플" 포함 → 제외
row("공동체리더십훈련", 16), // 포함 → 제외
row("글쓰기", 15)
);

when(repository.findTopCoursesByCategories(anySet(), any(Pageable.class)))
Expand All @@ -51,45 +57,87 @@ void getCourseRanking_returnsMajorAndLiberalTop10_withRanksStartingAt1() {
assertThat(res.major().get(1)).isEqualTo(new CourseRankingDto.RankingItem(2, "DB", 10, 0));
assertThat(res.major().get(2)).isEqualTo(new CourseRankingDto.RankingItem(3, "DS", 7, 0));

// then: liberal 랭킹 변환
assertThat(res.liberal()).hasSize(2);
assertThat(res.liberal().get(0)).isEqualTo(new CourseRankingDto.RankingItem(1, "채플", 30, 0));
assertThat(res.liberal().get(1)).isEqualTo(new CourseRankingDto.RankingItem(2, "실용영어", 20, 0));
// then: liberal 랭킹 변환 (필터링 + rank 재부여)
assertThat(res.liberal()).containsExactly(
new CourseRankingDto.RankingItem(1, "실용영어", 20, 0),
new CourseRankingDto.RankingItem(2, "철학의이해", 18, 0),
new CourseRankingDto.RankingItem(3, "글쓰기", 15, 0)
);

// repo 호출 검증(2번)
verify(repository, times(2)).findTopCoursesByCategories(anySet(), any(Pageable.class));
}

@Test
void getCourseRanking_callsRepositoryWithMajorAndNonMajorCategories_andPageSize10() {
void getCourseRanking_callsRepository_withMajorPageSize10_andLiberalPageSize30() {
// given
when(repository.findTopCoursesByCategories(anySet(), any(Pageable.class)))
.thenReturn(List.of(), List.of()); // major/libral 둘 다 빈 결과
.thenReturn(List.<CourseRankingRepository.CourseCountRow>of(),
List.<CourseRankingRepository.CourseCountRow>of());

// when
service.getCourseRanking();

// then: 인자 캡쳐해서 major/libral 분기 확인
ArgumentCaptor<Set<Category>> catCaptor = ArgumentCaptor.forClass(Set.class);
// then: 인자 캡쳐
@SuppressWarnings("unchecked")
ArgumentCaptor<Set<Category>> catCaptor = (ArgumentCaptor) ArgumentCaptor.forClass(Set.class);
ArgumentCaptor<Pageable> pageableCaptor = ArgumentCaptor.forClass(Pageable.class);

verify(repository, times(2)).findTopCoursesByCategories(catCaptor.capture(), pageableCaptor.capture());

List<Set<Category>> capturedCats = catCaptor.getAllValues();
List<Pageable> capturedPageables = pageableCaptor.getAllValues();

// pageable: page=0, size=10 확인
for (Pageable p : capturedPageables) {
assertThat(p.getPageNumber()).isEqualTo(0);
assertThat(p.getPageSize()).isEqualTo(10);
int majorIdx = -1;
int liberalIdx = -1;
for (int i = 0; i < capturedCats.size(); i++) {
if (capturedCats.get(i).contains(Category.MAJOR)) majorIdx = i;
else liberalIdx = i;
}

// categories: 한 번은 MAJOR 포함(전공), 한 번은 MAJOR 미포함(교양)
boolean sawMajorSet = capturedCats.stream().anyMatch(s -> s.contains(Category.MAJOR));
boolean sawNonMajorSet = capturedCats.stream().anyMatch(s -> !s.contains(Category.MAJOR));
assertThat(majorIdx).isNotEqualTo(-1);
assertThat(liberalIdx).isNotEqualTo(-1);

Pageable majorPageable = capturedPageables.get(majorIdx);
Pageable liberalPageable = capturedPageables.get(liberalIdx);

assertThat(majorPageable.getPageNumber()).isEqualTo(0);
assertThat(majorPageable.getPageSize()).isEqualTo(10);

assertThat(liberalPageable.getPageNumber()).isEqualTo(0);
assertThat(liberalPageable.getPageSize()).isEqualTo(30);
}

@Test
void liberalRanking_limitsTo10_afterFiltering_andRanksAreReassigned() {
// given
List<CourseRankingRepository.CourseCountRow> majorRows = List.of();

List<CourseRankingRepository.CourseCountRow> liberalRows = List.of(
row("채플", 100),
row("공동체리더십훈련", 99),
row("교양1", 98),
row("교양2", 97),
row("교양3", 96),
row("교양4", 95),
row("교양5", 94),
row("교양6", 93),
row("교양7", 92),
row("교양8", 91),
row("교양9", 90),
row("교양10", 89),
row("교양11", 88)
);

when(repository.findTopCoursesByCategories(anySet(), any(Pageable.class)))
.thenReturn(majorRows, liberalRows);

// when
CourseRankingDto.RankingResponse res = service.getCourseRanking();

assertThat(sawMajorSet).isTrue();
assertThat(sawNonMajorSet).isTrue();
// then
assertThat(res.liberal()).hasSize(10);
assertThat(res.liberal().get(0)).isEqualTo(new CourseRankingDto.RankingItem(1, "교양1", 98, 0));
assertThat(res.liberal().get(9)).isEqualTo(new CourseRankingDto.RankingItem(10, "교양10", 89, 0));
}

private static CourseRankingRepository.CourseCountRow row(String name, long takenCount) {
Expand Down
Loading