Conversation
Summary of ChangesHello @KimGyeongLock, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 전공 및 교양 과목 모두에 대한 서브 카테고리를 구현하여 과목 랭킹 기능을 크게 개선합니다. 프론트엔드에서는 전공 과목에 대한 드래그 기능을 포함한 서브 탭 UI 컴포넌트를 추가하고, 이러한 서브 카테고리에 따라 랭킹 표시 로직을 업데이트했습니다. 백엔드에서는 새로운 로드맵 인덱스를 사용하여 전공 과목을 학기별로 분류하고, 교양 과목을 특정 카테고리별로 분류하는 새로운 데이터 구조와 서비스를 개발하여 더욱 세분화되고 체계적인 랭킹 시스템을 제공합니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 과목 랭킹 기능을 전공과 교양 하위 카테고리별로 세분화하는 중요한 개선 작업을 포함하고 있네요. 백엔드에서는 학기/영역별로 데이터를 구조화하고, 프론트엔드에서는 서브 탭과 드래그 스크롤 같은 향상된 UX를 제공하여 사용성을 크게 높인 점이 인상적입니다. 전반적으로 좋은 방향의 변경이라고 생각합니다.
몇 가지 코드 품질 및 잠재적 버그 개선을 위한 제안 사항을 코멘트로 남겼습니다. 확인 부탁드립니다.
추가로, PR 제목과 설명이 없어 변경의도 파악에 어려움이 있었습니다. 저장소 스타일 가이드(131, 134-137번 라인)에 따라 Conventional Commits 형식의 커밋 메시지와 상세한 PR 설명을 작성해주시면 동료들이 변경 사항을 이해하고 리뷰하는 데 큰 도움이 될 것입니다.
| }; | ||
|
|
||
| export type MajorRanking = { | ||
| y1s1: RankingItem[]; |
There was a problem hiding this comment.
백엔드 API 응답의 major 객체에는 y1s1 필드가 포함되지 않습니다. 프론트엔드와 백엔드의 데이터 모델을 일치시키기 위해 이 라인을 제거하는 것이 좋습니다. 현재 CourseRankingService를 보면 전공 과목 랭킹은 1학년 2학기(y1s2)부터 집계하고 있습니다.
| y1s1: RankingItem[]; | |
| y1s2: RankingItem[]; |
| // ✅ 공백만 제거 + trim | ||
| private static String norm(String s) { | ||
| if (s == null) return ""; | ||
| return s.trim().replaceAll("\\s+", ""); |
There was a problem hiding this comment.
과목명으로 학기를 조회할 때, 현재 정규화 로직(norm)이 대소문자를 구분하고 있습니다. 예를 들어 'data structure'와 'Data Structure'를 다르게 취급하여 일치하지 않을 수 있습니다. toLowerCase()를 추가하여 대소문자 구분 없이 비교하면 더 안정적으로 과목을 찾을 수 있습니다.
| return s.trim().replaceAll("\\s+", ""); | |
| return s.trim().replaceAll("\\s+", "").toLowerCase(); |
| this.majorRoadmapIndex = majorRoadmapIndex; | ||
| } | ||
|
|
||
| public RankingResponse getCourseRanking() { |
There was a problem hiding this comment.
스타일 가이드 66번 라인에 따라, 읽기 전용 로직을 처리하는 서비스 메서드에는 @Transactional(readOnly = true)를 명시하는 것을 권장합니다. 이를 통해 불필요한 쓰기 트랜잭션을 방지하고 성능상 이점을 얻을 수 있습니다.
| public RankingResponse getCourseRanking() { | |
| @org.springframework.transaction.annotation.Transactional(readOnly = true) | |
| public RankingResponse getCourseRanking() { |
References
- 읽기 전용 쿼리는 성능 향상을 위해
@Transactional(readOnly = true)를 명시해야 합니다. (link)
| @@ -19,7 +18,7 @@ public CourseRankingController(CourseRankingService rankingService) { | |||
| } | |||
|
|
|||
| @GetMapping("/courses") | |||
There was a problem hiding this comment.
메서드 이름에 오타가 있습니다. courseBanking을 courseRanking으로 수정하여 의미를 명확하게 하는 것이 좋겠습니다.
public RankingResponse courseRanking() {| PageRequest.of(0, 300) | ||
| ); | ||
|
|
||
| for (var r : fetchedRows) { | ||
| var termOpt = majorRoadmapIndex.findTermByCourseName(r.getName()); | ||
| if (termOpt.isEmpty()) continue; | ||
|
|
||
| var key = termOpt.get().toBucketKey(); | ||
| var list = bucket.get(key); | ||
| if (list != null) list.add(r); // 혹시 1-1 같은게 들어오면 null이라 무시 | ||
| } | ||
|
|
||
| return new MajorRanking( | ||
| toTop10(bucket.get("y1s2")), | ||
| toTop10(bucket.get("y2s1")), | ||
| toTop10(bucket.get("y2s2")), | ||
| toTop10(bucket.get("y3s1")), | ||
| toTop10(bucket.get("y3s2")), | ||
| toTop10(bucket.get("y4s1")), | ||
| toTop10(bucket.get("y4s2")) | ||
| ); | ||
| } | ||
|
|
||
| private List<RankingItem> loadTop10(Set<Category> categories, boolean applyLiberalFilter) { | ||
| int fetchSize = applyLiberalFilter ? 30 : 10; | ||
| private List<RankingItem> toTop10(List<CourseRankingRepository.CourseCountRow> rows) { | ||
| if (rows == null) return List.of(); | ||
| var finalRows = rows.stream().limit(10).toList(); | ||
|
|
||
| return IntStream.range(0, finalRows.size()) | ||
| .mapToObj(i -> new RankingItem( | ||
| i + 1, | ||
| finalRows.get(i).getName(), | ||
| finalRows.get(i).getTakenCount(), | ||
| 0 | ||
| )) | ||
| .toList(); | ||
| } | ||
|
|
||
| private List<RankingItem> loadTop10(Set<Category> categories, boolean applyFilter) { | ||
| int fetchSize = applyFilter ? 30 : 10; |
There was a problem hiding this comment.
코드 내에 300, 30, 10과 같은 매직 넘버가 사용되었습니다. 이 값들을 의미를 명확하게 나타내는 private static final 상수로 추출하면 가독성과 유지보수성이 향상됩니다.
예시:
private static final int MAJOR_FETCH_SIZE = 300;
private static final int LIBERAL_FETCH_SIZE = 30;
private static final int RANKING_LIMIT = 10;References
- 코드의 가독성을 위해 매직넘버 사용을 지양하고, 의미있는 이름의 상수로 정의해야 합니다. (link)
|
1 participant
.