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은 과목 랭킹 서비스의 정확성과 사용자 친화성을 향상시키기 위한 중요한 개선 사항을 포함합니다. 특히, 과목명에 존재하는 미묘한 차이(예: 공백)로 인해 동일한 과목이 분리되어 집계되는 문제를 해결하고, 전공 로드맵 데이터의 정확성을 높여 전반적인 데이터 일관성을 강화했습니다. 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은 과목 랭킹을 집계할 때 공백 유무에 따라 다른 과목으로 집계되던 문제를 해결하고, major_roadmap.json의 학기별 과목 정보를 최신화하는 중요한 수정을 포함하고 있습니다.
toTop10Merged 메서드를 도입하여 과목명을 정규화하고 수강 횟수를 합산하는 로직을 추가한 점이 인상적입니다. 이를 통해 데이터의 정확성을 크게 향상시켰습니다.
리뷰에서는 주로 코드의 가독성과 재사용성을 높이기 위한 몇 가지 제안을 드렸습니다.
Agg내부 클래스를 Javarecord로 전환하여 코드를 간소화Comparator와Map.merge를 사용하여 로직을 더 선언적으로 표현- 여러 곳에서 중복되는
norm메서드를 공통 유틸리티로 분리
추가로, PR 설명이 비어있습니다. 스타일 가이드(134-137 라인)에 따라 변경 내용, 이유, 관련 이슈 등을 작성해주시면 다른 팀원들이 변경 사항을 이해하는 데 큰 도움이 될 것입니다.
전반적으로 기능 개선을 위한 좋은 커밋이라고 생각합니다. 수고하셨습니다!
| private static String norm(String s) { | ||
| if (s == null) return ""; | ||
| return s.trim().replaceAll("\\s+", ""); | ||
| } |
There was a problem hiding this comment.
norm() 유틸리티 메서드가 MajorRoadmapIndex 클래스에도 동일하게 존재합니다. 코드 중복을 피하고 재사용성을 높이기 위해, 이 메서드를 공통 유틸리티 클래스로 추출하는 것을 고려해 보세요.
스타일 가이드 25번 라인에 따라 com.gradu.common 패키지 아래에 StringUtil 같은 클래스를 만들어 관리하면 좋을 것 같습니다.
예시:
// in com.gradu.common.util.StringUtil.java
public final class StringUtil {
private StringUtil() {}
public static String normalize(String s) {
if (s == null) return "";
return s.trim().replaceAll("\\s+", "");
}
}References
- 공통 유틸리티는
com.gradu/common/패키지에 위치시켜야 합니다.norm()메서드는 여러 곳에서 사용되므로 공통 유틸리티로 분리하는 것이 좋습니다. (link)
| for (var r : rows) { | ||
| String raw = r.getName(); | ||
| String key = norm(raw); | ||
| if (key.isBlank()) continue; | ||
|
|
||
| Agg prev = map.get(key); | ||
| long nextCount = (prev == null ? 0L : prev.takenCount) + r.getTakenCount(); | ||
|
|
||
| // ✅ 대표 표기명: 사람이 읽기 편한 쪽(공백 포함, 더 긴 것) 우선 | ||
| String nextDisplay = (prev == null) | ||
| ? safe(raw) | ||
| : chooseDisplay(prev.displayName, safe(raw)); | ||
|
|
||
| map.put(key, new Agg(nextCount, nextDisplay)); | ||
| } |
There was a problem hiding this comment.
for 루프 내의 집계 로직을 Map.merge() 메서드를 사용하여 더 간결하게 개선할 수 있습니다. merge를 사용하면 키 존재 여부를 확인하고 값을 계산하여 업데이트하는 과정을 하나의 메서드 호출로 처리할 수 있어 코드 가독성이 향상됩니다. (이 제안은 Agg가 record라고 가정합니다.)
for (var r : rows) {
String raw = r.getName();
String key = norm(raw);
if (key.isBlank()) continue;
var newAgg = new Agg(r.getTakenCount(), safe(raw));
map.merge(key, newAgg, (oldAgg, agg) -> new Agg(
oldAgg.takenCount() + agg.takenCount(),
chooseDisplay(oldAgg.displayName(), agg.displayName())
));
}| .sorted((a, b) -> { | ||
| int c = Long.compare(b.takenCount, a.takenCount); | ||
| if (c != 0) return c; | ||
| return a.displayName.compareTo(a.displayName); | ||
| }) |
There was a problem hiding this comment.
Comparator 체이닝을 사용하면 정렬 로직을 더 간결하고 읽기 쉽게 표현할 수 있습니다. Comparator.comparingLong()과 thenComparing()을 활용하여 다중 조건 정렬을 구현하는 것을 권장합니다. (이 제안을 적용하려면 Agg 클래스를 record로 변경하거나 getter를 추가해야 합니다.)
.sorted(Comparator.comparingLong(Agg::takenCount).reversed()
.thenComparing(Agg::displayName))| private static final class Agg { | ||
| final long takenCount; | ||
| final String displayName; | ||
|
|
||
| private Agg(long takenCount, String displayName) { | ||
| this.takenCount = takenCount; | ||
| this.displayName = displayName; | ||
| } | ||
| } |
There was a problem hiding this comment.
Agg 클래스는 불변 데이터 컨테이너(immutable data carrier)의 역할을 합니다. Java record를 사용하면 보일러플레이트 코드(생성자, getter, equals, hashCode, toString) 없이 클래스를 더 간결하게 정의할 수 있습니다.
프로젝트 스타일 가이드 62번 라인("가독성 향상에 도움이 되면 record ... 사용")에서도 record 사용을 권장하고 있습니다.
private record Agg(long takenCount, String displayName) {}References
- 가독성 향상에 도움이 된다면 Java 21의
record와 같은 최신 기능을 사용하는 것을 권장합니다.Agg클래스는record로 변환하기에 좋은 후보입니다. (link)
|
1 participant
.