밸런스 게임 임시저장 시 파일 중복 오류 해결 #775
Conversation
Walkthrough이 풀 리퀘스트에서는 Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/main/java/balancetalk/game/domain/TempGameOption.java (1)
44-44: 이미지 ID 필드에 대한 유효성 검사 및 문서화 고려이미지 ID 필드가 추가되었습니다. 다음 사항들을 고려해보시기 바랍니다:
- 이미지가 필수인지 선택적인지 명확히 하기 위한 문서화 추가
- null 값 허용 여부에 대한 명시적인 처리
다음과 같은 개선을 제안드립니다:
+ /** + * 게임 옵션의 이미지 ID + * null 허용: 이미지가 없는 옵션의 경우 + */ private Long imgId;src/main/java/balancetalk/game/dto/TempGameOptionDto.java (1)
Line range hint
26-43: fileId와 imgId의 관계에 대한 문서화가 필요합니다.두 필드의 관계와 용도를 명확히 하기 위해 메서드와 필드에 대한 주석 추가를 제안드립니다.
다음과 같이 주석을 추가하는 것을 고려해보세요:
public TempGameOption toEntity(FileRepository fileRepository) { + // fileId가 존재하는 경우 파일의 존재 여부를 검증 if (fileId != null && !fileRepository.existsById(fileId)) { throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE); } + // fileId를 imgId로 매핑하여 엔티티 생성 return TempGameOption.builder() .name(name) .imgId(fileId) .description(description) .optionType(optionType) .build(); }src/main/java/balancetalk/game/dto/TempGameSetDto.java (2)
38-40: isLoaded 필드가 잘 구현되었습니다!필드 추가와 검증이 적절하게 구현되었습니다. 스키마 설명도 명확합니다.
다음과 같이 스키마 설명을 더 자세히 작성하는 것을 고려해보세요:
-@Schema(description = "최근 임시저장된 밸런스 게임 불러오기 여부", example = "true") +@Schema(description = "최근 임시저장된 밸런스 게임 불러오기 여부 (true: 기존 데이터 로드, false: 새로운 요청)", example = "true")
64-66: 메소드 이름을 더 명확하게 개선하면 좋을 것 같습니다.현재 구현은 정확하지만, 메소드의 의도를 더 잘 표현할 수 있습니다.
다음과 같은 이름 변경을 고려해보세요:
-public boolean isNewRequest() { +public boolean isNewGameSetRequest() {이렇게 하면 메소드의 목적이 더 명확해질 것 같습니다.
src/main/java/balancetalk/global/exception/ErrorCode.java (1)
23-23: 에러 메시지가 명확하고 적절하게 배치되었습니다.BAD_REQUEST 상태 코드와 함께 파일 ID와 GameOption ID 간의 매핑 오류를 처리하는 것이 적절합니다. 다만, 메시지의 일관성을 위해 다음과 같이 수정하는 것을 고려해보세요:
- FILE_ID_GAME_OPTION_ID_MISMATCH(BAD_REQUEST, "File id에 매핑되는 GameOption Id가 없습니다."), + FILE_ID_GAME_OPTION_ID_MISMATCH(BAD_REQUEST, "파일 ID에 매핑되는 게임 옵션 ID가 존재하지 않습니다."),이유:
- 다른 에러 메시지들과 같이 영문 대신 한글 사용
- ID를 대문자로 표기하여 일관성 유지
- "없습니다" 대신 "존재하지 않습니다" 사용하여 다른 NOT_FOUND 메시지들과 일관성 유지
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/balancetalk/file/domain/repository/FileRepository.java(1 hunks)src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameOption.java(2 hunks)src/main/java/balancetalk/game/domain/TempGameSet.java(3 hunks)src/main/java/balancetalk/game/dto/TempGameOptionDto.java(1 hunks)src/main/java/balancetalk/game/dto/TempGameSetDto.java(3 hunks)src/main/java/balancetalk/global/exception/ErrorCode.java(1 hunks)
🔇 Additional comments (10)
src/main/java/balancetalk/file/domain/repository/FileRepository.java (2)
5-5: LGTM - List 임포트 추가
새로운 메서드의 매개변수 타입을 지원하기 위한 적절한 임포트입니다.
12-12: 배치 삭제 메서드 구현이 적절합니다
여러 파일을 한 번에 삭제할 수 있는 효율적인 메서드가 추가되었습니다. 기존의 단일 삭제 메서드와 일관된 네이밍 컨벤션을 따르고 있습니다.
이 메서드의 사용처를 확인하기 위해 다음 스크립트를 실행합니다:
✅ Verification successful
Let me check the actual implementation and usage of this batch delete method.
배치 삭제 메서드의 구현과 사용이 적절함을 확인했습니다
TempGameService에서 임시 게임 세트를 새로 저장할 때 기존 파일들을 효율적으로 일괄 삭제하는 용도로 적절하게 사용되고 있습니다. 메서드의 네이밍과 구현이 Spring Data JPA의 규칙을 잘 따르고 있으며, 비즈니스 로직에서도 의도된 대로 활용되고 있습니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 새로운 배치 삭제 메서드의 사용처를 확인합니다.
# 메서드 호출 위치 검색
rg -l "deleteByResourceIdInAndFileType" --type java
# TempGameService에서의 사용 패턴 확인
ast-grep --pattern 'deleteByResourceIdInAndFileType($$$)'
Length of output: 244
Script:
#!/bin/bash
# Check the implementation in TempGameService
rg "deleteByResourceIdInAndFileType" src/main/java/balancetalk/game/application/TempGameService.java -A 5 -B 5
# Check if there are any similar delete methods in FileRepository
ast-grep --pattern 'delete$_($$$)' src/main/java/balancetalk/file/domain/repository/FileRepository.java
Length of output: 757
src/main/java/balancetalk/game/domain/TempGameOption.java (1)
57-57: imgId 업데이트 로직 검증 필요
이미지 ID 업데이트 로직이 추가되었습니다. 기존 이미지 파일과의 관계 처리가 필요할 수 있습니다.
다음 스크립트로 이미지 ID 사용 패턴을 확인해보세요:
✅ Verification successful
Let me analyze the code further to understand the image handling patterns.
Let me check one more aspect of the file handling implementation.
Based on the analysis of the codebase, I can now provide the final response:
이미지 ID 업데이트는 안전합니다
TempGameOption의 imgId 업데이트는 FileRepository와 FileService를 통해 적절히 관리되고 있습니다. FileType.TEMP_GAME_OPTION으로 파일 관계가 명확히 정의되어 있어 기존 이미지 파일과의 관계가 올바르게 처리됩니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 이미지 ID 관련 처리 로직 검색
rg "imgId|fileId" -t java
Length of output: 6361
Script:
#!/bin/bash
# Check file and image related entity relationships
ast-grep --pattern 'class $_ {
$$$
private Long imgId;
$$$
}'
# Check file repository implementation
rg "class.*FileRepository" -A 10
Length of output: 1333
Script:
#!/bin/bash
# Check file entity and its relationships
rg "class.*File.*{" -A 15 --type java
# Check file type enum
rg "enum.*FileType" -A 10 --type java
Length of output: 1043
src/main/java/balancetalk/game/dto/TempGameOptionDto.java (2)
39-39: 필드 매핑이 올바르게 구현되었습니다.
fileId를 imgId로 매핑하는 변경사항이 적절하게 구현되었습니다. 파일 중복 문제를 해결하는데 도움이 될 것으로 보입니다.
Line range hint 26-43: 파일 처리 로직의 일관성 검증이 필요합니다.
파일 ID 처리 방식이 다른 컴포넌트들과 일관되게 구현되었는지 확인이 필요합니다.
✅ Verification successful
Based on the search results, I can now generate the final response as we have sufficient information about file ID handling across the codebase.
파일 ID 처리가 일관되게 구현되었습니다.
코드베이스 전반에 걸쳐 파일 ID 처리 방식이 일관성 있게 구현되어 있습니다:
- FileRepository를 통한 existsById 검증
- FileType에 따른 적절한 파일 처리
- imgId/fileId 명명 규칙 준수
- 파일 재배치 및 삭제 로직의 통일성
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 파일 ID 처리 관련 코드 검색
# fileId와 imgId 관련 코드 검색
rg -A 5 "fileId|imgId" --type java
# TempGameOption 관련 코드 검색
ast-grep --pattern 'class $_ {
$$$
private Long imgId;
$$$
}'
Length of output: 24366
src/main/java/balancetalk/game/dto/TempGameSetDto.java (2)
12-12: 새로운 import 구문이 적절히 추가되었습니다!
NotNull 검증과 Objects 유틸리티를 위한 import가 잘 추가되었습니다.
Also applies to: 15-15
56-62: 파일 ID 수집 로직이 효율적으로 구현되었습니다!
스트림 연산을 활용한 구현이 깔끔하며, null 처리도 적절합니다.
파일 ID 중복 처리가 실제로 잘 동작하는지 확인이 필요합니다:
✅ Verification successful
파일 ID 중복 처리가 정상적으로 구현되어 있음을 확인했습니다.
코드베이스 분석 결과, TempGameSet 클래스에서 fileToOptionMap을 통해 파일 ID 중복을 적절히 처리하고 있으며, TempGameService에서 파일 재배치 및 삭제된 파일 ID 처리도 올바르게 구현되어 있습니다.
TempGameSet.java에서newFileIds.contains(fileId)검사로 중복 확인TempGameService.java에서deletedFileIds,existingFileIds를 통한 파일 상태 관리TempGameOptionDto의 유효성 검사에서fileRepository.existsById(fileId)확인
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# TempGameOptionDto에서 fileId 필드 사용 패턴 확인
ast-grep --pattern 'class TempGameOptionDto {
$$$
private Long fileId;
$$$
}'
# fileId가 실제로 사용되는 패턴 검색
rg "fileId" -A 3
Length of output: 15931
src/main/java/balancetalk/game/domain/TempGameSet.java (2)
3-5: [임포트된 클래스 확인 완료]
새로 추가된 클래스들이 정확하게 임포트되었습니다.
Also applies to: 7-8, 24-24, 26-27
82-88: [NPE 가능성 검토 필요]
getAllFileIds() 메소드에서 tempGames 또는 game.getTempGameOptions()가 null일 경우 NullPointerException이 발생할 수 있습니다. 이들 객체가 null이 아님을 보장하거나 null 체크를 추가하는 것이 좋습니다.
src/main/java/balancetalk/game/application/TempGameService.java (1)
49-51: 효율적인 스트림 활용으로 TempGame 객체 생성
request.getTempGames()를 스트림으로 처리하여 TempGame 엔티티 리스트를 생성하신 부분이 깔끔합니다.
| public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) { | ||
| Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); | ||
| List<CreateTempGameRequest> tempGameRequests = request.getTempGames(); | ||
|
|
||
| for (int i = 0; i < tempGameRequests.size(); i++) { | ||
| CreateTempGameRequest gameRequest = tempGameRequests.get(i); | ||
|
|
||
| List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions(); | ||
| for (TempGameOptionDto optionDto : tempGameOptions) { | ||
| Long fileId = optionDto.getFileId(); | ||
|
|
||
| if (fileId != null && newFileIds.contains(fileId)) { | ||
| TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions() | ||
| .stream() | ||
| .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) | ||
| .findFirst() | ||
| .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)); | ||
| fileToOptionMap.put(fileId, tempGameOption.getId()); | ||
| } | ||
| } | ||
| } | ||
| return fileToOptionMap; | ||
| } |
There was a problem hiding this comment.
[리스트 크기 불일치로 인한 예외 처리 필요]
getFileToOptionMap 메소드에서 tempGameRequests와 tempGames의 크기가 다를 경우 tempGames.get(i)에서 IndexOutOfBoundsException이 발생할 수 있습니다. 두 리스트의 크기가 항상 같음을 보장하거나 크기 체크를 추가하여 예외를 방지해야 합니다.
다음과 같이 리스트 크기를 검증하는 코드를 추가하는 것을 제안합니다:
public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) {
+ if (tempGames.size() != request.getTempGames().size()) {
+ throw new BalanceTalkException(ErrorCode.INVALID_GAME_SIZE);
+ }
Map<Long, Long> fileToOptionMap = new LinkedHashMap<>();
List<CreateTempGameRequest> tempGameRequests = request.getTempGames();
// 나머지 코드...
}필요하다면 적절한 ErrorCode를 정의해야 합니다.
Committable suggestion skipped: line range outside the PR's diff.
| if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제 | ||
| List<Long> oldFileIds = existGame.getAllFileIds(); | ||
| fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION); | ||
| } | ||
|
|
||
| // 새롭게 불러온 경우, 파일만 재배치 (isLoaded: true) | ||
| relocateFiles(request, existGame); |
There was a problem hiding this comment.
relocateFiles() 메서드 호출 로직 확인 필요
request.isNewRequest()가 true인 경우에는 파일을 모두 삭제하고 있는데, 그 후에도 relocateFiles() 메서드를 호출하고 있습니다. 파일을 삭제한 후에 재배치하는 것이 의도된 동작인지 확인 부탁드립니다. 만약 파일 삭제 후에는 재배치가 필요 없다면, else 블록을 사용하여 호출을 제한하는 것이 좋겠습니다.
if (member.hasTempGameSet()) { // 기존 임시저장이 존재하는 경우
TempGameSet existGame = member.getTempGameSet();
if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제
List<Long> oldFileIds = existGame.getAllFileIds();
fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION);
+ } else {
// 새롭게 불러온 경우, 파일만 재배치 (isLoaded: true)
relocateFiles(request, existGame);
}
existGame.updateTempGameSet(request.getTitle(), newTempGames);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제 | |
| List<Long> oldFileIds = existGame.getAllFileIds(); | |
| fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION); | |
| } | |
| // 새롭게 불러온 경우, 파일만 재배치 (isLoaded: true) | |
| relocateFiles(request, existGame); | |
| if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제 | |
| List<Long> oldFileIds = existGame.getAllFileIds(); | |
| fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION); | |
| } else { | |
| // 새롭게 불러온 경우, 파일만 재배치 (isLoaded: true) | |
| relocateFiles(request, existGame); | |
| } |
There was a problem hiding this comment.
@jschoi-96, 알겠습니다. 의도된 동작임을 확인하였습니다.
✏️ Learnings added
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.731Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| Map<Long, Long> fileToOptionMap = tempGameSet.getFileToOptionMap(request, newFileIds); | ||
|
|
||
| for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) { | ||
| Long fileId = entry.getKey(); | ||
| Long tempGameOptionId = entry.getValue(); | ||
|
|
||
| File file = fileRepository.findById(fileId) | ||
| .orElseThrow(() -> new BalanceTalkException(ErrorCode.NOT_FOUND_FILE)); | ||
| fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
데이터베이스 호출 최적화로 성능 향상
for 루프 내에서 매번 fileRepository.findById()를 호출하면 다수의 데이터베이스 접근이 발생하여 성능 저하의 원인이 될 수 있습니다. 모든 fileId를 한 번에 조회하여 처리하면 효율성을 높일 수 있습니다.
다음과 같이 코드를 수정해 보세요:
private void relocateFiles(CreateTempGameSetRequest request, List<Long> newFileIds, TempGameSet tempGameSet) {
if (!newFileIds.isEmpty()) {
Map<Long, Long> fileToOptionMap = tempGameSet.getFileToOptionMap(request, newFileIds);
+ List<File> files = fileRepository.findAllById(newFileIds);
+ Map<Long, File> fileMap = files.stream().collect(Collectors.toMap(File::getId, Function.identity()));
for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) {
Long fileId = entry.getKey();
Long tempGameOptionId = entry.getValue();
- File file = fileRepository.findById(fileId)
- .orElseThrow(() -> new BalanceTalkException(ErrorCode.NOT_FOUND_FILE));
+ File file = fileMap.get(fileId);
+ if (file == null) {
+ throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE);
+ }
fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION);
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Map<Long, Long> fileToOptionMap = tempGameSet.getFileToOptionMap(request, newFileIds); | |
| for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) { | |
| Long fileId = entry.getKey(); | |
| Long tempGameOptionId = entry.getValue(); | |
| File file = fileRepository.findById(fileId) | |
| .orElseThrow(() -> new BalanceTalkException(ErrorCode.NOT_FOUND_FILE)); | |
| fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION); | |
| } | |
| } | |
| Map<Long, Long> fileToOptionMap = tempGameSet.getFileToOptionMap(request, newFileIds); | |
| List<File> files = fileRepository.findAllById(newFileIds); | |
| Map<Long, File> fileMap = files.stream().collect(Collectors.toMap(File::getId, Function.identity())); | |
| for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) { | |
| Long fileId = entry.getKey(); | |
| Long tempGameOptionId = entry.getValue(); | |
| File file = fileMap.get(fileId); | |
| if (file == null) { | |
| throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE); | |
| } | |
| fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION); | |
| } | |
| } |
There was a problem hiding this comment.
한번에 가져오면 쿼리를 한번 날리는데 map의 size가 20이라면 20번의 쿼리를 날리는 셈인가?
There was a problem hiding this comment.
@jschoi-96, fileRepository.findAllById(newFileIds)를 사용하면 데이터베이스에 한 번의 쿼리로 모든 파일을 가져옵니다. 반면에 findById()를 반복문 내에서 사용하면 각 파일마다 개별 쿼리가 발생하여 총 20번의 쿼리가 실행됩니다. 따라서 제안된 수정은 데이터베이스 호출 횟수를 줄여 성능 개선에 도움이 됩니다.
There was a problem hiding this comment.
@jschoi-96, 수정 내용을 반영해주셔서 감사합니다!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (6)
src/main/java/balancetalk/game/domain/TempGameOption.java (1)
44-44: 이미지 ID 필드에 대한 개선 제안
imgId필드에 대해 다음과 같은 개선을 제안드립니다:
- 데이터베이스 매핑을 명확히 하기 위해
@Column어노테이션 추가- 필드의 용도를 설명하는 주석 추가
다음과 같이 수정하는 것을 제안드립니다:
+ /** + * 임시 저장된 이미지의 식별자 + */ + @Column(name = "img_id") private Long imgId;src/main/java/balancetalk/game/dto/TempGameOptionDto.java (1)
Line range hint
31-34: 파일 유효성 검사 개선 제안현재 파일의 존재 여부만 확인하고 있습니다. 파일 중복 오류를 방지하기 위해 파일 타입 검증도 추가하는 것이 좋을 것 같습니다.
public TempGameOption toEntity(FileRepository fileRepository) { - if (fileId != null && !fileRepository.existsById(fileId)) { + if (fileId != null && (!fileRepository.existsById(fileId) || + !fileRepository.findById(fileId) + .map(file -> file.getFileType().isImageType()) + .orElse(false))) { throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE); }src/main/java/balancetalk/game/dto/TempGameSetDto.java (2)
38-40: Schema 설명을 더 자세히 작성해주세요.현재 설명이 단순히 "불러오기 여부"만 나타내고 있습니다. 이 필드가 임시저장된 게임의 중복 저장을 방지하는 데 어떤 역할을 하는지 명확히 설명하면 좋겠습니다.
예시:
-@Schema(description = "최근 임시저장된 밸런스 게임 불러오기 여부", example = "true") +@Schema(description = "최근 임시저장된 밸런스 게임 불러오기 여부 (true: 기존 데이터 불러오기, false: 새로운 임시저장)", example = "true")
64-66: isLoaded가 null일 경우를 고려해야 합니다.@NotNull 어노테이션이 있지만, 런타임에서의 안전성을 위해 null 체크를 추가하는 것이 좋습니다.
다음과 같이 수정하는 것을 제안드립니다:
public boolean isNewRequest() { - return !isLoaded; + return isLoaded == null || !isLoaded; }src/main/java/balancetalk/game/domain/TempGameSet.java (1)
84-90: 메서드 문서화 추가 필요이 메서드는 모든 임시 게임 옵션의 이미지 ID를 수집하는 중요한 역할을 합니다. 메서드의 목적과 반환값에 대한 JavaDoc 문서화를 추가하면 좋을 것 같습니다.
다음과 같은 문서화를 추가해보세요:
+ /** + * 모든 임시 게임 옵션의 이미지 ID를 수집합니다. + * @return null이 아닌 모든 이미지 ID 목록 + */ public List<Long> getAllFileIds() {src/main/java/balancetalk/global/exception/ErrorCode.java (1)
23-23: 에러 코드가 적절하게 추가되었습니다.새로운 에러 코드가 기존 패턴을 잘 따르고 있으며, 임시 저장 시 파일 중복 오류를 해결하는데 도움이 될 것으로 보입니다.
다음과 같이 코드 주석을 추가하여 이 에러가 발생하는 구체적인 시나리오를 문서화하는 것이 좋을 것 같습니다:
+ /** + * 임시 저장 시 파일 ID와 GameOption ID 간의 매핑이 일치하지 않을 때 발생하는 에러입니다. + * 주로 다음과 같은 경우에 발생할 수 있습니다: + * 1. 파일이 삭제되었거나 잘못된 파일 ID가 전달된 경우 + * 2. GameOption이 존재하지 않거나 잘못된 GameOption ID가 전달된 경우 + */ FILE_ID_GAME_OPTION_ID_MISMATCH(BAD_REQUEST, "File id에 매핑되는 GameOption Id가 없습니다."),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/main/java/balancetalk/file/domain/repository/FileRepository.java(1 hunks)src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameOption.java(2 hunks)src/main/java/balancetalk/game/domain/TempGameSet.java(3 hunks)src/main/java/balancetalk/game/dto/TempGameOptionDto.java(1 hunks)src/main/java/balancetalk/game/dto/TempGameSetDto.java(3 hunks)src/main/java/balancetalk/global/exception/ErrorCode.java(1 hunks)
🔇 Additional comments (7)
src/main/java/balancetalk/file/domain/repository/FileRepository.java (2)
5-5: LGTM - List import 추가
필요한 import가 적절하게 추가되었습니다.
12-12: 일괄 삭제 메서드 구현 검증 필요
새로운 메서드가 JPA 명명 규칙을 잘 따르고 있습니다. 하지만 일괄 삭제 작업이 안전하게 수행되는지 확인이 필요합니다.
다음 스크립트로 관련 테스트 코드와 사용 사례를 확인하겠습니다:
✅ Verification successful
일괄 삭제 메서드 안전성 검증 완료
deleteByResourceIdInAndFileType 메서드는 TempGameService의 트랜잭션 범위 내에서 안전하게 실행되고 있습니다. 다음과 같은 안전장치가 확인되었습니다:
@Transactional어노테이션으로 트랜잭션 처리가 보장됨- 특정 파일 타입(
TEMP_GAME_OPTION)으로 제한된 삭제 연산 - 삭제 후 새로운 파일 재배치 로직이 동일 트랜잭션에서 수행됨
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 일괄 삭제 메서드의 테스트 코드와 사용 사례 확인
# 테스트 코드 확인
echo "테스트 코드 검색 중..."
fd -e java -e kt test --exec rg -l "deleteByResourceIdInAndFileType"
# 실제 사용 사례 확인
echo "사용 사례 검색 중..."
rg "deleteByResourceIdInAndFileType" --type java --type kotlin -l
Length of output: 343
Script:
#!/bin/bash
# TempGameService에서의 사용 사례 확인
echo "TempGameService에서의 사용 사례:"
rg "deleteByResourceIdInAndFileType" src/main/java/balancetalk/game/application/TempGameService.java -A 5
# 트랜잭션 관련 설정 확인
echo "\n트랜잭션 설정 확인:"
rg "@Transactional" src/main/java/balancetalk/game/application/TempGameService.java -B 2 -A 10
Length of output: 1659
src/main/java/balancetalk/game/domain/TempGameOption.java (1)
57-57: 🛠️ Refactor suggestion
이미지 ID 업데이트 시 유효성 검사 추가 필요
imgId 업데이트 시 유효성 검사가 없어 잠재적인 문제가 발생할 수 있습니다.
이미지 ID의 유효성을 검증하는 로직이 다른 곳에 있는지 확인이 필요합니다:
서비스 레이어에서 유효성 검사가 없다면, 다음과 같은 방어 로직 추가를 제안드립니다:
public void updateTempGameOption(TempGameOption newTempGameOption) {
this.name = newTempGameOption.getName();
this.description = newTempGameOption.getDescription();
+ if (newTempGameOption.getImgId() != null && newTempGameOption.getImgId() <= 0) {
+ throw new IllegalArgumentException("유효하지 않은 이미지 ID입니다");
+ }
this.imgId = newTempGameOption.getImgId();
}src/main/java/balancetalk/game/dto/TempGameOptionDto.java (1)
39-39: imgId 매핑이 추가되었습니다.
이미지 ID를 엔티티에 매핑하는 변경사항이 적절해 보입니다. 다만 TempGameOption 엔티티에서 이 필드가 올바르게 처리되는지 확인이 필요합니다.
✅ Verification successful
imgId 필드가 올바르게 구현되어 있음을 확인했습니다.
TempGameOption 엔티티에서 다음 사항들이 확인되었습니다:
- imgId 필드가 적절하게 선언되어 있음
- updateTempGameOption() 메서드에서 imgId 값이 올바르게 업데이트됨
- 다른 관련 엔티티(GameOption)에서도 일관된 방식으로 imgId를 처리하고 있음
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# TempGameOption 엔티티에서 imgId 필드 처리 확인
ast-grep --pattern 'class TempGameOption {
$$$
private Long imgId;
$$$
}'
Length of output: 81
Script:
#!/bin/bash
# TempGameOption 엔티티 파일 찾기
fd TempGameOption.java
# TempGameOption 엔티티 내용 확인
fd TempGameOption.java -x cat {}
# imgId 관련 필드나 메서드 검색
rg "imgId" -A 2 -B 2
Length of output: 5841
src/main/java/balancetalk/game/dto/TempGameSetDto.java (1)
12-12: 새로운 import 구문이 적절히 추가되었습니다!
새로운 기능을 위해 필요한 import들이 잘 추가되었습니다.
Also applies to: 15-15
src/main/java/balancetalk/game/domain/TempGameSet.java (1)
3-8: 새로운 의존성이 적절히 추가되었습니다!
필요한 DTO 클래스들과 예외 처리를 위한 클래스들이 잘 임포트되었습니다.
Also applies to: 24-27
src/main/java/balancetalk/game/application/TempGameService.java (1)
79-89: 파일 재배치 로직이 정확하게 구현됨
relocateFiles 메서드에서 파일 재배치 로직이 올바르게 구현되었습니다.
| public List<Long> getAllFileIds() { | ||
| return tempGames.stream() | ||
| .flatMap(game -> game.getTempGameOptions().stream()) | ||
| .map(TempGameOptionDto::getFileId) | ||
| .filter(Objects::nonNull) | ||
| .toList(); | ||
| } |
There was a problem hiding this comment.
tempGames가 null일 경우에 대한 방어 로직이 필요합니다.
현재 구현은 tempGames가 null일 경우 NullPointerException이 발생할 수 있습니다.
다음과 같이 수정하는 것을 제안드립니다:
public List<Long> getAllFileIds() {
+ if (tempGames == null) {
+ return List.of();
+ }
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOptionDto::getFileId)
.filter(Objects::nonNull)
.toList();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public List<Long> getAllFileIds() { | |
| return tempGames.stream() | |
| .flatMap(game -> game.getTempGameOptions().stream()) | |
| .map(TempGameOptionDto::getFileId) | |
| .filter(Objects::nonNull) | |
| .toList(); | |
| } | |
| public List<Long> getAllFileIds() { | |
| if (tempGames == null) { | |
| return List.of(); | |
| } | |
| return tempGames.stream() | |
| .flatMap(game -> game.getTempGameOptions().stream()) | |
| .map(TempGameOptionDto::getFileId) | |
| .filter(Objects::nonNull) | |
| .toList(); | |
| } |
| public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) { | ||
| Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); | ||
| List<CreateTempGameRequest> tempGameRequests = request.getTempGames(); | ||
|
|
||
| for (int i = 0; i < tempGameRequests.size(); i++) { | ||
| CreateTempGameRequest gameRequest = tempGameRequests.get(i); | ||
|
|
||
| List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions(); | ||
| for (TempGameOptionDto optionDto : tempGameOptions) { | ||
| Long fileId = optionDto.getFileId(); | ||
|
|
||
| if (fileId != null && newFileIds.contains(fileId)) { | ||
| TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions() | ||
| .stream() | ||
| .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) | ||
| .findFirst() | ||
| .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)); | ||
| fileToOptionMap.put(fileId, tempGameOption.getId()); | ||
| } | ||
| } | ||
| } | ||
| return fileToOptionMap; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
코드 구조 개선 및 예외 처리 강화 필요
현재 구현에서 몇 가지 개선이 필요한 부분이 있습니다:
- 중첩된 루프 구조가 복잡성을 높이고 있습니다.
- Stream API를 활용하여 더 간결하게 구현할 수 있습니다.
- 예외 메시지가 구체적이지 않습니다.
다음과 같이 개선해보세요:
public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) {
- Map<Long, Long> fileToOptionMap = new LinkedHashMap<>();
- List<CreateTempGameRequest> tempGameRequests = request.getTempGames();
-
- for (int i = 0; i < tempGameRequests.size(); i++) {
- CreateTempGameRequest gameRequest = tempGameRequests.get(i);
-
- List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions();
- for (TempGameOptionDto optionDto : tempGameOptions) {
- Long fileId = optionDto.getFileId();
-
- if (fileId != null && newFileIds.contains(fileId)) {
- TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions()
- .stream()
- .filter(option -> option.getOptionType().equals(optionDto.getOptionType()))
- .findFirst()
- .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH));
- fileToOptionMap.put(fileId, tempGameOption.getId());
- }
- }
- }
- return fileToOptionMap;
+ return IntStream.range(0, request.getTempGames().size())
+ .boxed()
+ .flatMap(i -> request.getTempGames().get(i).getTempGameOptions().stream()
+ .filter(optionDto -> optionDto.getFileId() != null && newFileIds.contains(optionDto.getFileId()))
+ .map(optionDto -> Map.entry(optionDto.getFileId(),
+ tempGames.get(i).getTempGameOptions().stream()
+ .filter(option -> option.getOptionType().equals(optionDto.getOptionType()))
+ .findFirst()
+ .orElseThrow(() -> new BalanceTalkException(
+ ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH,
+ String.format("게임 %d의 옵션 타입 %s에 대한 매칭이 실패했습니다.",
+ i, optionDto.getOptionType())
+ )).getId())))
+ .collect(Collectors.toMap(
+ Map.Entry::getKey,
+ Map.Entry::getValue,
+ (existing, replacement) -> existing,
+ LinkedHashMap::new
+ ));
}추가로 메서드 문서화도 필요합니다:
+ /**
+ * 파일 ID와 옵션 ID 간의 매핑을 생성합니다.
+ * @param request 임시 게임 세트 생성 요청
+ * @param newFileIds 새로운 파일 ID 목록
+ * @return 파일 ID를 키로, 옵션 ID를 값으로 하는 매핑
+ * @throws BalanceTalkException 파일 ID와 게임 옵션 ID가 일치하지 않는 경우
+ */
public Map<Long, Long> getFileToOptionMap(...)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) { | |
| Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); | |
| List<CreateTempGameRequest> tempGameRequests = request.getTempGames(); | |
| for (int i = 0; i < tempGameRequests.size(); i++) { | |
| CreateTempGameRequest gameRequest = tempGameRequests.get(i); | |
| List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions(); | |
| for (TempGameOptionDto optionDto : tempGameOptions) { | |
| Long fileId = optionDto.getFileId(); | |
| if (fileId != null && newFileIds.contains(fileId)) { | |
| TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions() | |
| .stream() | |
| .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) | |
| .findFirst() | |
| .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)); | |
| fileToOptionMap.put(fileId, tempGameOption.getId()); | |
| } | |
| } | |
| } | |
| return fileToOptionMap; | |
| } | |
| /** | |
| * 파일 ID와 옵션 ID 간의 매핑을 생성합니다. | |
| * @param request 임시 게임 세트 생성 요청 | |
| * @param newFileIds 새로운 파일 ID 목록 | |
| * @return 파일 ID를 키로, 옵션 ID를 값으로 하는 매핑 | |
| * @throws BalanceTalkException 파일 ID와 게임 옵션 ID가 일치하지 않는 경우 | |
| */ | |
| public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) { | |
| return IntStream.range(0, request.getTempGames().size()) | |
| .boxed() | |
| .flatMap(i -> request.getTempGames().get(i).getTempGameOptions().stream() | |
| .filter(optionDto -> optionDto.getFileId() != null && newFileIds.contains(optionDto.getFileId())) | |
| .map(optionDto -> Map.entry(optionDto.getFileId(), | |
| tempGames.get(i).getTempGameOptions().stream() | |
| .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) | |
| .findFirst() | |
| .orElseThrow(() -> new BalanceTalkException( | |
| ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH, | |
| String.format("게임 %d의 옵션 타입 %s에 대한 매칭이 실패했습니다.", | |
| i, optionDto.getOptionType()) | |
| )).getId()))) | |
| .collect(Collectors.toMap( | |
| Map.Entry::getKey, | |
| Map.Entry::getValue, | |
| (existing, replacement) -> existing, | |
| LinkedHashMap::new | |
| )); | |
| } |
There was a problem hiding this comment.
@jschoi-96 이해하셨습니다. 기존 코드가 더 이해하기 쉽다고 느끼신다면 그대로 유지하시면 됩니다.
(∩˃o˂∩)♡
✏️ Learnings added
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/domain/TempGameSet.java:92-114
Timestamp: 2024-11-29T18:44:42.615Z
Learning: `TempGameSet.java` 파일의 `getFileToOptionMap` 메서드에서 Stream API를 사용한 복잡한 리팩토링은 코드의 가독성을 떨어뜨릴 수 있으므로 피해야 합니다. 기존의 for-loop 구조를 선호합니다.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| if (request.isNewRequest()) { // 새롭게 임시저장 하는 경우, 파일 모두 삭제 | ||
| List<Long> oldFileIds = existGame.getAllFileIds(); | ||
| fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION); | ||
| } |
There was a problem hiding this comment.
파일 삭제 시 빈 파일 ID 목록 처리 필요
deleteByResourceIdInAndFileType 메서드를 호출할 때, oldFileIds가 비어 있을 경우 예외가 발생할 수 있으므로, 파일 ID 목록이 비어 있지 않은지 확인하는 로직이 필요합니다.
수정 제안:
if (!oldFileIds.isEmpty()) {
fileRepository.deleteByResourceIdInAndFileType(oldFileIds, TEMP_GAME_OPTION);
}There was a problem hiding this comment.
@jschoi-96 수정해주셔서 감사합니다!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/balancetalk/game/domain/TempGameSet.java (1)
96-112: 성능 최적화: 중첩 루프 개선 제안현재 구현은 이중 루프와 스트림 연산을 함께 사용하고 있어 성능에 영향을 줄 수 있습니다. 특히 데이터가 많은 경우 성능 저하가 발생할 수 있습니다.
다음과 같은 최적화를 고려해보세요:
public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) { Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); List<CreateTempGameRequest> tempGameRequests = request.getTempGames(); + + // 옵션 타입에 대한 매핑을 미리 생성 + for (int i = 0; i < tempGames.size(); i++) { + TempGame game = tempGames.get(i); + Map<OptionType, TempGameOption> optionTypeMap = game.getTempGameOptions().stream() + .collect(Collectors.toMap( + TempGameOption::getOptionType, + option -> option + )); + + CreateTempGameRequest gameRequest = tempGameRequests.get(i); + gameRequest.getTempGameOptions().stream() + .filter(optionDto -> optionDto.getFileId() != null && newFileIds.contains(optionDto.getFileId())) + .forEach(optionDto -> { + TempGameOption option = optionTypeMap.get(optionDto.getOptionType()); + if (option == null) { + throw new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH); + } + fileToOptionMap.put(optionDto.getFileId(), option.getId()); + }); + } return fileToOptionMap; }src/main/java/balancetalk/game/application/TempGameService.java (1)
114-128: 트랜잭션 관리 개선 필요파일 삭제는 중요한 작업이므로, 이 메서드에
@Transactional어노테이션을 추가하여 트랜잭션 안전성을 보장하는 것이 좋습니다.+ @Transactional private List<Long> deleteFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameSet.java(3 hunks)
🔇 Additional comments (6)
src/main/java/balancetalk/game/domain/TempGameSet.java (2)
84-90: 구현이 깔끔하고 효율적입니다!
스트림 연산을 적절히 활용하여 이미지 ID를 수집하고, null 값도 올바르게 처리하고 있습니다.
92-114:
크리티컬: tempGames와 tempGameRequests 크기 검증이 필요합니다
이전 리뷰에서 지적된 문제가 아직 해결되지 않았습니다. tempGames.get(i)에서 IndexOutOfBoundsException이 발생할 수 있습니다.
다음과 같이 수정을 제안합니다:
public Map<Long, Long> getFileToOptionMap(CreateTempGameSetRequest request, List<Long> newFileIds) {
+ if (tempGames.size() != request.getTempGames().size()) {
+ throw new BalanceTalkException(ErrorCode.INVALID_GAME_SIZE);
+ }
Map<Long, Long> fileToOptionMap = new LinkedHashMap<>();
// ... 나머지 코드src/main/java/balancetalk/game/application/TempGameService.java (4)
56-62: relocateFiles() 메서드 호출 로직 확인 필요
파일을 모두 삭제한 후에도 relocateFiles()를 호출하고 있습니다. 이는 불필요한 작업을 수행할 수 있으므로, else 블록을 사용하여 호출을 제한하는 것이 좋습니다.
106-112: 구현이 깔끔하고 효율적입니다
중복 파일 ID를 필터링하는 로직이 스트림을 활용하여 명확하게 구현되었습니다.
91-104: 🛠️ Refactor suggestion
데이터베이스 호출 최적화 필요
for 루프 내에서 개별적으로 fileRepository.findById()를 호출하는 것은 성능상 비효율적입니다.
예외 처리 강화 필요
파일 재배치 과정에서 발생할 수 있는 I/O 예외에 대한 처리가 필요합니다. fileHandler.relocateFile() 호출 시 발생할 수 있는 예외를 적절히 처리하여 시스템의 안정성을 높여주세요.
49-51: 스트림 연산 전 유효성 검증 추가 필요
tempGames 리스트의 요소들이 null이 아님을 보장하기 위해 유효성 검증을 추가하는 것이 좋습니다.
…o feat/770-temp-save-isloaded
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/java/balancetalk/game/application/TempGameService.java (2)
84-115: 파일 재배치 프로세스에 로깅 추가 권장파일 재배치 과정의 추적성을 높이기 위해 주요 지점에 로깅을 추가하면 좋겠습니다.
private void relocateFiles(CreateTempGameSetRequest request, List<Long> newFileIds, TempGameSet tempGameSet) { if (!newFileIds.isEmpty()) { + log.debug("파일 재배치 시작: {} 개의 파일", newFileIds.size()); Map<Long, Long> fileToOptionMap = tempGameSet.getFileToOptionMap(request, newFileIds); List<File> files = fileRepository.findAllById(newFileIds); + log.debug("조회된 파일 수: {}", files.size()); Map<Long, File> fileMap = files.stream() .collect(Collectors.toMap(File::getId, file -> file)); for (Map.Entry<Long, Long> entry : fileToOptionMap.entrySet()) { Long fileId = entry.getKey(); Long tempGameOptionId = entry.getValue(); File file = fileMap.get(fileId); if (file == null) { + log.error("파일을 찾을 수 없음: fileId={}", fileId); throw new BalanceTalkException(ErrorCode.NOT_FOUND_FILE); } fileHandler.relocateFile(file, tempGameOptionId, TEMP_GAME_OPTION); } + log.debug("파일 재배치 완료"); } }
125-139: 파일 삭제 작업의 트랜잭션 처리 개선 필요파일 삭제 작업이 실패할 경우를 대비한 트랜잭션 처리가 필요합니다. 또한, 대량의 파일 삭제 시 발생할 수 있는 성능 이슈를 고려해야 합니다.
+ @Transactional private List<Long> deleteFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) { List<Long> existingFileIds = tempGameSet.getAllFileIds(); + log.info("파일 삭제 프로세스 시작: {} 개의 기존 파일", existingFileIds.size()); List<Long> newFileIds = request.getAllFileIds(); List<Long> filesIdsToDelete = existingFileIds.stream() .filter(existingFileId -> !newFileIds.contains(existingFileId)) .toList(); if (!filesIdsToDelete.isEmpty()) { + log.info("삭제 대상 파일 수: {}", filesIdsToDelete.size()); List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete); fileHandler.deleteFiles(filesToDelete); } return filesIdsToDelete; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/resources/config(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/config
🧰 Additional context used
📓 Learnings (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.945Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
🔇 Additional comments (2)
src/main/java/balancetalk/game/application/TempGameService.java (2)
26-33: 로깅 기능 추가 승인
Map 유틸리티와 Log4j2를 활용한 로깅 기능 추가는 적절해 보입니다.
117-123: 중복 파일 ID 필터링 로직 승인
스트림 API를 효율적으로 활용하여 중복 파일 ID를 필터링하는 구현이 깔끔합니다.
| List<TempGame> newTempGames = request.getTempGames().stream() | ||
| .map(game -> game.toEntity(fileRepository)) | ||
| .toList(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
스트림 처리 시 예외 처리 보완 필요
toEntity 메서드 호출 시 발생할 수 있는 예외에 대한 처리가 필요합니다. 파일 저장소 접근 실패나 잘못된 데이터로 인한 예외를 적절히 처리하면 좋겠습니다.
List<TempGame> newTempGames = request.getTempGames().stream()
- .map(game -> game.toEntity(fileRepository))
+ .map(game -> {
+ try {
+ return game.toEntity(fileRepository);
+ } catch (Exception e) {
+ log.error("임시 게임 엔티티 생성 중 오류 발생: {}", e.getMessage());
+ throw new BalanceTalkException(ErrorCode.TEMP_GAME_CREATION_ERROR);
+ }
+ })
.toList();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
117-139: 메소드 문서화 및 성능 최적화 검토 필요복잡한 매핑 로직에 대한 문서화가 있으면 좋을 것 같습니다. 또한, 대용량 데이터 처리 시 성능 이슈가 발생할 수 있으니 검토가 필요합니다.
+/** + * 파일 ID와 게임 옵션 ID 간의 매핑을 생성합니다. + * @param tempGames 임시 게임 목록 + * @param request 요청 데이터 + * @param newFileIds 새로운 파일 ID 목록 + * @return 파일 ID를 키로, 게임 옵션 ID를 값으로 하는 매핑 + */ private Map<Long, Long> getFileToOptionMap(List<TempGame> tempGames, CreateTempGameSetRequest request, List<Long> newFileIds) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameSet.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/balancetalk/game/domain/TempGameSet.java
🧰 Additional context used
📓 Learnings (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.945Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
🔇 Additional comments (5)
src/main/java/balancetalk/game/application/TempGameService.java (5)
58-63: 구현이 PR 목적에 잘 부합합니다
파일 중복을 방지하기 위한 삭제 로직이 적절히 구현되었고, 빈 리스트 처리도 잘 되어 있습니다.
83-93: 메소드 구조가 잘 설계되었습니다
파일 재배치 로직이 작은 단위의 메소드로 잘 분리되어 있어 코드의 가독성과 유지보수성이 좋습니다.
95-115: 데이터베이스 최적화가 잘 적용되었습니다
findAllById를 사용하여 데이터베이스 호출을 최적화했고, 누락된 파일에 대한 예외 처리도 적절합니다.
153-167: 파일 삭제 로직이 효율적으로 구현되었습니다
데이터베이스 조회와 파일 시스템 작업이 잘 분리되어 있고, 배치 작업을 통해 효율적으로 구현되었습니다.
51-53: 🛠️ Refactor suggestion
스트림 작업에서 예외 처리 개선 필요
스트림 작업 중 발생할 수 있는 예외를 적절히 처리하고 로깅을 추가하면 디버깅이 용이해질 것 같습니다.
List<TempGame> newTempGames = request.getTempGames().stream()
- .map(game -> game.toEntity(fileRepository))
+ .map(game -> {
+ try {
+ return game.toEntity(fileRepository);
+ } catch (Exception e) {
+ log.error("임시 게임 엔티티 생성 중 오류 발생: {}", e.getMessage());
+ throw new BalanceTalkException(ErrorCode.TEMP_GAME_CREATION_ERROR);
+ }
+ })
.toList();Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/java/balancetalk/game/application/TempGameService.java (2)
121-147: 성능 최적화 및 에러 메시지 개선 제안중첩된 루프와 스트림 연산이 큰 데이터셋에서 성능 저하를 일으킬 수 있습니다. 또한, 에러 메시지가 더 구체적이면 좋겠습니다.
private Map<Long, Long> getFileToOptionMap( List<TempGame> tempGames, CreateTempGameSetRequest request, List<Long> newFileIds ) { Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); + if (tempGames.size() != request.getTempGames().size()) { + throw new BalanceTalkException(ErrorCode.INVALID_GAME_COUNT); + } + // 옵션 타입별 매핑을 미리 생성하여 조회 성능 개선 + Map<Integer, Map<String, TempGameOption>> gameOptionsMap = new HashMap<>(); + for (int i = 0; i < tempGames.size(); i++) { + gameOptionsMap.put(i, tempGames.get(i).getTempGameOptions().stream() + .collect(Collectors.toMap( + option -> option.getOptionType(), + option -> option + ))); + }
161-175: 로깅 추가 제안파일 삭제 작업에 대한 로깅을 추가하면 운영 시 문제 추적이 용이할 것 같습니다.
private List<Long> deleteFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) { List<Long> existingFileIds = tempGameSet.getAllFileIds(); List<Long> newFileIds = request.getAllFileIds(); List<Long> filesIdsToDelete = existingFileIds.stream() .filter(existingFileId -> !newFileIds.contains(existingFileId)) .toList(); if (!filesIdsToDelete.isEmpty()) { + log.info("임시 게임 파일 삭제 시작. 삭제할 파일 수: {}", filesIdsToDelete.size()); List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete); fileHandler.deleteFiles(filesToDelete); + log.info("임시 게임 파일 삭제 완료. 삭제된 파일 ID: {}", filesIdsToDelete); } return filesIdsToDelete; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.945Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
🔇 Additional comments (4)
src/main/java/balancetalk/game/application/TempGameService.java (4)
58-63: 파일 삭제 로직이 적절히 구현되었습니다
빈 파일 ID 목록에 대한 검사가 포함되어 있어 안전하게 구현되었습니다.
95-119: 데이터베이스 접근 최적화가 잘 구현되었습니다
findAllById를 사용하여 데이터베이스 쿼리를 최적화하고, 적절한 null 체크도 포함되어 있습니다.
149-159: 중복 파일 ID 필터링이 효율적으로 구현되었습니다
스트림 연산을 활용하여 가독성 있게 구현되었으며, 중복 제거 로직이 명확합니다.
51-53: 🛠️ Refactor suggestion
스트림 처리 시 예외 처리 보완 필요
파일 저장소 접근 실패나 잘못된 데이터로 인한 예외를 명시적으로 처리하면 좋겠습니다.
List<TempGame> newTempGames = request.getTempGames().stream()
- .map(game -> game.toEntity(fileRepository))
+ .map(game -> {
+ try {
+ return game.toEntity(fileRepository);
+ } catch (Exception e) {
+ throw new BalanceTalkException(ErrorCode.FILE_PROCESSING_ERROR, e);
+ }
+ })
.toList();Likely invalid or redundant comment.
| List<TempGame> newTempGames = tempGames.stream() | ||
| .map(game -> game.toEntity(fileRepository)) | ||
| .toList(); | ||
| List<TempGame> newTempGames = request.getTempGames().stream() | ||
| .map(game -> game.toEntity(fileRepository)) | ||
| .toList(); |
There was a problem hiding this comment.
왜 수정했는지 알 수 있을까요??
위에 request.getTempGamse()를 이미 호출했는데 또 호출하는 방식으로 변경돼서 궁금합니다
| @@ -41,6 +41,8 @@ public class TempGameOption { | |||
| @Enumerated(value = EnumType.STRING) | |||
| private VoteOption optionType; | |||
|
|
|||
| private Long imgId; | |||
|
|
|||
There was a problem hiding this comment.
GameOption과 일관성을 위해 imgUrl도 추가 부탁드립니다
There was a problem hiding this comment.
처음에 Game 엔티티에서 imgUrl이 불필요한 값이다보니 TempGame에도 불필요하다고 생각해서 제외했습니다.
근데 다시 생각해보니 임시게임 불러오기의 응답값을 밸런스 게임 생성할 때 쓰기 때문에 추가하는게 맞는것 같네요
| @@ -37,6 +36,7 @@ public TempGameOption toEntity(FileRepository fileRepository) { | |||
|
|
|||
| return TempGameOption.builder() | |||
| .name(name) | |||
| .imgId(fileId) | |||
There was a problem hiding this comment.
응답할 때도 TempGameOptionDto를 사용하는 것 같은데, imgUrl도 추가해야 하는거 아닌가요?
| CreateTempGameSetRequest request, | ||
| List<Long> newFileIds, | ||
| TempGameSet tempGameSet | ||
| ) { |
There was a problem hiding this comment.
sonarqube 라인 120이 넘어서 저렇게 설정했는데
)
{
형태가 되어야 할까요??
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
121-147: 파일-옵션 매핑 로직 개선 제안현재 중첩된 반복문 구조가 있어 성능에 영향을 줄 수 있습니다.
다음과 같이 스트림을 활용하여 개선할 수 있습니다:
private Map<Long, Long> getFileToOptionMap( List<TempGame> tempGames, CreateTempGameSetRequest request, List<Long> newFileIds ) { - Map<Long, Long> fileToOptionMap = new LinkedHashMap<>(); - List<CreateTempGameRequest> tempGameRequests = request.getTempGames(); - - for (int i = 0; i < tempGameRequests.size(); i++) { - CreateTempGameRequest gameRequest = tempGameRequests.get(i); - - List<TempGameOptionDto> tempGameOptions = gameRequest.getTempGameOptions(); - for (TempGameOptionDto optionDto : tempGameOptions) { - Long fileId = optionDto.getFileId(); - - if (fileId != null && newFileIds.contains(fileId)) { - TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions() - .stream() - .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) - .findFirst() - .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)); - fileToOptionMap.put(fileId, tempGameOption.getId()); - } - } - } - return fileToOptionMap; + return IntStream.range(0, request.getTempGames().size()) + .boxed() + .flatMap(i -> request.getTempGames().get(i).getTempGameOptions().stream() + .filter(optionDto -> optionDto.getFileId() != null && newFileIds.contains(optionDto.getFileId())) + .map(optionDto -> Map.entry(optionDto.getFileId(), + tempGames.get(i).getTempGameOptions().stream() + .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) + .findFirst() + .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)) + .getId() + ))) + .collect(Collectors.toMap( + Map.Entry::getKey, + Map.Entry::getValue, + (existing, replacement) -> existing, + LinkedHashMap::new + )); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameOption.java(2 hunks)src/main/java/balancetalk/game/dto/TempGameOptionDto.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/balancetalk/game/domain/TempGameOption.java
🧰 Additional context used
📓 Learnings (1)
src/main/java/balancetalk/game/application/TempGameService.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.945Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
🔇 Additional comments (6)
src/main/java/balancetalk/game/dto/TempGameOptionDto.java (2)
24-31: 필드 추가가 적절히 구현되었습니다.
imgUrl 필드가 @JsonInclude(JsonInclude.Include.NON_NULL)로 적절히 처리되어 있고, Swagger 문서화도 잘 되어 있습니다.
46-47: 엔티티 매핑이 올바르게 구현되었습니다.
fileId와 imgUrl이 각각 imgId와 imgUrl로 정확하게 매핑되어 있습니다.
src/main/java/balancetalk/game/application/TempGameService.java (4)
52-67: 스트림 처리와 파일 관리 로직이 개선되었습니다.
파일 삭제와 재배치 로직이 명확하게 분리되어 있고, 빈 파일 ID 목록에 대한 처리도 추가되었습니다.
83-93: 파일 재배치 메서드의 책임이 명확합니다.
각 단계(삭제, 중복 제거, 재배치)가 잘 분리되어 있어 코드의 가독성과 유지보수성이 향상되었습니다.
95-119: 데이터베이스 조회 최적화가 잘 구현되었습니다.
findAllById를 사용하여 데이터베이스 호출을 최적화했고, 파일 매핑과 재배치 로직이 깔끔하게 구현되었습니다.
149-175: 파일 ID 관리 로직이 견고하게 구현되었습니다.
중복 파일 처리와 불필요한 파일 삭제 로직이 명확하게 구현되어 있습니다. 특히 deleteFiles 메서드에서 빈 리스트 체크를 통해 불필요한 데이터베이스 호출을 방지하고 있습니다.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
src/main/java/balancetalk/game/domain/TempGameSet.java (1)
67-73: getAllFileIds 메서드 구현 개선 제안스트림 API를 사용한 구현이 깔끔하지만, 성능과 가독성을 위해 몇 가지 개선사항을 제안드립니다:
public List<Long> getAllFileIds() { - return tempGames.stream() - .flatMap(game -> game.getTempGameOptions().stream()) - .map(TempGameOption::getImgId) - .filter(Objects::nonNull) - .toList(); + List<Long> fileIds = new ArrayList<>(); + for (TempGame game : tempGames) { + for (TempGameOption option : game.getTempGameOptions()) { + Long imgId = option.getImgId(); + if (imgId != null) { + fileIds.add(imgId); + } + } + } + return fileIds;개선 사항:
- 기존 코드보다 더 직관적인 구조
- 중첩 반복문을 사용하여 가독성 향상
- 이전 리뷰 피드백 반영 (스트림 API 대신 전통적인 반복문 사용)
src/main/java/balancetalk/game/application/TempGameService.java (2)
132-137: 스트림 체인 가독성 개선 필요긴 스트림 체인은 가독성을 해치고 디버깅을 어렵게 만듭니다.
다음과 같이 분리하여 작성하는 것을 제안합니다:
- TempGameOption tempGameOption = tempGames.get(i).getTempGameOptions() - .stream() - .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) - .findFirst() - .orElseThrow(() -> new BalanceTalkException(ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH)); + List<TempGameOption> gameOptions = tempGames.get(i).getTempGameOptions(); + TempGameOption tempGameOption = gameOptions.stream() + .filter(option -> option.getOptionType().equals(optionDto.getOptionType())) + .findFirst() + .orElseThrow(() -> new BalanceTalkException( + ErrorCode.FILE_ID_GAME_OPTION_ID_MISMATCH, + String.format("옵션 타입 %s에 해당하는 게임 옵션을 찾을 수 없습니다.", optionDto.getOptionType()) + ));
78-88: 메서드 문서화 필요파일 재배치 로직의 복잡성을 고려할 때, 메서드의 동작과 매개변수에 대한 문서화가 필요합니다.
다음과 같이 JavaDoc 추가를 제안합니다:
+/** + * 임시 게임의 파일들을 재배치합니다. + * 1. 삭제된 파일 처리 + * 2. 새로운 파일 식별 + * 3. 파일 재배치 수행 + * + * @param request 임시 게임 생성 요청 정보 + * @param tempGameSet 현재 임시 게임 세트 + */ private void relocateFiles(CreateTempGameSetRequest request, TempGameSet tempGameSet) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/main/java/balancetalk/game/application/TempGameService.java(4 hunks)src/main/java/balancetalk/game/domain/TempGameSet.java(2 hunks)src/main/java/balancetalk/game/dto/TempGameSetDto.java(2 hunks)
🧰 Additional context used
📓 Learnings (3)
src/main/java/balancetalk/game/domain/TempGameSet.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/domain/TempGameSet.java:92-114
Timestamp: 2024-11-29T18:44:42.778Z
Learning: `TempGameSet.java` 파일의 `getFileToOptionMap` 메서드에서 Stream API를 사용한 복잡한 리팩토링은 코드의 가독성을 떨어뜨릴 수 있으므로 피해야 합니다. 기존의 for-loop 구조를 선호합니다.
src/main/java/balancetalk/game/application/TempGameService.java (1)
Learnt from: jschoi-96
PR: JECT-Study/PICK-O-Server#775
File: src/main/java/balancetalk/game/application/TempGameService.java:56-0
Timestamp: 2024-11-29T18:43:32.945Z
Learning: `TempGameService.java` 파일에서 `request.isNewRequest()`가 `true`인 경우, 파일 삭제 후에도 `relocateFiles(request, existGame);`를 호출하는 것은 의도된 동작입니다.
src/main/java/balancetalk/game/dto/TempGameSetDto.java (1)
Learnt from: Hanjaemo
PR: JECT-Study/PICK-O-Server#713
File: src/main/java/balancetalk/game/dto/TempGameSetDto.java:31-38
Timestamp: 2024-11-20T13:05:40.623Z
Learning: CreateTempGameSetRequest DTO는 `title`과 `mainTag` 필드를 가지고 있으므로, 엔티티로 변환할 때 `toEntity` 메서드에서 외부에서 주입하지 말고 DTO의 필드를 직접 사용해야 합니다.
🔇 Additional comments (6)
src/main/java/balancetalk/game/domain/TempGameSet.java (2)
19-19: Objects 유틸리티 클래스 import 추가 적절
새로운 getAllFileIds 메서드에서 null 체크를 위해 필요한 import문이 적절히 추가되었습니다.
58-65: updateTempGameSet 메서드의 매개변수 간소화 검증 필요
mainTag와 subTag 매개변수가 제거되었습니다. 이는 코드를 단순화시키지만, 몇 가지 고려사항이 있습니다.
다음 스크립트를 실행하여 이 변경이 다른 부분에 영향을 미치지 않는지 확인하겠습니다:
✅ Verification successful
Let me gather more information about the usage of updateTempGameSet method.
Let me check one more thing to verify the request object structure and usage.
매개변수 간소화가 안전하게 적용되었음을 확인했습니다
코드 검증 결과, updateTempGameSet 메서드에서 mainTag와 subTag 매개변수를 제거한 변경이 안전합니다. 다음과 같은 이유로 이 변경이 적절합니다:
TempGameService에서updateTempGameSet호출 시 title과 newTempGames만 사용하고 있습니다.- 태그 관련 필드들은 다른 클래스들에서 독립적으로 관리되고 있습니다.
- 메서드의 책임이 더 명확해졌으며, 단일 책임 원칙에 더 부합합니다.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# mainTag와 subTag 사용 여부 확인
rg -l "mainTag|subTag" --type java
# updateTempGameSet 메서드 호출 패턴 확인
ast-grep --pattern 'updateTempGameSet($$$)'
Length of output: 630
Script:
#!/bin/bash
# Check for updateTempGameSet method calls with context
rg "updateTempGameSet" -A 3 -B 3 --type java
# Check TempGameSet class implementation
ast-grep --pattern 'class TempGameSet {
$$$
}'
# Check if there are any test files using updateTempGameSet
fd -e java -e Test.java -x rg "updateTempGameSet" {}
Length of output: 1596
Script:
#!/bin/bash
# Check the request object structure and usage
rg "getTitle|mainTag|subTag" -A 3 -B 3 src/main/java/balancetalk/game/application/TempGameService.java
# Check TempGameSet constructor and other related methods
ast-grep --pattern 'class TempGameSet {
$$$
TempGameSet($$$) {
$$$
}
$$$
}'
Length of output: 400
src/main/java/balancetalk/game/dto/TempGameSetDto.java (3)
11-14: 필드 추가와 임포트가 적절히 구현되었습니다!
isLoaded 필드가 @NotNull 어노테이션으로 적절히 검증되어 있으며, Swagger 문서화도 명확하게 되어 있습니다.
Also applies to: 30-32
54-56: 메소드 구현이 적절합니다!
isLoaded 필드가 @NotNull로 검증되어 있어 안전하게 구현되었습니다.
46-52:
tempGames의 null 체크가 필요합니다.
tempGames가 null일 경우 NullPointerException이 발생할 수 있습니다.
다음과 같이 수정하는 것을 제안드립니다:
public List<Long> getAllFileIds() {
+ if (tempGames == null) {
+ return List.of();
+ }
return tempGames.stream()
.flatMap(game -> game.getTempGameOptions().stream())
.map(TempGameOptionDto::getFileId)
.filter(Objects::nonNull)
.toList();
}src/main/java/balancetalk/game/application/TempGameService.java (1)
47-48: 🛠️ Refactor suggestion
스트림 처리 시 예외 처리 보완 필요
toEntity 메서드 호출 시 발생할 수 있는 예외를 적절히 처리하면 좋겠습니다.
다음과 같이 수정을 제안합니다:
List<TempGame> newTempGames = tempGames.stream()
- .map(game -> game.toEntity(fileRepository))
+ .map(game -> {
+ try {
+ return game.toEntity(fileRepository);
+ } catch (Exception e) {
+ throw new BalanceTalkException(ErrorCode.TEMP_GAME_CREATION_ERROR, e);
+ }
+ })
.toList();Likely invalid or redundant comment.
| public TempGameSet toEntity(Member member) { | ||
| if (tempGames == null || tempGames.size() < GAME_SIZE) { | ||
| throw new BalanceTalkException(ErrorCode.BALANCE_GAME_SIZE_TEN); | ||
| } | ||
| return TempGameSet.builder() | ||
| .title(title) | ||
| .mainTag(mainTag) | ||
| .subTag(subTag) | ||
| .member(member) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
title 필드의 활용 방식 개선이 필요합니다.
이전 PR #713의 피드백을 반영하여, title 필드가 null일 경우에 대한 검증이 필요합니다.
다음과 같이 수정하는 것을 제안드립니다:
public TempGameSet toEntity(Member member) {
if (tempGames == null || tempGames.size() < GAME_SIZE) {
throw new BalanceTalkException(ErrorCode.BALANCE_GAME_SIZE_TEN);
}
+ if (title == null || title.trim().isEmpty()) {
+ throw new BalanceTalkException(ErrorCode.INVALID_INPUT_VALUE);
+ }
return TempGameSet.builder()
.title(title)
.member(member)
.build();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public TempGameSet toEntity(Member member) { | |
| if (tempGames == null || tempGames.size() < GAME_SIZE) { | |
| throw new BalanceTalkException(ErrorCode.BALANCE_GAME_SIZE_TEN); | |
| } | |
| return TempGameSet.builder() | |
| .title(title) | |
| .mainTag(mainTag) | |
| .subTag(subTag) | |
| .member(member) | |
| .build(); | |
| } | |
| public TempGameSet toEntity(Member member) { | |
| if (tempGames == null || tempGames.size() < GAME_SIZE) { | |
| throw new BalanceTalkException(ErrorCode.BALANCE_GAME_SIZE_TEN); | |
| } | |
| if (title == null || title.trim().isEmpty()) { | |
| throw new BalanceTalkException(ErrorCode.INVALID_INPUT_VALUE); | |
| } | |
| return TempGameSet.builder() | |
| .title(title) | |
| .member(member) | |
| .build(); | |
| } |
| if (!filesIdsToDelete.isEmpty()) { | ||
| List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete); | ||
| fileHandler.deleteFiles(filesToDelete); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
파일 삭제 실패 시 예외 처리 필요
파일 삭제 작업이 실패할 경우에 대한 예외 처리가 없습니다.
다음과 같이 수정을 제안합니다:
if (!filesIdsToDelete.isEmpty()) {
List<File> filesToDelete = fileRepository.findAllById(filesIdsToDelete);
- fileHandler.deleteFiles(filesToDelete);
+ try {
+ fileHandler.deleteFiles(filesToDelete);
+ } catch (Exception e) {
+ throw new BalanceTalkException(ErrorCode.FILE_DELETE_ERROR,
+ String.format("파일 삭제 중 오류가 발생했습니다. 파일 ID: %s", filesIdsToDelete));
+ }
}Committable suggestion skipped: line range outside the PR's diff.
| List<File> files = fileRepository.findAllById(newFileIds); | ||
| Map<Long, File> fileMap = files.stream() | ||
| .collect(Collectors.toMap(File::getId, file -> file)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Map 생성 시 NPE 방지 필요
fileMap 생성 시 null 값에 대한 처리가 없어 NPE가 발생할 수 있습니다.
다음과 같이 수정을 제안합니다:
Map<Long, File> fileMap = files.stream()
- .collect(Collectors.toMap(File::getId, file -> file));
+ .filter(file -> file != null)
+ .collect(Collectors.toMap(
+ File::getId,
+ file -> file,
+ (existing, replacement) -> existing,
+ LinkedHashMap::new
+ ));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| List<File> files = fileRepository.findAllById(newFileIds); | |
| Map<Long, File> fileMap = files.stream() | |
| .collect(Collectors.toMap(File::getId, file -> file)); | |
| List<File> files = fileRepository.findAllById(newFileIds); | |
| Map<Long, File> fileMap = files.stream() | |
| .filter(file -> file != null) | |
| .collect(Collectors.toMap( | |
| File::getId, | |
| file -> file, | |
| (existing, replacement) -> existing, | |
| LinkedHashMap::new | |
| )); |
💡 작업 내용
💡 자세한 설명
최근 임시저장된 게임 불러오기를 한 경우
isLoaded가 true인 경우로, 해당 경우일 때는 파일만 재배치 해주었습니다.
임시저장을 누르는 경우
해당 게임에 존재하는 파일들을 모두 삭제해준 뒤, 마찬가지로 재배치 작업을 해주었습니다.
📗 참고 자료 (선택)
📢 리뷰 요구 사항 (선택)
🚩 후속 작업 (선택)
✅ 셀프 체크리스트
closes #770
Summary by CodeRabbit
신규 기능
TempGameOption클래스에 이미지 ID 및 URL 필드 추가.TempGameSetDto클래스에 로딩 상태 및 파일 ID 검색 기능 추가.버그 수정