Conversation
Summary of ChangesHello @sezeme, 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! This pull request introduces a feature to synchronize quiz progress, allowing users to persist their progress as they complete quizzes within a scene. It includes a new API endpoint, data transfer object, error handling, and service layer implementation to support this functionality. Additionally, it corrects the delimiter used for multiple choice answers. Highlights
Changelog
Activity
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 record SyncProgressRequest( | ||
| Long lastQuizId, | ||
| Integer totalQuestions, | ||
| Integer success, | ||
| Integer failure, | ||
| Integer solveTime, | ||
| boolean isComplete) { | ||
| } |
There was a problem hiding this comment.
SyncProgressRequest DTO의 필드에 대한 유효성 검증이 누락되었습니다. 컨트롤러에서 @Valid 어노테이션을 사용하고 있으므로, DTO 필드에 제약 조건을 추가하여 예상치 못한 값(예: 음수)이 서비스 레이어로 전달되는 것을 막아야 합니다. 이는 데이터 정합성을 보장하고 잠재적인 버그를 예방하는 데 중요합니다. null 값을 허용하지 않으려면 @NotNull도 함께 사용하는 것이 좋습니다.
| public record SyncProgressRequest( | |
| Long lastQuizId, | |
| Integer totalQuestions, | |
| Integer success, | |
| Integer failure, | |
| Integer solveTime, | |
| boolean isComplete) { | |
| } | |
| public record SyncProgressRequest( | |
| Long lastQuizId, | |
| @jakarta.validation.constraints.PositiveOrZero Integer totalQuestions, | |
| @jakarta.validation.constraints.PositiveOrZero Integer success, | |
| @jakarta.validation.constraints.PositiveOrZero Integer failure, | |
| @jakarta.validation.constraints.PositiveOrZero Integer solveTime, | |
| boolean isComplete) { | |
| } |
| public void syncProgress(Long sceneId, QuizDto.SyncProgressRequest request, User user) { | ||
| QuizUserProgress progress = progressRepository.findByUserIdAndSceneId(user.getId(), sceneId) | ||
| .orElseThrow(() -> new BusinessException(CommonErrorCode.QUIZ_PROGRESS_NOT_FOUND)); | ||
|
|
||
| QuizUserProgress updated = QuizUserProgress.builder() | ||
| .id(progress.getId()) | ||
| .user(progress.getUser()) | ||
| .scene(progress.getScene()) | ||
| .lastQuizId(request.lastQuizId()) | ||
| .totalQuestions(request.totalQuestions()) | ||
| .success(request.success()) | ||
| .failure(request.failure()) | ||
| .solveTime(request.solveTime()) | ||
| .isComplete(request.isComplete()) | ||
| .build(); | ||
|
|
||
| progressRepository.save(updated); | ||
| } |
There was a problem hiding this comment.
JPA 엔티티를 업데이트하는 현재 방식은 비효율적이며, 더 나은 대안이 있습니다.
현재 방식의 문제점 (Cons):
builder()를 사용해 새 인스턴스를 생성하고save()를 호출하는 것은 JPA의merge동작을 유발합니다.merge는 영속성 컨텍스트에 해당 엔티티가 없는 경우 데이터베이스에서 다시 조회하는 추가적인SELECT쿼리를 발생시킬 수 있어 비효율적입니다.- 코드를 읽는 개발자에게 새로운 엔티티를 생성하는 것처럼 보여 혼란을 줄 수 있습니다.
개선 방안 (Pros):
JPA의 변경 감지(dirty checking) 기능을 활용하는 것이 좋습니다. @Transactional 메소드 내에서 영속 상태의 엔티티를 수정한 후 명시적으로 save()를 호출하지 않아도 트랜잭션이 커밋될 때 변경 사항이 자동으로 데이터베이스에 반영됩니다. 이를 통해 성능을 개선하고 코드를 더 명확하게 만들 수 있습니다.
대안 1: 엔티티에 업데이트 메소드 추가 (권장)
QuizUserProgress.java에 업데이트 로직을 캡슐화하는 메소드를 추가합니다.
// QuizUserProgress.java
public void syncProgress(QuizDto.SyncProgressRequest request) {
this.lastQuizId = request.lastQuizId();
this.totalQuestions = request.totalQuestions();
this.success = request.success();
this.failure = request.failure();
this.solveTime = request.solveTime();
this.isComplete = request.isComplete();
}그 후 QuizService의 syncProgress 메소드를 다음과 같이 수정합니다.
// QuizService.java
@Transactional
public void syncProgress(Long sceneId, QuizDto.SyncProgressRequest request, User user) {
QuizUserProgress progress = progressRepository.findByUserIdAndSceneId(user.getId(), sceneId)
.orElseThrow(() -> new BusinessException(CommonErrorCode.QUIZ_PROGRESS_NOT_FOUND));
progress.syncProgress(request);
// progressRepository.save() 호출이 더 이상 필요하지 않습니다.
}대안 2: Setter 사용
엔티티에 Setter를 열어두고 서비스 레이어에서 직접 값을 변경할 수도 있지만, 도메인 객체의 캡슐화를 해칠 수 있어 첫 번째 방안을 더 권장합니다.
| User user = userRepository.findByUsername(userDetails.getUsername()) | ||
| .orElseThrow(() -> new BusinessException(CommonErrorCode.USER_NOT_FOUND)); |
There was a problem hiding this comment.
컨트롤러 내 여러 메소드에서 사용자 정보를 조회하는 로직이 중복되고 있습니다.
User user = userRepository.findByUsername(userDetails.getUsername())
.orElseThrow(() -> new BusinessException(CommonErrorCode.USER_NOT_FOUND));이러한 중복은 코드 유지보수성을 저하시킬 수 있습니다.
개선 방안:
HandlerMethodArgumentResolver를 구현하여 사용자 조회 로직을 중앙에서 관리하고, 컨트롤러 메소드에서는 @AuthUser User user 와 같이 어노테이션을 통해 바로 User 객체를 주입받는 방식을 고려해볼 수 있습니다. 이렇게 하면 코드 중복을 제거하고 컨트롤러를 더 깔끔하게 유지할 수 있습니다. 이는 장기적인 관점에서 프로젝트의 유지보수성을 높이는 데 도움이 될 것입니다.
References
- 단순한 코드 수정 요청을 넘어, 개발자의 성장을 돕는 통찰을 제공하십시오. 코드 패턴을 분석하여 유지보수성 및 성능 측면에서의 잘한 점(Pros)과 개선할 점(Cons)을 명확히 구분해 설명하십시오. 장기적으로 나아가야 할 기술적 방향성을 제시하여 자기계발에 도움을 주십시오. (link)
* Feat/add mock data (#44) * refactor(security): update permitted endpoints and remove ChatController - Updated SecurityConfig to restrict access by removing the chat endpoint from the permitted list. - Deleted ChatController as it is no longer needed in the application. * refactor(quiz): rename quiz class and delete quiz choice * feat: add & fix mock data * feat: rebase 마무리 * feat(data loader): add scene rank data * fix: accept review --------- Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr> * Feat/31 get quiz list (#46) * refactor(security): update permitted endpoints and remove ChatController - Updated SecurityConfig to restrict access by removing the chat endpoint from the permitted list. - Deleted ChatController as it is no longer needed in the application. * feat(quiz): get quiz list * fix: order by quiz id * fix(quiz): 정답을 프론트로 내려주지 않도록 수정, 객관식 채점 시 정답 올바르게 파싱하도록 수정 --------- Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr> * fix: 정답 리턴값 수정 (#47) --------- Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr>
* Feat/add mock data (#44) * refactor(security): update permitted endpoints and remove ChatController - Updated SecurityConfig to restrict access by removing the chat endpoint from the permitted list. - Deleted ChatController as it is no longer needed in the application. * refactor(quiz): rename quiz class and delete quiz choice * feat: add & fix mock data * feat: rebase 마무리 * feat(data loader): add scene rank data * fix: accept review --------- Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr> * Feat/31 get quiz list (#46) * refactor(security): update permitted endpoints and remove ChatController - Updated SecurityConfig to restrict access by removing the chat endpoint from the permitted list. - Deleted ChatController as it is no longer needed in the application. * feat(quiz): get quiz list * fix: order by quiz id * fix(quiz): 정답을 프론트로 내려주지 않도록 수정, 객관식 채점 시 정답 올바르게 파싱하도록 수정 --------- Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr> * fix: 정답 리턴값 수정 (#47) * fix(quiz): prevent totalQuestions from incrementing during grading * fix: ci error 해결 --------- Co-authored-by: Chewon Kim <stellaa223@gmail.com> Co-authored-by: DaehyeonKim <qwertypotter@gachon.ac.kr>
1 participant
Close #32