Conversation
Summary of ChangesHello @Yeeyahou, 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 호출에 대한 Rate Limiting을 적용하여 시스템의 정확성과 안정성을 높였습니다. 또한, 랭킹 응답 데이터에 프로필 이미지와 총자산 정보를 추가하여 사용자에게 더 유용한 정보를 제공합니다. 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.
Code Review
이번 PR은 랭킹 산정 기준을 기존의 '현금 잔액'에서 '총자산(현금+주식 평가액)'으로 변경하는 중요한 리팩토링을 포함하고 있습니다. 이를 위해 주식 현재가를 일괄 조회하고, 외부 API 호출을 제어하기 위해 RateLimiter를 도입하는 등 많은 개선이 이루어졌습니다. 전반적인 로직의 방향은 좋으나, 몇 가지 심각한 성능 문제를 유발할 수 있는 부분(N+1 쿼리, 비효율적인 데이터 수집)이 발견되었습니다. 또한, 일부 코드의 가독성 및 중복을 개선할 여지가 있습니다. 아래에 자세한 리뷰를 남겼으니 확인 부탁드립니다.
| private BigDecimal calculateTotalAssets(Account account, Map<String, BigDecimal> currentPrices) { | ||
| BigDecimal cash = account.getCash(); | ||
| BigDecimal stockValue = BigDecimal.ZERO; | ||
|
|
||
| List<AccountStock> holdings = accountStockRepository.findAllByAccount(account); | ||
| for (AccountStock holding : holdings) { | ||
| if (holding.getQuantity() <= 0) { | ||
| continue; | ||
| } | ||
|
|
||
| String stockCode = holding.getStock().getCode(); | ||
| BigDecimal currentPrice = currentPrices.getOrDefault(stockCode, BigDecimal.ZERO); | ||
| BigDecimal value = currentPrice.multiply(BigDecimal.valueOf(holding.getQuantity())); | ||
| stockValue = stockValue.add(value); | ||
| } | ||
|
|
||
| return cash.add(stockValue); | ||
| } |
There was a problem hiding this comment.
calculateTotalAssets 메소드 내에서 accountStockRepository.findAllByAccount(account)를 호출하고 있습니다. 이 메소드는 getMainRankingsWithPrices와 getContestRankingsWithPrices의 스트림 내부에서 각 계정마다 호출되어, 계정 수만큼(N) 데이터베이스 쿼리가 추가로 발생하는 "N+1 쿼리 문제"를 유발합니다. 사용자가 많아지면 랭킹을 계산하는 데 엄청난 수의 쿼리가 발생하여 시스템에 심각한 성능 저하를 가져올 수 있습니다.
랭킹을 계산하기 전에 필요한 모든 AccountStock 정보를 한 번의 쿼리로 가져온 후, 메모리에서 계정별로 그룹화하여 사용하는 방식으로 리팩토링해야 합니다.
예시:
- 랭킹 대상 계정 목록을 가져옵니다.
- 해당 계정 ID 목록을 사용하여
AccountStockRepository에서 모든 관련AccountStock을 한 번에 조회합니다. (findAllByAccountIn(...)) - 조회된
AccountStock목록을Map<Account, List<AccountStock>>형태로 가공합니다. calculateTotalAssets에서는 이 Map을 사용하여 DB 조회 없이 총자산을 계산합니다.
| private Set<String> collectAllHeldStockCodes() { | ||
| List<AccountStock> allAccountStocks = accountStockRepository.findAll(); | ||
| return allAccountStocks.stream() | ||
| .filter(as -> as.getQuantity() > 0) | ||
| .map(as -> as.getStock().getCode()) | ||
| .collect(Collectors.toSet()); | ||
| } |
There was a problem hiding this comment.
collectAllHeldStockCodes 메소드에서 accountStockRepository.findAll()을 호출하여 모든 AccountStock 엔티티를 메모리로 로드한 후 스트림으로 처리하고 있습니다. 보유 주식 데이터가 많아질 경우 심각한 메모리 부족(OOM)을 유발할 수 있는 매우 비효율적인 방식입니다. AccountStockRepository에 JPQL을 사용하여 SELECT DISTINCT as.stock.code FROM AccountStock as WHERE as.quantity > 0와 같이 데이터베이스에서 직접 필요한 데이터(stock code)만 조회하는 메소드를 추가하고, 이 메소드를 호출하도록 수정하는 것을 강력히 권장합니다.
| private List<RankingDto> convertToRankingDtosWithAssets(List<AccountWithAssets> accountsWithAssets, boolean includeReturn) { | ||
| List<RankingDto> rankings = new ArrayList<>(); | ||
| int rank = 1; | ||
| BigDecimal prevValue = null; | ||
| int sameRankCount = 0; | ||
|
|
||
| for (AccountWithAssets awa : accountsWithAssets) { | ||
| Account account = awa.account; | ||
| BigDecimal currentValue = awa.totalAssets; | ||
|
|
||
| // 동률 처리: 이전 값과 같으면 같은 순위 | ||
| if (prevValue != null && prevValue.compareTo(currentValue) == 0) { | ||
| sameRankCount++; | ||
| } else { | ||
| rank += sameRankCount; | ||
| sameRankCount = 1; | ||
| } | ||
|
|
||
| for (Account account : accounts) { | ||
| BigDecimal returnRate = null; | ||
|
|
||
| // 수익률 계산 (대회 계좌만) | ||
| if (includeReturn && account.getContest() != null) { | ||
| returnRate = calculateReturnRate(account, account.getContest()); | ||
| } | ||
|
|
||
| RankingDto dto = RankingDto.builder() | ||
| .rank(rank++) | ||
| .rank(rank) | ||
| .memberId(account.getMember().getMemberId()) | ||
| .nickname(account.getMember().getName()) | ||
| .balance(account.getCash()) | ||
| .profileImage(account.getMember().getProfileImage()) | ||
| .balance(account.getCash()) // 실제 잔액 (현금만) | ||
| .totalAssets(currentValue) // 총자산 (잔액 + 주식) | ||
| .returnRate(returnRate) | ||
| .build(); | ||
|
|
||
| rankings.add(dto); | ||
| prevValue = currentValue; | ||
| } | ||
|
|
||
| return rankings; | ||
| } |
There was a problem hiding this comment.
동점자 순위 처리 로직이 다소 복잡하여 가독성이 떨어집니다. rank와 sameRankCount 변수를 사용하여 다음 순위를 계산하는 방식이 직관적이지 않습니다. 정렬된 리스트의 인덱스를 활용하면 훨씬 간단하고 명확하게 동점자 순위를 처리할 수 있습니다. 가독성 및 유지보수성 향상을 위해 로직을 개선하는 것을 제안합니다.
private List<RankingDto> convertToRankingDtosWithAssets(List<AccountWithAssets> accountsWithAssets, boolean includeReturn) {
List<RankingDto> rankings = new ArrayList<>();
for (int i = 0; i < accountsWithAssets.size(); i++) {
AccountWithAssets awa = accountsWithAssets.get(i);
Account account = awa.account;
int currentRank;
// 이전 사용자와 점수가 같으면 같은 순위, 아니면 현재 인덱스+1을 순위로 사용
if (i > 0 && awa.totalAssets.compareTo(accountsWithAssets.get(i - 1).totalAssets) == 0) {
currentRank = rankings.get(i - 1).getRank();
} else {
currentRank = i + 1;
}
BigDecimal returnRate = null;
if (includeReturn && account.getContest() != null) {
returnRate = calculateReturnRate(account, account.getContest());
}
RankingDto dto = RankingDto.builder()
.rank(currentRank)
.memberId(account.getMember().getMemberId())
.nickname(account.getMember().getName())
.profileImage(account.getMember().getProfileImage())
.balance(account.getCash()) // 실제 잔액 (현금만)
.totalAssets(awa.totalAssets) // 총자산 (잔액 + 주식)
.returnRate(returnRate)
.build();
rankings.add(dto);
}
return rankings;
}| private List<RankingDto> convertToRankingDtosWithAssetsForReturnRate( | ||
| List<AccountWithAssets> accountsWithAssets, Contest contest, Map<String, BigDecimal> currentPrices) { | ||
|
|
||
| List<RankingDto> rankings = new ArrayList<>(); | ||
| int rank = 1; | ||
| BigDecimal prevReturnRate = null; | ||
| int sameRankCount = 0; | ||
|
|
||
| for (AccountWithAssets awa : accountsWithAssets) { | ||
| Account account = awa.account; | ||
| BigDecimal returnRateValue = awa.totalAssets; // totalAssets에 수익률이 들어있음 | ||
|
|
||
| // 동률 처리 | ||
| if (prevReturnRate != null && prevReturnRate.compareTo(returnRateValue) == 0) { | ||
| sameRankCount++; | ||
| } else { | ||
| rank += sameRankCount; | ||
| sameRankCount = 1; | ||
| } | ||
|
|
||
| // 실제 총자산 계산 | ||
| BigDecimal actualTotalAssets = currentPrices.isEmpty() | ||
| ? account.getCash() | ||
| : calculateTotalAssets(account, currentPrices); | ||
|
|
||
| RankingDto dto = RankingDto.builder() | ||
| .rank(rank) | ||
| .memberId(account.getMember().getMemberId()) | ||
| .nickname(account.getMember().getName()) | ||
| .profileImage(account.getMember().getProfileImage()) | ||
| .balance(account.getCash()) // 실제 잔액 (현금만) | ||
| .totalAssets(actualTotalAssets) // 총자산 (잔액 + 주식) | ||
| .returnRate(returnRateValue) | ||
| .build(); | ||
|
|
||
| rankings.add(dto); | ||
| prevReturnRate = returnRateValue; | ||
| } | ||
|
|
||
| BigDecimal seedMoney = BigDecimal.valueOf(contest.getSeedMoney()); | ||
| if (seedMoney.compareTo(BigDecimal.ZERO) == 0) { | ||
| return BigDecimal.ZERO; // 0으로 나누기 방지 | ||
| return rankings; | ||
| } |
There was a problem hiding this comment.
| log.info("📊 Main 계좌 랭킹 조회 (총자산 기준 - DB에서 로드)"); | ||
|
|
||
| // 1. DB에서 Main 계좌 전체 조회 | ||
| List<Account> accounts = accountRepository.findMainAccountsOrderByBalance(); |
There was a problem hiding this comment.
accountRepository.findMainAccountsOrderByBalance() 메소드를 호출하고 있지만, 이후 스트림에서 .sorted((a, b) -> b.totalAssets.compareTo(a.totalAssets))를 통해 메모리에서 '총자산' 기준으로 다시 정렬하고 있습니다. 데이터베이스에서 balance 기준으로 정렬하는 것은 이제 불필요하며, 메소드 이름 또한 현재의 사용 방식과 맞지 않아 혼란을 줄 수 있습니다.
AccountRepository에서 ORDER BY 절을 제거하고, 메소드 이름을 findMainAccounts()와 같이 보다 일반적인 이름으로 변경하는 것을 권장합니다.
| jpa: | ||
| hibernate: | ||
| ddl-auto: update | ||
| ddl-auto: create |
|
@gemini-code-assist |
There was a problem hiding this comment.
Code Review
안녕하세요. 랭킹 시스템을 기존의 잔액(cash) 기준에서 총자산(total assets) 기준으로 변경하는 리팩토링 작업을 리뷰했습니다. 전반적으로 주식의 현재가를 포함하여 랭킹을 산정하는 방향은 매우 긍정적이며, 외부 API 호출에 대한 Rate Limiter 적용과 캐싱 전략을 통해 성능을 개선하려는 노력이 돋보입니다. 하지만 getMyRank 메서드의 정확성과 성능에 몇 가지 치명적인 문제가 발견되었으며, 전체 랭킹을 생성하는 로직에서도 성능 저하를 유발할 수 있는 부분이 확인되었습니다. 또한, 캐시 키 관리와 개발 환경 설정에 대한 개선점도 제안드렸습니다. 아래의 상세 리뷰를 확인하시고 수정 부탁드립니다.
src/main/java/grit/stockIt/domain/ranking/service/RankingService.java
Outdated
Show resolved
Hide resolved
| Set<String> requiredStockCodes = collectAllHeldStockCodes(); | ||
| Map<String, BigDecimal> currentPrices = batchFetchCurrentPrices(requiredStockCodes); |
There was a problem hiding this comment.
getMyRank 메서드는 특정 사용자 한 명의 순위를 조회하는 기능인데, collectAllHeldStockCodes()를 호출하여 시스템의 모든 사용자가 보유한 주식 코드를 조회하고 있습니다. 이는 사용자와 보유 주식이 늘어날수록 심각한 성능 저하를 유발합니다. 해당 사용자가 보유한 주식의 현재가만 조회하도록 로직을 수정해야 합니다.
| Set<String> requiredStockCodes = collectAllHeldStockCodes(); | |
| Map<String, BigDecimal> currentPrices = batchFetchCurrentPrices(requiredStockCodes); | |
| // 2-1. 현재가 수집 및 총자산 계산 | |
| List<AccountStock> myAccountStocks = accountStockRepository.findAllByAccount(myAccount); | |
| Set<String> requiredStockCodes = myAccountStocks.stream() | |
| .map(as -> as.getStock().getCode()) | |
| .collect(Collectors.toSet()); | |
| Map<String, BigDecimal> currentPrices = batchFetchCurrentPrices(requiredStockCodes); |
| List<AccountStock> allAccountStocks = accountStockRepository.findAll(); | ||
| Map<Account, List<AccountStock>> accountStocksMap = allAccountStocks.stream() | ||
| .collect(Collectors.groupingBy(AccountStock::getAccount)); |
There was a problem hiding this comment.
accountStockRepository.findAll()을 호출하여 데이터베이스의 모든 AccountStock 데이터를 메모리로 가져오고 있습니다. 이는 데이터 양이 많아질 경우 심각한 성능 저하와 메모리 부족(OOM)을 유발할 수 있는 위험한 코드입니다. 현재 랭킹을 계산하는데 필요한 계좌들(accounts 리스트)에 대해서만 AccountStock을 조회하도록 수정해야 합니다.
AccountStockRepository에 findByAccountIn(List<Account> accounts)과 같은 메서드를 추가하여 이 문제를 해결할 수 있습니다.
// 2. 모든 계좌의 AccountStock을 한 번에 조회 (N+1 해결)
// TODO: accountRepository에 findByAccountIn(accounts)와 같은 메서드를 추가하여 필요한 데이터만 조회하도록 개선해야 합니다.
List<AccountStock> allAccountStocks = accountStockRepository.findAll(); // 개선 필요
Map<Account, List<AccountStock>> accountStocksMap = allAccountStocks.stream()
.filter(as -> accounts.contains(as.getAccount())) // 임시 필터링
.collect(Collectors.groupingBy(AccountStock::getAccount));| @PathVariable("contestId") Long contestId, | ||
|
|
||
| @Parameter(description = "정렬 기준 (balance: 잔액순, returnRate: 수익률순)", example = "balance") | ||
| @RequestParam(defaultValue = "balance") String sortBy | ||
| @Parameter(description = "정렬 기준 (totalAssets: 총자산순, returnRate: 수익률순)", example = "totalAssets") | ||
| @RequestParam(name = "sortBy", defaultValue = "totalAssets") String sortBy |
| * | ||
| * @return RankingResponse (contestId = null, sortBy = "balance") | ||
| */ | ||
| @Cacheable(value = "rankings", key = "'main:balance'") |
There was a problem hiding this comment.
랭킹 산정 기준이 totalAssets으로 변경되었으므로, 캐시 키를 'main:balance'로 사용하는 것은 혼란을 줄 수 있습니다. 로직의 내용과 일치하도록 키를 'main:totalAssets'로 변경하는 것을 권장합니다. 이렇게 하면 캐시의 내용과 키가 명확하게 일치하여 유지보수 시 오해를 줄일 수 있습니다.
| @Cacheable(value = "rankings", key = "'main:balance'") | |
| @Cacheable(value = "rankings", key = "'main:totalAssets'") |
| @Cacheable(value = "rankings", key = "'contest:' + #contestId + ':' + #sortBy") | ||
| public RankingResponse getContestRankings(Long contestId, String sortBy) { | ||
| log.info("📊 대회 [{}] 랭킹 조회 (sortBy: {}) - DB에서 로드", contestId, sortBy); | ||
| log.info("📊 대회 [{}] 랭킹 조회 (sortBy: {}) - 총자산 기준 DB 로드", contestId, sortBy); | ||
|
|
||
| // 1. 대회 존재 여부 확인 | ||
| Contest contest = contestRepository.findById(contestId) | ||
| .orElseThrow(() -> new IllegalArgumentException("대회를 찾을 수 없습니다. (ID: " + contestId + ")")); | ||
|
|
||
| // 2. sortBy에 따라 DB 조회 | ||
| List<Account> accounts; | ||
| boolean isReturnRate = "returnRate".equalsIgnoreCase(sortBy); | ||
|
|
||
| if (isReturnRate) { | ||
| // 수익률순 조회 | ||
| accounts = accountRepository.findByContestIdOrderByReturnRate(contestId); | ||
| } else { | ||
| // 잔액순 조회 (기본값) | ||
| accounts = accountRepository.findByContestIdOrderByBalance(contestId); | ||
| // sortBy 정규화: balance → totalAssets (하위 호환성) | ||
| if ("balance".equalsIgnoreCase(sortBy)) { | ||
| sortBy = "totalAssets"; | ||
| } |
There was a problem hiding this comment.
클라이언트가 sortBy 파라미터로 'balance'를 보내도 내부적으로 'totalAssets'으로 처리하여 동일한 결과를 반환합니다. 하지만 현재 @Cacheable 설정은 메서드에 전달된 원본 sortBy 값을 기준으로 캐시 키를 생성합니다. 이로 인해 sortBy=balance와 sortBy=totalAssets 요청이 각각 다른 키로 캐시되어, 동일한 데이터가 중복으로 캐시되는 비효율이 발생합니다. SpEL을 사용하여 캐시 키를 생성하기 전에 sortBy 값을 정규화하는 것을 추천합니다.
| @Cacheable(value = "rankings", key = "'contest:' + #contestId + ':' + #sortBy") | |
| public RankingResponse getContestRankings(Long contestId, String sortBy) { | |
| log.info("📊 대회 [{}] 랭킹 조회 (sortBy: {}) - DB에서 로드", contestId, sortBy); | |
| log.info("📊 대회 [{}] 랭킹 조회 (sortBy: {}) - 총자산 기준 DB 로드", contestId, sortBy); | |
| // 1. 대회 존재 여부 확인 | |
| Contest contest = contestRepository.findById(contestId) | |
| .orElseThrow(() -> new IllegalArgumentException("대회를 찾을 수 없습니다. (ID: " + contestId + ")")); | |
| // 2. sortBy에 따라 DB 조회 | |
| List<Account> accounts; | |
| boolean isReturnRate = "returnRate".equalsIgnoreCase(sortBy); | |
| if (isReturnRate) { | |
| // 수익률순 조회 | |
| accounts = accountRepository.findByContestIdOrderByReturnRate(contestId); | |
| } else { | |
| // 잔액순 조회 (기본값) | |
| accounts = accountRepository.findByContestIdOrderByBalance(contestId); | |
| // sortBy 정규화: balance → totalAssets (하위 호환성) | |
| if ("balance".equalsIgnoreCase(sortBy)) { | |
| sortBy = "totalAssets"; | |
| } | |
| @Cacheable(value = "rankings", key = "'contest:' + #contestId + ':' + (#sortBy.equalsIgnoreCase('balance') ? 'totalAssets' : #sortBy)") | |
| public RankingResponse getContestRankings(Long contestId, String sortBy) { | |
| log.info("📊 대회 [{}] 랭킹 조회 (sortBy: {}) - 총자산 기준 DB 로드", contestId, sortBy); | |
| // sortBy 정규화: balance → totalAssets (하위 호환성) | |
| if ("balance".equalsIgnoreCase(sortBy)) { | |
| sortBy = "totalAssets"; | |
| } |
| jpa: | ||
| hibernate: | ||
| ddl-auto: update | ||
| ddl-auto: create |
|
@gemini-code-assist |
|
안녕하세요 @Yeeyahou님, 변경사항을 적용해주셔서 감사합니다. 새로운 변경사항에 대해 다시 한번 자세한 코드 리뷰를 원하시면 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
안녕하세요. 랭킹 시스템을 잔액 기준에서 총자산 기준으로 변경하는 대규모 리팩토링 작업을 리뷰했습니다. 실질적인 투자 순위를 더 정확하게 반영할 수 있는 훌륭한 개선입니다. 특히, 외부 API 호출을 위해 현재가를 일괄 수집하고 Rate Limiter를 적용한 점은 성능과 안정성 측면에서 좋은 접근입니다.
다만, 코드 리뷰 과정에서 몇 가지 심각한 성능 문제를 발견했습니다. 특히 getMyRank 메소드의 비효율적인 구조와 랭킹 계산 시 모든 보유 주식 정보를 불러오는 부분은 반드시 수정이 필요합니다. 그 외에도 몇 가지 개선점과 확인이 필요한 설정 변경 사항이 있으니 아래 상세 리뷰를 참고해 주시기 바랍니다.
| // StockDetailService의 getCurrentPrice 호출 (비동기 → 동기 변환) | ||
| BigDecimal price = stockDetailService.getCurrentPrice(stockCode) | ||
| .timeout(Duration.ofSeconds(3)) | ||
| .block(); |
There was a problem hiding this comment.
fetchPricesWithRateLimit 메소드 내의 for 루프에서 stockDetailService.getCurrentPrice(...).block()을 호출하고 있습니다. 이는 리액티브 스트림을 동기적으로 차단하는 방식으로, "리액티브 안티패턴"으로 알려져 있습니다. 현재 스케줄러나 웹 스레드에서 동작하므로 치명적인 문제는 아니지만, 리소스를 비효율적으로 사용할 수 있습니다.
향후 성능 개선을 위해 전체 가격 조회 로직을 Flux와 flatMap 등을 사용하여 완전히 비동기적으로 처리하는 것을 고려해볼 수 있습니다. 이렇게 하면 제한된 비율 내에서 API 호출을 병렬로 처리하여 전체 시간을 단축할 수 있습니다.
현재 로직이 정상 동작하므로 즉시 수정할 필요는 없지만, 장기적인 관점에서 리팩토링을 권장합니다.
| jpa: | ||
| hibernate: | ||
| ddl-auto: update | ||
| ddl-auto: create |
There was a problem hiding this comment.
🚀 Summary
랭킹 기준을 계좌 잔고에서 총 평가를 기준으로 바꿨습니다.
추가로 랭킹 요청시에 응답 바디도 그에 맞게 변경하였습니다.
✨ Description
🎲 Issue Number
close #127