Conversation
Summary of ChangesHello @TAEW00KIM, 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! 이 Pull Request는 프론트엔드 하단 카드에서 필요한 대회 기록과 득점왕 정보를 효율적으로 가져올 수 있도록 새로운 통합 API를 도입합니다. 기존에는 여러 API를 호출해야 했던 비효율성을 개선하여, 단일 요청으로 필요한 모든 데이터를 제공함으로써 프론트엔드의 데이터 로딩 과정을 간소화하고 성능을 향상시키는 것을 목표로 합니다. 또한, 코드 품질 향상을 위해 Jacoco를 통한 코드 커버리지 측정 환경을 구축하고, 새로운 기능에 대한 철저한 테스트와 문서화를 완료했습니다. 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. Changelog
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}
| public LeagueRecentSummaryResponse findRecentSummary(Integer year, Integer recordLimit, Integer topScorerLimit) { | ||
| int safeRecordLimit = Math.max(recordLimit, 0); | ||
| int safeTopScorerLimit = Math.max(topScorerLimit, 0); | ||
|
|
||
| List<LeagueRecentSummaryResponse.LeagueRecord> records = findLeagues(new LeagueQueryRequestDto(year, LeagueProgress.FINISHED)).stream() | ||
| .filter(league -> league.winnerTeamName() != null) | ||
| .limit(safeRecordLimit) | ||
| .map(LeagueRecentSummaryResponse.LeagueRecord::from) | ||
| .toList(); | ||
|
|
||
| List<TopScorerResponse> topScorers = findTopScorersByYear(year, safeTopScorerLimit); | ||
|
|
||
| return new LeagueRecentSummaryResponse(records, topScorers); | ||
| } |
There was a problem hiding this comment.
현재 구현은 특정 연도의 모든 종료된 리그를 조회한 후, 메모리에서 필터링하고 제한하고 있습니다. 리그 수가 많아질 경우 성능 저하가 발생할 수 있습니다.
데이터베이스 수준에서 필터링과 제한을 적용하는 것이 더 효율적입니다. LeagueQueryRepository에 Pageable을 인자로 받아 필요한 만큼의 데이터만 조회하는 새로운 메소드를 추가하는 것을 고려해보세요.
예를 들어, 다음과 같은 메소드를 추가할 수 있습니다.
// In LeagueQueryRepository.java
List<League> findRecentFinishedLeagues(Integer year, Pageable pageable);이 메소드는 year와 LeagueProgress.FINISHED로 리그를 필터링하고, 우승팀이 있는 리그만 조회하며, Pageable을 통해 결과를 제한해야 합니다. 이렇게 하면 서비스 레이어의 코드가 더 간결해지고 성능도 향상될 것입니다.
| @@ -182,6 +182,10 @@ operation::league-query-controller-test/리그_득점왕을_조회한다[snippet | |||
|
|
|||
There was a problem hiding this comment.
- PR 본문에 화면 캡처본이 추가되면 더 좋을 것 같아요!
- PR 본문에
확인해야 할 부분이 대부분 프론트와의 소통이 필요한 부분인 것 같아서, 명세의 경우에는 따로 논의하신 뒤에 리뷰 요청 주셔도 좋을 것 같아요! 물론 항상 그래야 한다는 것은 아닙니닷. 다만 명세가 달라져야 하면 두 번 일을 하게 될 수도 있으니.. ㅎㅎ
| int safeRecordLimit = Math.max(recordLimit, 0); | ||
| int safeTopScorerLimit = Math.max(topScorerLimit, 0); | ||
|
|
||
| List<LeagueRecentSummaryResponse.LeagueRecord> records = findLeagues(new LeagueQueryRequestDto(year, LeagueProgress.FINISHED)).stream() |
There was a problem hiding this comment.
꼭 수정해야 하는 것은 아니고,
보다 보니까 떠오르는 생각인데 간접적으로 다른 API 와 의존도가 높아지는 방향으로의 구현이 아닐까 하는 생각이 들어요.
예시 상황
/leagues에서의 API 명세가 변경됨- 그로 인해 현재 요약 관련 API 에는 필요한 필드가 제거됨
서로 다른 API 가 의존도를 갖는 방향이 위험할 수도 있겠다는 생각도 들어요!
태우님 의견은 어떠신가요?
There was a problem hiding this comment.
아 좋은 의견인 것 같습니다. recent-summary api 전용 dto로 분리하는 방안 고려해서 다시 커밋하겠습니다
수정하는 게 나을 것 같아요 지금 보니까
| jacoco { | ||
| toolVersion = "0.8.12" | ||
| } | ||
|
|
||
| jacocoTestReport { | ||
| dependsOn test | ||
| reports { | ||
| xml.required = true | ||
| html.required = true | ||
| csv.required = false | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
./gradlew test jacocoTestReport
저렇게 수정해야 이 명령어가 돌아가서 바꿨습니다!
xml/html 리포트 생성만 보장하는 변경이고, 런타임 기능에는 영향 없습니다
There was a problem hiding this comment.
오! 태우님이 자코코 추가하고 싶으셔서 추가하신 건가요?
기존 동작에는 딱히 없던 것으로 보여서.. 👀
There was a problem hiding this comment.
네네 온보딩할 때 테스트 방식에 자코코도 있더라구요! 그래서 한번 추가해봤습니다
| int safeRecordLimit = Math.max(recordLimit, 0); | ||
| int safeTopScorerLimit = Math.max(topScorerLimit, 0); | ||
|
|
||
| List<LeagueRecentSummaryResponse.LeagueRecord> records = findLeagues(new LeagueQueryRequestDto(year, LeagueProgress.FINISHED)).stream() |
There was a problem hiding this comment.
기존의 의도는 메서드에 대해서도 마찬가지이긴 했어요!
findLeagues 도 사실상 /leagues 조회에서 사용 되고 있기에,
간접적으로 현재 API 의 구현이 /leagues 의 조회에 간접적으로 의존하는 건 아닐까요?
findLeagues(new LeagueQueryRequestDto(year, LeagueProgress.FINISHED)
현재 이 구현부 역시도, /leagues 에서의 request dto 가 바뀌면 같이 영향을 받는 부분이 아닐까요?
만약 동일한 기능을 재사용할 것으로 보인다면, public 메서드로 열려 있는 메서드를 그대로 호출하는 것이 아니라 동일한 기능을 하는 부분을 메서드로 뽑아 내는 게 안전하지 않을까 싶었어요
그리고 현재 정도의 기능은 사실상, 특성 명세에 의존되는 메서드를 호출하는 게 아니라 레포지토리 단에서의 호출로도 충분할 것 같아요. 그러면 이 코멘트에서의 성능 이슈도 해결될 듯 보이네요!
2 participants
이슈 배경
기존의 문제
해결 방식
GET /leagues/recent-summaryrecords(대회 기록)+topScorers(득점왕)를 한 번에 내려줄 수 있도록 수정확인해야 할 부분
records,topScorers)가 충분한지recordLimit,topScorerLimit파라미터 기본값/사용 방식이 프론트 요구와 맞는지