-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor/444] 증분 업데이트 로직 추가 #445
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @coldmans, 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은 라이엇 API를 사용하는 전적 갱신 시스템의 성능과 효율성을 대폭 개선하기 위한 증분 업데이트 로직을 도입합니다. 기존에는 매번 라이엇 API를 통해 모든 매치 데이터를 조회했지만, 이제는 Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
이번 PR은 Riot 게임 기록을 가져오는 로직에 증분 업데이트를 성공적으로 구현하여 API 호출을 크게 줄이고 성능을 향상시켰습니다. GameMatch 엔티티와 리포지토리는 적절한 인덱스와 유니크 제약 조건을 포함하여 잘 설계되었습니다. 신규 매치를 먼저 데이터베이스에 저장한 후 통계를 계산하는 방식으로 리팩토링한 것은 안정적인 접근 방식입니다. 특히 동시성 테스트를 포함한 포괄적인 통합 테스트를 추가한 점은 변경 사항에 대한 신뢰도를 높여줍니다.
몇 가지 개선점을 제안합니다.
RiotRecordService에서 서버 환경에 따른 시간 처리 문제를 피하기 위해 시스템 기본 시간대 대신 UTC와 같은 특정 시간대를 사용하는 것이 좋습니다.- 가독성과 유지보수성을 위해 기본 게임 시간에 대한 매직 넘버를 상수로 정의해야 합니다.
- 테스트 파일에 API 키가 하드코딩되어 있어 보안상 위험하므로 제거해야 합니다.
전반적으로 매우 수준 높은 리팩토링 작업입니다. 제안된 사항들을 반영하면 코드가 더욱 견고하고 안전해질 것입니다.
src/test/java/com/gamegoo/gamegoo_v2/external/riot/CheckNewAramQueueIdTest.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
이 PR은 Riot API 호출을 최적화하기 위해 증분 업데이트 로직을 도입하는 중요한 리팩토링을 수행합니다. 전반적으로 새로운 GameMatch 엔티티와 GameMatchRepository를 도입하여 매치 데이터를 DB에 저장하고, 이를 기반으로 통계를 계산하는 방식의 변경은 매우 훌륭합니다. 특히 동시성 문제를 고려한 예외 처리와 성능을 위한 인덱스 설정 등 세심한 구현이 돋보입니다. 테스트 코드 또한 통합 테스트와 동시성 테스트를 포함하여 견고하게 작성되었습니다. 몇 가지 개선점을 제안 드리며, 이를 반영하면 코드의 안정성과 유지보수성이 더욱 향상될 것입니다.
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
이번 PR은 Riot API 호출을 줄이고 성능을 개선하기 위해 사용자 전적 조회에 증분 업데이트 로직을 도입하는 중요한 리팩토링입니다. GameMatch 엔티티에 매치 데이터를 캐싱하고 새로운 매치만 가져오도록 변경하여 시스템 효율성을 크게 향상시켰습니다. 데이터베이스 인덱스와 유니크 제약 조건의 활용이 매우 적절합니다. 또한, 플레이어 식별을 위해 gameName 대신 puuid를 사용하도록 변경한 점은 시스템의 안정성을 높이는 훌륭한 개선입니다. 특히 동시성 환경을 고려한 통합 테스트를 추가한 점이 인상적입니다.
리뷰 결과, 동시 요청 시 발생할 수 있는 트랜잭션 관련 치명적인(critical) 문제점을 한 가지 발견했습니다. 이에 대한 상세한 설명과 해결 방안을 코멘트로 남겼습니다. 그 외 몇 가지 코드 개선을 위한 제안도 포함되어 있습니다. 전반적으로 매우 잘 수행된 중요한 리팩토링이라고 생각합니다.
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Show resolved
Hide resolved
src/main/java/com/gamegoo/gamegoo_v2/external/riot/domain/GameMatch.java
Show resolved
Hide resolved
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Show resolved
Hide resolved
rimi3226
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생 많으셨습니다!
저도 함께 해야하는데 먼저 해주셔서 감사합니다 ㅎㅎ
다만, 주석에서 코파일럿이나 지피티 같이 생성형 AI를 사용한 부분이 너무 많이 티가 나는 것 같아요. 또한, RiotRecordService에서 너무 많은 일을 한다고 느껴졌습니다. 그래서 해당 클래스의 리팩토링이 필요하다고 생각합니다.
제가 코드에 대한 이해가 부족하여 자세하게 리뷰를 드리지 못해서 죄송합니다. 그래서 함께 코드 보면서 수정해보면 좋을 것 같습니다.
제가 맡은 RSO 회원가입에서도 진형님께서 작성해주신 코드를 사용할 예정입니다. 따라서, 코드에 대한 설명을 듣는 것이 더욱 좋을 것 같습니다.
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gamegoo/gamegoo_v2/external/riot/service/RiotRecordService.java
Show resolved
Hide resolved
| public void updateFrom(Recent30GameStatsResponse stats) { | ||
| this.update( | ||
| stats.getRecTotalWins(), | ||
| stats.getRecTotalLosses(), | ||
| stats.getRecWinRate(), | ||
| stats.getRecAvgKDA(), | ||
| stats.getRecAvgKills(), | ||
| stats.getRecAvgDeaths(), | ||
| stats.getRecAvgAssists(), | ||
| stats.getRecAvgCsPerMinute(), | ||
| stats.getRecTotalCs() | ||
| ); | ||
| } | ||
|
|
||
| public void updateSoloStatsFrom(Recent30GameStatsResponse stats) { | ||
| this.updateSoloStats( | ||
| stats.getRecTotalWins(), | ||
| stats.getRecTotalLosses(), | ||
| stats.getRecWinRate(), | ||
| stats.getRecAvgKDA(), | ||
| stats.getRecAvgKills(), | ||
| stats.getRecAvgDeaths(), | ||
| stats.getRecAvgAssists(), | ||
| stats.getRecAvgCsPerMinute(), | ||
| stats.getRecTotalCs() | ||
| ); | ||
| } | ||
|
|
||
| public void updateFreeStatsFrom(Recent30GameStatsResponse stats) { | ||
| this.updateFreeStats( | ||
| stats.getRecTotalWins(), | ||
| stats.getRecTotalLosses(), | ||
| stats.getRecWinRate(), | ||
| stats.getRecAvgKDA(), | ||
| stats.getRecAvgKills(), | ||
| stats.getRecAvgDeaths(), | ||
| stats.getRecAvgAssists(), | ||
| stats.getRecAvgCsPerMinute(), | ||
| stats.getRecTotalCs() | ||
| ); | ||
| } | ||
|
|
||
| public void updateAramStatsFrom(Recent30GameStatsResponse stats) { | ||
| this.updateAramStats( | ||
| stats.getRecTotalWins(), | ||
| stats.getRecTotalLosses(), | ||
| stats.getRecWinRate(), | ||
| stats.getRecAvgKDA(), | ||
| stats.getRecAvgKills(), | ||
| stats.getRecAvgDeaths(), | ||
| stats.getRecAvgAssists(), | ||
| stats.getRecAvgCsPerMinute(), | ||
| stats.getRecTotalCs() | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위에있는 메소드들과 겹친다. Recent30GameStatsResponse dto를 매개변수로 사용하는 메서드만 냅두기.
| try { | ||
| Member member = memberService.findMemberById(memberId); | ||
| championStatsRefreshService.refreshChampionStats(member); | ||
| championStatsRefreshService.refreshChampionStats(memberId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
파사드 서비스가 아닌 곳에서 서비스 호출 금지
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
비동기는 트랜잭션 시작을 확실하게 하기 위해서 Riot 관련 파사드는 비동기용으로 하나 더 추가하기.
| String gameName = freshMember.getGameName(); | ||
| String tag = freshMember.getTag(); | ||
| String puuid = freshMember.getPuuid() != null ? freshMember.getPuuid() : riotAuthService.getPuuid(gameName, tag); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
refreshChampionStats 메서드 안에 하고 있는 일이 너무 많음. 조회도 하고 저장도 하고 확인도 하고, 이거를 get, save, validate 같은 다른 여러가지 메서드로 나눠야한다고 생각함.
var 사용을 지양해줬으면 좋겠음. 어떤 자료형인지 한 눈에 보이지 않고 알려고하면 메서드를 한 번 클릭해서 들어가야함.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getTopChampions 메서드가 dto가 아니라 converter를 새로 만들던지, service를 새로 만들어서 넣으면 좋을 것 같음
| /** | ||
| * 매치를 플레이한 사용자 | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기 // 주석으로 통일해줬으면 좋겠어
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RiotRecordService를 기록하는 용도, 저장하는 용도로만 사용하고, API를 조회하는 서비스는 따로 파는 것이 좋을 것 같음.
| private Recent30GameStatsResponse calculateStatsFromMatches(List<String> matchIds, String puuid, | ||
| java.util.function.Predicate<ChampionStats> filter) { | ||
| List<ChampionStats> filteredStats = matchIds.stream() | ||
| .map(matchId -> fetchChampionStatsFromMatch(matchId, puuid)) | ||
| .filter(Optional::isPresent) | ||
| .map(Optional::get) | ||
| .filter(filter) | ||
| .collect(Collectors.toList()); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
계산하는 서비스도 따로 분리하는 것이 좋을 것 같음.
| // =========================== | ||
| // 증분 업데이트 (DB 캐싱) 로직 | ||
| // =========================== | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이렇게 구분점이 꼭 필요할 정도라면 차라리 클래스를 하나 더 만들어서 하는 편이 좋지 않을까라는 생각이 들음.
| AllModeStatsCollector collector = new AllModeStatsCollector(); | ||
| championStatsList.forEach(collector::processChampionStats); | ||
|
|
||
| return collector.buildResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new 생성자 사용 지양
| public AllModeStatsResponse buildResponse() { | ||
| Recent30GameStatsResponse combinedStats = buildStatsResponse( | ||
| totalWins[0], totalLosses[0], totalKills[0], totalDeaths[0], | ||
| totalAssists[0], totalCs[0], totalCsPerMinute[0], totalGames[0]); | ||
| Recent30GameStatsResponse soloStats = buildStatsResponse( | ||
| totalWins[1], totalLosses[1], totalKills[1], totalDeaths[1], | ||
| totalAssists[1], totalCs[1], totalCsPerMinute[1], totalGames[1]); | ||
| Recent30GameStatsResponse freeStats = buildStatsResponse( | ||
| totalWins[2], totalLosses[2], totalKills[2], totalDeaths[2], | ||
| totalAssists[2], totalCs[2], totalCsPerMinute[2], totalGames[2]); | ||
| Recent30GameStatsResponse aramStats = buildStatsResponse( | ||
| totalWins[3], totalLosses[3], totalKills[3], totalDeaths[3], | ||
| totalAssists[3], totalCs[3], totalCsPerMinute[3], totalGames[3]); | ||
|
|
||
| return AllModeStatsResponse.builder() | ||
| .combinedStats(combinedStats) | ||
| .soloStats(soloStats) | ||
| .freeStats(freeStats) | ||
| .aramStats(aramStats) | ||
| .combinedChampionStats(combinedChampionStats) | ||
| .soloChampionStats(soloChampionStats) | ||
| .freeChampionStats(freeChampionStats) | ||
| .aramChampionStats(aramChampionStats) | ||
| .build(); | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response를 만드는거는 dto의 컨버터가 할 부분이지, 유틸에서 하는 일이 아니라고 생각함.
컨트롤러 : 엔드포인트와 파사드 서비스를 엮기 위한 클래스. 클라이언트로 받아오는 요청을 검증하는 어노테이션 붙이기.
파사드 서비스 : 서비스를 합쳐서 컨트롤러로 전달해주는 역할
서비스 : 비즈니스 로직에 해당, 하나의 메서드가 하나의 일을 하도록 제일 엄격하게 관리되어야함.
dto : 요청/응답 dto와 데이터를 담아두기 위한 dto로 나뉨. 각 dto의 데이터를 관리하고 만드는 일은 dto 내의 builder, from, of나 새로운 컨버터에서 진행.
유틸리티 : 레포지토리와 관련되지 않는, @component 가 붙을 필요가 없는데, "여러가지 서비스에서 다양한 용도로 사용될 수 있는 함수"인 경우에 사용 가능
domain : 엔티티 관련 내용만 들어가야함. 근데 엔티티의 데이터를 지지고 볶는 작업은 repository나 custom을 만들어서 하는 일임. 여기 클래스에 들어가도 되는 메서드는 @Setter를 지양하기 위한 update 메서드만 들어가야함.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그래서 collector라는 클래스가 하는 일을 명확하게 하는 편이 좋아보임.
buildResponse같은 메서드는 dto에서 해야하는 일이기 때문에 나머지 메서드들이 있어야할 곳을 생각하면서 각 하는 일을 의도에 맞게 나누는 것이 중요해보임.
🚀 개요
⏳ 작업 상세 내용
📝 더 꼼꼼히 봐야할 부분
🔍 반드시 참고해야하는 변경 사항