feat: 스터디 통계 조회 API 추가 #809
Conversation
Walkthrough이 PR에서는 Changes
Assessment against linked issues
Possibly related PRs
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 (
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
src/main/java/com/gdschongik/gdsc/domain/study/dao/AttendanceRepository.java (1)
9-9: 새로운 메서드가 올바르게 추가되었습니다.새로 추가된
countByStudyDetailId메서드는 Spring Data JPA 명명 규칙을 잘 따르고 있으며, PR의 목적인 스터디 통계 조회 기능 구현에 적절히 기여할 것으로 보입니다.메서드의 목적을 명확히 하기 위해 다음과 같은 주석을 추가하는 것이 좋을 것 같습니다:
/** * 특정 스터디 세부 정보에 대한 출석 기록 수를 조회합니다. * @param studyDetailId 조회할 스터디 세부 정보의 ID * @return 해당 스터디 세부 정보에 대한 출석 기록 수 */ long countByStudyDetailId(Long studyDetailId);src/main/java/com/gdschongik/gdsc/domain/study/dao/AssignmentHistoryRepository.java (1)
14-14: 새로운 메서드가 PR 목표에 부합하게 추가되었습니다.
countByStudyDetailIdAndSubmissionStatusEquals메서드는 Spring Data JPA 쿼리 메서드 명명 규칙을 따르고 있으며, 스터디 통계 조회 API의 요구사항을 충족시키는 것으로 보입니다. 이 메서드를 통해 특정 스터디의 과제 제출 상태에 따른 통계를 계산할 수 있을 것입니다.다만, 메서드 이름이 다소 길어 가독성이 떨어질 수 있습니다. 필요하다면
@Query어노테이션을 사용하여 메서드 이름을 간소화하는 것을 고려해 보시기 바랍니다.예를 들어, 다음과 같이 변경할 수 있습니다:
@Query("SELECT COUNT(ah) FROM AssignmentHistory ah WHERE ah.studyDetail.id = :studyDetailId AND ah.submissionStatus = :status") long countAssignmentsByStudyAndStatus(@Param("studyDetailId") Long studyDetailId, @Param("status") AssignmentSubmissionStatus status);이렇게 하면 메서드 이름이 더 간결해지고 쿼리의 의도가 명확해집니다.
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1)
11-14: 메서드 이름을 'of'로 변경하는 것이 어떨까요?이전 리뷰 코멘트와 프로젝트의 네이밍 컨벤션을 고려할 때,
openedWeekStatisticsOf메서드의 이름을of로 변경하는 것이 좋을 것 같습니다. 이렇게 하면 다른 DTO들과 일관성을 유지할 수 있습니다.다음과 같이 변경을 제안합니다:
- public static StudyWeekStatisticsResponse openedWeekStatisticsOf( + public static StudyWeekStatisticsResponse of( Long studyWeek, Long attendanceRate, Long assignmentSubmitRate) { return new StudyWeekStatisticsResponse(studyWeek, attendanceRate, assignmentSubmitRate, false); }src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1)
6-12: 레코드 선언이 잘 구성되어 있습니다.레코드 사용과 Swagger 어노테이션 적용이 적절합니다. 필드 이름과 타입도 목적에 맞게 잘 선택되었습니다.
studyCompleteRate필드는 팩토리 메서드에서 계산되므로, 이를 명확히 하기 위해 주석을 추가하는 것이 좋을 것 같습니다. 예를 들어:@Schema(description = "스터디 수료율 (팩토리 메서드에서 계산됨)") Long studyCompleteRate,src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java (1)
85-88: LGTM! 메서드 구현이 적절합니다.
isComplete()메서드의 구현이 간결하고 명확합니다. 기존의complete()메서드와 잘 연계되어 있습니다.가독성을 더욱 높이기 위해 다음과 같이 간단한 JavaDoc 주석을 추가하는 것을 고려해 보세요:
/** * 스터디 이력의 완료 상태를 반환합니다. * @return 스터디가 완료되었으면 true, 그렇지 않으면 false */ public boolean isComplete() { return studyHistoryStatus == COMPLETED; }src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyDetailController.java (1)
85-90: LGTM: 새로운 메서드가 잘 구현되었습니다. 작은 개선 사항이 있습니다.새로운
getStudyStatistics메서드가 잘 구현되었습니다. Swagger 문서화를 위한@Operation어노테이션, HTTP GET 요청을 위한@GetMapping어노테이션, 그리고 적절한 메서드 시그니처를 사용하고 있습니다. 서비스 계층에 작업을 위임하는 것도 좋은 방식입니다.작은 개선 사항:
- 일관성을 위해 다른 메서드들과 마찬가지로
ResponseEntity.ok().body(response)를 사용하는 것이 좋습니다.다음과 같이 수정하는 것을 고려해보세요:
@GetMapping("/statistics") public ResponseEntity<StudyStatisticsResponse> getStudyStatistics(@RequestParam(name = "studyId") Long studyId) { StudyStatisticsResponse response = mentorStudyDetailService.getStudyStatistics(studyId); - return ResponseEntity.ok(response); + return ResponseEntity.ok().body(response); }src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (1)
129-157: 단위 테스트 추가 권장새로 추가된
getStudyStatistics메서드 및 관련 계산 메서드들에 대한 단위 테스트를 작성하여 기능의 정확성을 검증하는 것이 좋습니다. 이는 이후 유지보수와 기능 변경 시 오류를 사전에 방지하는 데 도움이 됩니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyDetailController.java (2 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (3 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dao/AssignmentHistoryRepository.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dao/AttendanceRepository.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
src/main/java/com/gdschongik/gdsc/domain/study/dao/AssignmentHistoryRepository.java (1)
5-5: 새로운 import 문이 올바르게 추가되었습니다.
AssignmentSubmissionStatus를 import하는 것은 새로 추가된 메서드에 필요하며, 적절한 위치에 배치되었습니다.src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1)
5-9: LGTM: 레코드 선언과 필드가 적절합니다.레코드 선언과 필드 구성이 잘 되어 있습니다. @Schema 어노테이션을 사용하여 API 문서화를 위한 설명을 추가한 것이 좋습니다. 필드 이름과 타입도 목적에 맞게 적절히 선택되었습니다.
src/main/java/com/gdschongik/gdsc/domain/study/api/MentorStudyDetailController.java (1)
8-8: LGTM: 새로운 import 문이 올바르게 추가되었습니다.새로운
StudyStatisticsResponse클래스의 import 문이 적절하게 추가되었습니다. 이는 새로 추가된 메서드의 반환 타입과 일치하며, 파일의 기존 import 스타일을 따르고 있습니다.src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (1)
135-142: [기존 리뷰 반영 확인]이미 이전 리뷰에서
studyHistories조회 로직을 코드 상단으로 이동하는 것에 대한 의견이 있었고, 이에 대해 고민하신 것으로 보입니다. 해당 부분에 대한 수정 여부를 한 번 더 검토해 주시기 바랍니다.
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| List<StudyDetail> studyDetails, Long totalStudentCount) { | ||
|
|
||
| return studyDetails.stream() | ||
| .map((studyDetail -> { |
There was a problem hiding this comment.
map 내부 람다식이 복잡한데, 별도 메서드로 추출하면 좋을듯 합니다
There was a problem hiding this comment.
우선 DTO 정적 팩토리 메서드 종류를 나눠서 케이스마다 반환하도록 했는데, 지금 생각하기에는 내부 로직에서 메서드로 더 분리할 만한 부분은 평균 출석률 계산, 평균 과제 제출률 계산 로직 정도가 있을 것 같습니다.
개인적으로는 각 계산 로직의 길이가 2줄로 길지 않고, 변수명을 통해 코드의 의도를 이해할 수 있을 것 같아서 메서드로 분리하지 않고 놔두었는데, 의도하신 리뷰에 맞는지 확인 부탁드립니다!
There was a problem hiding this comment.
아 람다식을 extract해서, 스트림 내부를 method reference로 처리하면 더 깔끔할 것 같다는 이야기었어요. 제가 설명을 제대로 못했네요
++ 후속 리뷰 필요한 경우 resolve 처리하지 마시고 열어두시면 제가 리뷰 확인할 때 더 편할 것 같아요~ 이번엔 제가 unresolve 처리해두었습니다
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (3 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java
🧰 Additional context used
🔇 Additional comments (5)
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (3)
5-10: 레코드 선언이 잘 구성되어 있습니다.레코드 구조가 적절하며 각 필드에 대한 @Schema 어노테이션을 통해 API 문서화가 잘 되어 있습니다. 필드 이름과 타입이 목적에 맞게 잘 선택되었습니다.
12-14: openedOf 메서드가 적절히 구현되었습니다.이 메서드는 열린 스터디 주차에 대한 StudyWeekStatisticsResponse를 생성하며, 로직이 명확하고 간결합니다. 메서드 이름이 이전 리뷰 코멘트에서 제안된 컨벤션을 따르고 있습니다.
16-18: emptyOf 메서드가 잘 구현되었습니다.이 메서드는 출석률과 과제 제출률을 0으로 설정하면서 과제 휴강과 스터디 휴강을 별도로 지정할 수 있어 유연성이 높습니다. 메서드 이름이 이전 리뷰 코멘트에서 제안된 컨벤션을 따르고 있습니다.
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (2)
3-3: 새로운 의존성 추가가 적절해 보입니다.새로운 기능을 위해 필요한 리포지토리와 도메인 클래스들이 잘 추가되었습니다. 이러한 변경은 스터디 통계 기능 구현에 필요한 것으로 보입니다.
Also applies to: 7-8, 10-12, 15-16, 21-22, 40-44
180-189: 평균 출석률 계산 로직이 적절해 보입니다.
calculateAverageWeekAttendanceRate메서드의 구현이 잘 되어 있습니다. 취소된 주를 제외하고 평균을 계산하는 로직이 명확하고 효율적으로 구현되어 있습니다. Java 8의 스트림 API를 잘 활용하고 있어 코드가 간결하고 읽기 쉽습니다.
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
|
정적 팩토리 메서드 이름으로 과제 휴강도 아니고 스터디 휴강도 아닌 경우 혹시 다른 좋은 네이밍 아이디어가 있으시면 추천 부탁드립니다! |
네이밍의 경우 |
| studyValidator.validateStudyMentor(currentMember, study); | ||
|
|
||
| long totalStudentCount = studyHistories.size(); | ||
| long studyCompleteStudentCount = |
There was a problem hiding this comment.
| long studyCompleteStudentCount = | |
| long studyCompletedStudentCount = |
There was a problem hiding this comment.
이거 아직 반영 안 된 것 같은데 resolve되어 있어서 unresolve 해둘게요!
There was a problem hiding this comment.
제가 코드 상에 반영해두고 resolve 한 뒤 커밋을 안 했었네요. 앞으론 커밋하고 resolve 할게요! 감사합니당
| List<StudyDetail> studyDetails, Long totalStudentCount) { | ||
|
|
||
| return studyDetails.stream() | ||
| .map((studyDetail -> { |
There was a problem hiding this comment.
아 람다식을 extract해서, 스트림 내부를 method reference로 처리하면 더 깔끔할 것 같다는 이야기었어요. 제가 설명을 제대로 못했네요
++ 후속 리뷰 필요한 경우 resolve 처리하지 마시고 열어두시면 제가 리뷰 확인할 때 더 편할 것 같아요~ 이번엔 제가 unresolve 처리해두었습니다
| } | ||
|
|
||
| private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) { | ||
| boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen(); |
There was a problem hiding this comment.
그리고 개인적인 생각인데 '스터다 주차' 라고 하면 수업과 과제 모두를 포괄하는 느낌이라서, 뭔가 좀 어색한 것 같네요. '해당 주차가 취소되었는지' 보다는 '해당 주차 수업 / 과제가 취소되었는지' 로 나타내는게 더 정확하지 않나 싶네요 (cc @Sangwook02 )
There was a problem hiding this comment.
week 가 포괄적인 느낌이라는데 공감합니다. 변수명 고민을 좀 더 해봐야겠네요..
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
Outdated
Show resolved
Hide resolved
| return StudyWeekStatisticsResponse.emptyOf(week, true, true); | ||
| } | ||
|
|
||
| public static StudyWeekStatisticsResponse canceledAssignmentOf(Long week, Long attendanceRate) { |
There was a problem hiding this comment.
| public static StudyWeekStatisticsResponse canceledAssignmentOf(Long week, Long attendanceRate) { | |
| public static StudyWeekStatisticsResponse assignmentCanceled(Long week, Long attendanceRate) { |
There was a problem hiding this comment.
of, from 컨벤션을 지키면서 케이스가 나뉘는 경우 어떻게 처리해야 하나 궁금했는데, 자세하게 알려주셔서 감사합니다~!
There was a problem hiding this comment.
이전 컨벤션은 create 접두사를 붙이는 쪽이었는데, 이번에 DTO / VO 쪽에서는 접두사 없이 가면서 조금 어색한 감이 있을 수는 있어요.
일단 쭉 써보면서 계속 논의해봅시다~
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) { | ||
| boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen(); |
There was a problem hiding this comment.
isCanceled 메서드가 있는데 isOpen을 쓰신 이유가 있나요?
There was a problem hiding this comment.
저도 그걸로 고민을 했었는데, 지금 존재하는 상태가 open, cancel, none 이렇게 3가지가 있어서, isCanceled 를 사용하면 none 으로 설정된 studyDetail 도 열린 주차로 인식하는 문제가 발생할 것 같았어요. 그래서 cancel, none 타입인 경우 모두 canceled week 로 간주하도록 했습니다.
There was a problem hiding this comment.
아 open이 아닌지만 확인하면 되는거였군요
그럼 변수명을 수정하면 좋을 것 같아요!
| return StudyWeekStatisticsResponse.canceledAssignmentOf(studyDetail.getWeek(), attendanceRate); | ||
| } | ||
|
|
||
| long assignmentCount = |
There was a problem hiding this comment.
제출 성공인 과제의 수를 count하는 것 같은데 변수명을 보고는 알기 어려운 것 같습니다!
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1)
6-12: Schema 문서화가 잘 되어있으나, 필드 값의 범위에 대한 설명이 필요합니다.
각 필드에 대한 기본적인 설명은 잘 되어있습니다. 하지만 통계 데이터의 특성상 다음과 같은 추가 정보가 있으면 좋을 것 같습니다:
- 비율 필드들(averageAttendanceRate, averageAssignmentSubmissionRate, studyCompleteRate)의 값 범위 (0-100)
- 각 카운트 필드들의 제약 조건 (0 이상의 정수)
다음과 같이 Schema 설명을 보완하는 것을 제안드립니다:
-@Schema(description = "평균 출석률") Long averageAttendanceRate,
+@Schema(description = "평균 출석률 (0-100 범위의 백분율)") Long averageAttendanceRate,📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (3 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1 hunks)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1 hunks)
🔇 Additional comments (3)
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java (1)
5-10: 레코드 구조가 명확하고 잘 설계되었습니다!
각 필드의 역할이 명확하게 구분되어 있으며, @Schema 어노테이션을 통한 문서화가 잘 되어있습니다. 특히 과제 휴강과 스터디 휴강을 분리한 것이 좋은 설계 결정이었습니다.
src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyStatisticsResponse.java (1)
1-28: 🛠️ Refactor suggestion
불변성 보장을 위한 방어적 복사 검토가 필요합니다.
studyWeekStatisticsResponses 리스트가 외부에서 수정될 수 있는 가능성이 있습니다.
리스트의 불변성을 검증하기 위해 다음 스크립트를 실행해보겠습니다:
리스트의 불변성을 보장하기 위해 다음과 같은 수정을 제안드립니다:
public static StudyStatisticsResponse of(
Long totalStudentCount,
Long completeStudentCount,
Long averageAttendanceRate,
Long averageAssignmentSubmissionRate,
List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) {
return new StudyStatisticsResponse(
totalStudentCount,
completeStudentCount,
averageAttendanceRate,
averageAssignmentSubmissionRate,
totalStudentCount == 0 ? 0 : Math.round(completeStudentCount / (double) totalStudentCount * 100),
- studyWeekStatisticsResponses);
+ List.copyOf(studyWeekStatisticsResponses));
}src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (1)
128-153: getStudyStatistics 메서드의 구현이 훌륭합니다
getStudyStatistics 메서드는 필요한 통계를 효과적으로 계산하고 있습니다. 전체적인 로직과 데이터 조회 방법이 명확하고 읽기 쉽습니다.
| public static StudyWeekStatisticsResponse opened(Long week, Long attendanceRate, Long assignmentSubmissionRate) { | ||
| return new StudyWeekStatisticsResponse(week, attendanceRate, assignmentSubmissionRate, false, false); | ||
| } | ||
|
|
||
| public static StudyWeekStatisticsResponse empty(Long week, boolean isCanceledAssignment, boolean isCanceledWeek) { | ||
| return new StudyWeekStatisticsResponse(week, 0L, 0L, isCanceledAssignment, isCanceledWeek); | ||
| } | ||
|
|
||
| public static StudyWeekStatisticsResponse canceledWeek(Long week) { | ||
| return StudyWeekStatisticsResponse.empty(week, true, true); | ||
| } | ||
|
|
||
| public static StudyWeekStatisticsResponse assignmentCanceled(Long week, Long attendanceRate) { | ||
| return new StudyWeekStatisticsResponse(week, attendanceRate, 0L, true, false); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
매개변수 유효성 검증 추가 제안
각 정적 팩토리 메서드에 대해 매개변수 유효성 검증을 추가하면 좋을 것 같습니다:
week가 null이거나 음수인 경우attendanceRate와assignmentSubmissionRate가 0-100 범위를 벗어나는 경우
다음과 같은 변경을 제안합니다:
public static StudyWeekStatisticsResponse opened(Long week, Long attendanceRate, Long assignmentSubmissionRate) {
+ validateWeek(week);
+ validateRate(attendanceRate, "출석률");
+ validateRate(assignmentSubmissionRate, "과제 제출률");
return new StudyWeekStatisticsResponse(week, attendanceRate, assignmentSubmissionRate, false, false);
}
+private static void validateWeek(Long week) {
+ if (week == null || week < 1) {
+ throw new IllegalArgumentException("주차는 1 이상의 값이어야 합니다.");
+ }
+}
+
+private static void validateRate(Long rate, String fieldName) {
+ if (rate == null || rate < 0 || rate > 100) {
+ throw new IllegalArgumentException(fieldName + "은(는) 0에서 100 사이의 값이어야 합니다.");
+ }
+}Committable suggestion was skipped due to low confidence.
| public static StudyStatisticsResponse of( | ||
| Long totalStudentCount, | ||
| Long completeStudentCount, | ||
| Long averageAttendanceRate, | ||
| Long averageAssignmentSubmissionRate, | ||
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | ||
| return new StudyStatisticsResponse( | ||
| totalStudentCount, | ||
| completeStudentCount, | ||
| averageAttendanceRate, | ||
| averageAssignmentSubmissionRate, | ||
| totalStudentCount == 0 ? 0 : Math.round(completeStudentCount / (double) totalStudentCount * 100), | ||
| studyWeekStatisticsResponses); | ||
| } |
There was a problem hiding this comment.
팩토리 메서드의 안전성 개선이 필요합니다.
현재 구현에서 다음과 같은 잠재적인 문제가 있습니다:
completeStudentCount가 null일 경우 NPE 발생 가능completeStudentCount가totalStudentCount보다 큰 경우에 대한 검증 부재
다음과 같은 개선을 제안드립니다:
public static StudyStatisticsResponse of(
Long totalStudentCount,
Long completeStudentCount,
Long averageAttendanceRate,
Long averageAssignmentSubmissionRate,
List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) {
+ long safeTotal = totalStudentCount != null ? totalStudentCount : 0L;
+ long safeComplete = completeStudentCount != null ? completeStudentCount : 0L;
+
+ if (safeComplete > safeTotal) {
+ throw new IllegalArgumentException("완료 학생 수는 전체 학생 수를 초과할 수 없습니다.");
+ }
+
return new StudyStatisticsResponse(
- totalStudentCount,
- completeStudentCount,
+ safeTotal,
+ safeComplete,
averageAttendanceRate,
averageAssignmentSubmissionRate,
- totalStudentCount == 0 ? 0 : Math.round(completeStudentCount / (double) totalStudentCount * 100),
+ safeTotal == 0 ? 0 : Math.round(safeComplete / (double) safeTotal * 100),
studyWeekStatisticsResponses);
}📝 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 static StudyStatisticsResponse of( | |
| Long totalStudentCount, | |
| Long completeStudentCount, | |
| Long averageAttendanceRate, | |
| Long averageAssignmentSubmissionRate, | |
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | |
| return new StudyStatisticsResponse( | |
| totalStudentCount, | |
| completeStudentCount, | |
| averageAttendanceRate, | |
| averageAssignmentSubmissionRate, | |
| totalStudentCount == 0 ? 0 : Math.round(completeStudentCount / (double) totalStudentCount * 100), | |
| studyWeekStatisticsResponses); | |
| } | |
| public static StudyStatisticsResponse of( | |
| Long totalStudentCount, | |
| Long completeStudentCount, | |
| Long averageAttendanceRate, | |
| Long averageAssignmentSubmissionRate, | |
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | |
| long safeTotal = totalStudentCount != null ? totalStudentCount : 0L; | |
| long safeComplete = completeStudentCount != null ? completeStudentCount : 0L; | |
| if (safeComplete > safeTotal) { | |
| throw new IllegalArgumentException("완료 학생 수는 전체 학생 수를 초과할 수 없습니다."); | |
| } | |
| return new StudyStatisticsResponse( | |
| safeTotal, | |
| safeComplete, | |
| averageAttendanceRate, | |
| averageAssignmentSubmissionRate, | |
| safeTotal == 0 ? 0 : Math.round(safeComplete / (double) safeTotal * 100), | |
| studyWeekStatisticsResponses); | |
| } |
| private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) { | ||
| boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen(); | ||
| boolean isCanceledAssignment = !studyDetail.getAssignment().isOpen() || isCanceledWeek; | ||
|
|
||
| if (totalStudentCount == 0) { | ||
| return StudyWeekStatisticsResponse.empty(studyDetail.getWeek(), isCanceledAssignment, isCanceledWeek); | ||
| } | ||
|
|
||
| if (isCanceledWeek) { | ||
| return StudyWeekStatisticsResponse.canceledWeek(studyDetail.getWeek()); | ||
| } | ||
|
|
||
| long attendanceCount = attendanceRepository.countByStudyDetailId(studyDetail.getId()); | ||
| long attendanceRate = Math.round(attendanceCount / (double) totalStudentCount * 100); | ||
|
|
||
| if (isCanceledAssignment) { | ||
| return StudyWeekStatisticsResponse.assignmentCanceled(studyDetail.getWeek(), attendanceRate); | ||
| } | ||
|
|
||
| long assignmentCount = | ||
| assignmentHistoryRepository.countByStudyDetailIdAndSubmissionStatusEquals(studyDetail.getId(), SUCCESS); | ||
| long assignmentRate = Math.round(assignmentCount / (double) totalStudentCount * 100); | ||
|
|
||
| return StudyWeekStatisticsResponse.opened(studyDetail.getWeek(), attendanceRate, assignmentRate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
calculateWeekStatistics 메서드의 가독성 향상을 위한 리팩토링 제안
현재 calculateWeekStatistics 메서드에서 여러 return 문이 존재하여 흐름을 따라가기 어려울 수 있습니다. 가독성을 높이기 위해 조건 분기를 구조화하거나, 메서드를 분리하여 코드의 명확성을 높이는 것을 고려해보세요.
| boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen(); | ||
| boolean isCanceledAssignment = !studyDetail.getAssignment().isOpen() || isCanceledWeek; |
There was a problem hiding this comment.
NullPointerException 방지를 위한 Null 체크 추가 필요
studyDetail.getCurriculum() 또는 studyDetail.getAssignment()가 null인 경우 NullPointerException이 발생할 수 있습니다. 이를 방지하기 위해 null 체크를 추가하는 것이 좋습니다.
적용할 코드 변경 사항:
- boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen();
+ boolean isCanceledWeek = studyDetail.getCurriculum() == null || !studyDetail.getCurriculum().isOpen();
- boolean isCanceledAssignment = !studyDetail.getAssignment().isOpen() || isCanceledWeek;
+ boolean isCanceledAssignment = studyDetail.getAssignment() == null || !studyDetail.getAssignment().isOpen() || isCanceledWeek;📝 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.
| boolean isCanceledWeek = !studyDetail.getCurriculum().isOpen(); | |
| boolean isCanceledAssignment = !studyDetail.getAssignment().isOpen() || isCanceledWeek; | |
| boolean isCanceledWeek = studyDetail.getCurriculum() == null || !studyDetail.getCurriculum().isOpen(); | |
| boolean isCanceledAssignment = studyDetail.getAssignment() == null || !studyDetail.getAssignment().isOpen() || isCanceledWeek; |
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java
Outdated
Show resolved
Hide resolved
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
| public StudyStatisticsResponse getStudyStatistics(Long studyId) { | ||
| Member currentMember = memberUtil.getCurrentMember(); | ||
| Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); | ||
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId); | ||
| List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId); | ||
| studyValidator.validateStudyMentor(currentMember, study); | ||
|
|
||
| long totalStudentCount = studyHistories.size(); | ||
| long studyCompletedStudentCount = | ||
| studyHistories.stream().filter(StudyHistory::isComplete).count(); | ||
|
|
||
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses = studyDetails.stream() | ||
| .map((studyDetail -> calculateWeekStatistics(studyDetail, totalStudentCount))) | ||
| .toList(); | ||
|
|
||
| long averageAttendanceRate = calculateAverageWeekAttendanceRate(studyWeekStatisticsResponses); | ||
| long averageAssignmentSubmissionRate = | ||
| calculateAverageWeekAssignmentSubmissionRate(studyWeekStatisticsResponses); | ||
|
|
||
| return StudyStatisticsResponse.of( | ||
| totalStudentCount, | ||
| studyCompletedStudentCount, | ||
| averageAttendanceRate, | ||
| averageAssignmentSubmissionRate, | ||
| studyWeekStatisticsResponses); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
성능 최적화 및 방어 로직 추가 제안
다음과 같은 개선사항을 제안드립니다:
studyHistories가 null일 경우를 대비한 방어 로직이 필요합니다.- 데이터베이스 쿼리 최적화를 위해
studyHistories와studyDetails를 한 번의 쿼리로 조회하는 것이 좋습니다.
@Transactional(readOnly = true)
public StudyStatisticsResponse getStudyStatistics(Long studyId) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId)
.orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
studyValidator.validateStudyMentor(currentMember, study);
List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId);
if (studyHistories == null) {
studyHistories = List.of();
}
// TODO: Consider optimizing by fetching studyHistories and studyDetails in a single query
List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId);
// ... rest of the code
}| private long calculateAverageWeekAttendanceRate(List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | ||
|
|
||
| double averageAttendanceRate = studyWeekStatisticsResponses.stream() | ||
| .filter(weekStatisticsResponse -> !weekStatisticsResponse.isCanceledWeek()) | ||
| .mapToLong(StudyWeekStatisticsResponse::attendanceRate) | ||
| .average() | ||
| .orElse(0); | ||
|
|
||
| return Math.round(averageAttendanceRate); | ||
| } | ||
|
|
||
| private long calculateAverageWeekAssignmentSubmissionRate( | ||
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | ||
|
|
||
| double averageAssignmentSubmissionRate = studyWeekStatisticsResponses.stream() | ||
| .filter(studyWeekStatistics -> !studyWeekStatistics.isAssignmentCanceled()) | ||
| .mapToLong(StudyWeekStatisticsResponse::assignmentSubmissionRate) | ||
| .average() | ||
| .orElse(0); | ||
|
|
||
| return Math.round(averageAssignmentSubmissionRate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
평균 계산 로직 중복 제거 필요
출석률과 과제 제출률 계산 로직이 매우 유사합니다. 공통 메서드로 추출하여 중복을 제거하는 것이 좋습니다.
private long calculateAverageRate(
List<StudyWeekStatisticsResponse> responses,
java.util.function.Predicate<StudyWeekStatisticsResponse> filterCondition,
java.util.function.ToLongFunction<StudyWeekStatisticsResponse> rateExtractor) {
return Math.round(responses.stream()
.filter(filterCondition)
.mapToLong(rateExtractor)
.average()
.orElse(0));
}
private long calculateAverageWeekAttendanceRate(List<StudyWeekStatisticsResponse> responses) {
return calculateAverageRate(
responses,
response -> !response.isCanceledWeek(),
StudyWeekStatisticsResponse::attendanceRate);
}
private long calculateAverageWeekAssignmentSubmissionRate(List<StudyWeekStatisticsResponse> responses) {
return calculateAverageRate(
responses,
response -> !response.isAssignmentCanceled(),
StudyWeekStatisticsResponse::assignmentSubmissionRate);
}| private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) { | ||
| boolean isNotOpenedCurriculum = !studyDetail.getCurriculum().isOpen(); | ||
| boolean isNotOpenedAssignment = !studyDetail.getAssignment().isOpen() || isNotOpenedCurriculum; | ||
|
|
||
| if (totalStudentCount == 0) { | ||
| return StudyWeekStatisticsResponse.empty( | ||
| studyDetail.getWeek(), isNotOpenedAssignment, isNotOpenedCurriculum); | ||
| } | ||
|
|
||
| if (isNotOpenedCurriculum) { | ||
| return StudyWeekStatisticsResponse.canceledWeek(studyDetail.getWeek()); | ||
| } | ||
|
|
||
| long attendanceCount = attendanceRepository.countByStudyDetailId(studyDetail.getId()); | ||
| long attendanceRate = Math.round(attendanceCount / (double) totalStudentCount * 100); | ||
|
|
||
| if (isNotOpenedAssignment) { | ||
| return StudyWeekStatisticsResponse.assignmentCanceled(studyDetail.getWeek(), attendanceRate); | ||
| } | ||
|
|
||
| long successfullySubmittedAssignmentCount = | ||
| assignmentHistoryRepository.countByStudyDetailIdAndSubmissionStatusEquals(studyDetail.getId(), SUCCESS); | ||
| long assignmentSubmissionRate = | ||
| Math.round(successfullySubmittedAssignmentCount / (double) totalStudentCount * 100); | ||
|
|
||
| return StudyWeekStatisticsResponse.opened(studyDetail.getWeek(), attendanceRate, assignmentSubmissionRate); | ||
| } |
There was a problem hiding this comment.
Null 체크 추가 및 변수명 개선 필요
getCurriculum()과getAssignment()의 null 체크가 필요합니다.isNotOpenedCurriculum과isNotOpenedAssignment변수명이 부정문을 사용하여 이해하기 어렵습니다.
private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) {
- boolean isNotOpenedCurriculum = !studyDetail.getCurriculum().isOpen();
- boolean isNotOpenedAssignment = !studyDetail.getAssignment().isOpen() || isNotOpenedCurriculum;
+ boolean isCurriculumClosed = studyDetail.getCurriculum() == null || !studyDetail.getCurriculum().isOpen();
+ boolean isAssignmentClosed = studyDetail.getAssignment() == null || !studyDetail.getAssignment().isOpen() || isCurriculumClosed;
if (totalStudentCount == 0) {
return StudyWeekStatisticsResponse.empty(
- studyDetail.getWeek(), isNotOpenedAssignment, isNotOpenedCurriculum);
+ studyDetail.getWeek(), isAssignmentClosed, isCurriculumClosed);
}
// ... rest of the code with updated variable names
}Committable suggestion was skipped due to low confidence.
|
canceled week 대신에 사용할 표현으로 class, lecture, course 같은 걸 생각해봤는데, 커리큘럼이 취소된다는 것이 일반적으로 어색하긴 하지만 프로젝트에서 |
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java(3 hunks)src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/gdschongik/gdsc/domain/study/dto/response/StudyWeekStatisticsResponse.java
🔇 Additional comments (3)
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (3)
Line range hint 3-44: 새로운 의존성 주입이 적절히 구성되었습니다.
필요한 레포지토리들과 validator가 잘 추가되었습니다.
128-153: 🛠️ Refactor suggestion
성능 최적화 및 방어 로직 개선이 필요합니다.
다음과 같은 개선사항을 제안드립니다:
studyHistories와studyDetails를 단일 쿼리로 조회하여 데이터베이스 호출을 최적화할 수 있습니다.studyHistories가 null인 경우를 대비한 방어 로직이 필요합니다.
@Transactional(readOnly = true)
public StudyStatisticsResponse getStudyStatistics(Long studyId) {
Member currentMember = memberUtil.getCurrentMember();
Study study = studyRepository.findById(studyId)
.orElseThrow(() -> new CustomException(STUDY_NOT_FOUND));
studyValidator.validateStudyMentor(currentMember, study);
List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId);
+ if (studyHistories == null) {
+ studyHistories = List.of();
+ }
// TODO: studyHistories와 studyDetails를 단일 쿼리로 조회하는 것을 고려해보세요
List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId);
// ... 나머지 코드
}Likely invalid or redundant comment.
155-181:
NPE 방지 및 변수명 개선이 필요합니다.
getCurriculum()과getAssignment()의 null 체크가 필요합니다.- 부정문을 사용한 변수명(
isNotOpenedCurriculum,isNotOpenedAssignment)은 이해하기 어렵습니다.
private StudyWeekStatisticsResponse calculateWeekStatistics(StudyDetail studyDetail, Long totalStudentCount) {
- boolean isNotOpenedCurriculum = !studyDetail.getCurriculum().isOpen();
- boolean isNotOpenedAssignment = !studyDetail.getAssignment().isOpen() || isNotOpenedCurriculum;
+ boolean isCurriculumClosed = studyDetail.getCurriculum() == null || !studyDetail.getCurriculum().isOpen();
+ boolean isAssignmentClosed = studyDetail.getAssignment() == null || !studyDetail.getAssignment().isOpen() || isCurriculumClosed;
if (totalStudentCount == 0) {
return StudyWeekStatisticsResponse.empty(
- studyDetail.getWeek(), isNotOpenedAssignment, isNotOpenedCurriculum);
+ studyDetail.getWeek(), isAssignmentClosed, isCurriculumClosed);
}
// ... 나머지 코드에서도 변수명 업데이트
}Likely invalid or redundant comment.
| @@ -82,4 +82,8 @@ public void complete() { | |||
| public boolean isWithinApplicationAndCourse() { | |||
| return study.isWithinApplicationAndCourse(); | |||
| } | |||
|
|
|||
| public boolean isComplete() { | |||
There was a problem hiding this comment.
| public boolean isComplete() { | |
| public boolean isCompleted() { |
Job Summary for GradleCheck Style and Test to Develop :: build-test
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java(3 hunks)src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java(1 hunks)
🔇 Additional comments (3)
src/main/java/com/gdschongik/gdsc/domain/study/domain/StudyHistory.java (1)
86-88: 구현이 깔끔하고 명확합니다!
isCompleted() 메서드는 스터디 통계 조회 API에서 수료율을 계산하는데 적절하게 활용될 수 있습니다. 메서드 명이 Java 컨벤션을 잘 따르고 있으며, 구현이 간단명료합니다.
src/main/java/com/gdschongik/gdsc/domain/study/application/MentorStudyDetailService.java (2)
Line range hint 3-44: 의존성 주입이 적절히 구성되었습니다!
새로운 통계 기능을 위한 레포지토리들이 잘 구성되어 있습니다.
155-181:
Null 안전성 확보 및 변수명 개선이 필요합니다.
getCurriculum()과getAssignment()의 null 체크가 누락되었습니다.isNotOpenedCurriculum과 같은 이중 부정 변수명은 코드 이해를 어렵게 만듭니다.
다음과 같이 수정을 제안드립니다:
- boolean isNotOpenedCurriculum = !studyDetail.getCurriculum().isOpen();
- boolean isNotOpenedAssignment = !studyDetail.getAssignment().isOpen() || isNotOpenedCurriculum;
+ boolean isCurriculumClosed = studyDetail.getCurriculum() == null || !studyDetail.getCurriculum().isOpen();
+ boolean isAssignmentClosed = studyDetail.getAssignment() == null || !studyDetail.getAssignment().isOpen() || isCurriculumClosed;Likely invalid or redundant comment.
| private long calculateAverageWeekAttendanceRate(List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | ||
|
|
||
| double averageAttendanceRate = studyWeekStatisticsResponses.stream() | ||
| .filter(weekStatisticsResponse -> !weekStatisticsResponse.isCurriculumCanceled()) | ||
| .mapToLong(StudyWeekStatisticsResponse::attendanceRate) | ||
| .average() | ||
| .orElse(0); | ||
|
|
||
| return Math.round(averageAttendanceRate); | ||
| } | ||
|
|
||
| private long calculateAverageWeekAssignmentSubmissionRate( | ||
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses) { | ||
|
|
||
| double averageAssignmentSubmissionRate = studyWeekStatisticsResponses.stream() | ||
| .filter(studyWeekStatistics -> !studyWeekStatistics.isAssignmentCanceled()) | ||
| .mapToLong(StudyWeekStatisticsResponse::assignmentSubmissionRate) | ||
| .average() | ||
| .orElse(0); | ||
|
|
||
| return Math.round(averageAssignmentSubmissionRate); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
평균 계산 로직의 중복을 제거해주세요.
출석률과 과제 제출률 계산 로직이 매우 유사합니다. 공통 메서드로 추출하여 코드 중복을 제거하는 것이 좋습니다.
다음과 같은 리팩토링을 제안드립니다:
private long calculateAverageRate(
List<StudyWeekStatisticsResponse> responses,
Predicate<StudyWeekStatisticsResponse> filterCondition,
ToLongFunction<StudyWeekStatisticsResponse> rateExtractor) {
return Math.round(responses.stream()
.filter(filterCondition)
.mapToLong(rateExtractor)
.average()
.orElse(0));
}
private long calculateAverageWeekAttendanceRate(List<StudyWeekStatisticsResponse> responses) {
return calculateAverageRate(
responses,
response -> !response.isCurriculumCanceled(),
StudyWeekStatisticsResponse::attendanceRate);
}
private long calculateAverageWeekAssignmentSubmissionRate(List<StudyWeekStatisticsResponse> responses) {
return calculateAverageRate(
responses,
response -> !response.isAssignmentCanceled(),
StudyWeekStatisticsResponse::assignmentSubmissionRate);
}| @Transactional(readOnly = true) | ||
| public StudyStatisticsResponse getStudyStatistics(Long studyId) { | ||
| Member currentMember = memberUtil.getCurrentMember(); | ||
| Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); | ||
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId); | ||
| List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId); | ||
| studyValidator.validateStudyMentor(currentMember, study); | ||
|
|
||
| long totalStudentCount = studyHistories.size(); | ||
| long studyCompletedStudentCount = | ||
| studyHistories.stream().filter(StudyHistory::isCompleted).count(); | ||
|
|
||
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses = studyDetails.stream() | ||
| .map((studyDetail -> calculateWeekStatistics(studyDetail, totalStudentCount))) | ||
| .toList(); | ||
|
|
||
| long averageAttendanceRate = calculateAverageWeekAttendanceRate(studyWeekStatisticsResponses); | ||
| long averageAssignmentSubmissionRate = | ||
| calculateAverageWeekAssignmentSubmissionRate(studyWeekStatisticsResponses); | ||
|
|
||
| return StudyStatisticsResponse.of( | ||
| totalStudentCount, | ||
| studyCompletedStudentCount, | ||
| averageAttendanceRate, | ||
| averageAssignmentSubmissionRate, | ||
| studyWeekStatisticsResponses); | ||
| } |
There was a problem hiding this comment.
Repository 조회 결과에 대한 Null 처리가 필요합니다.
studyHistories와 studyDetails 조회 결과가 null일 경우를 대비한 방어 로직이 필요합니다.
다음과 같이 수정하는 것을 제안드립니다:
- List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId);
- List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId);
+ List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId);
+ if (studyHistories == null) {
+ studyHistories = List.of();
+ }
+
+ List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId);
+ if (studyDetails == null) {
+ studyDetails = List.of();
+ }📝 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.
| @Transactional(readOnly = true) | |
| public StudyStatisticsResponse getStudyStatistics(Long studyId) { | |
| Member currentMember = memberUtil.getCurrentMember(); | |
| Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); | |
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId); | |
| List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId); | |
| studyValidator.validateStudyMentor(currentMember, study); | |
| long totalStudentCount = studyHistories.size(); | |
| long studyCompletedStudentCount = | |
| studyHistories.stream().filter(StudyHistory::isCompleted).count(); | |
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses = studyDetails.stream() | |
| .map((studyDetail -> calculateWeekStatistics(studyDetail, totalStudentCount))) | |
| .toList(); | |
| long averageAttendanceRate = calculateAverageWeekAttendanceRate(studyWeekStatisticsResponses); | |
| long averageAssignmentSubmissionRate = | |
| calculateAverageWeekAssignmentSubmissionRate(studyWeekStatisticsResponses); | |
| return StudyStatisticsResponse.of( | |
| totalStudentCount, | |
| studyCompletedStudentCount, | |
| averageAttendanceRate, | |
| averageAssignmentSubmissionRate, | |
| studyWeekStatisticsResponses); | |
| } | |
| @Transactional(readOnly = true) | |
| public StudyStatisticsResponse getStudyStatistics(Long studyId) { | |
| Member currentMember = memberUtil.getCurrentMember(); | |
| Study study = studyRepository.findById(studyId).orElseThrow(() -> new CustomException(STUDY_NOT_FOUND)); | |
| List<StudyHistory> studyHistories = studyHistoryRepository.findAllByStudyId(studyId); | |
| if (studyHistories == null) { | |
| studyHistories = List.of(); | |
| } | |
| List<StudyDetail> studyDetails = studyDetailRepository.findAllByStudyIdOrderByWeekAsc(studyId); | |
| if (studyDetails == null) { | |
| studyDetails = List.of(); | |
| } | |
| studyValidator.validateStudyMentor(currentMember, study); | |
| long totalStudentCount = studyHistories.size(); | |
| long studyCompletedStudentCount = | |
| studyHistories.stream().filter(StudyHistory::isCompleted).count(); | |
| List<StudyWeekStatisticsResponse> studyWeekStatisticsResponses = studyDetails.stream() | |
| .map((studyDetail -> calculateWeekStatistics(studyDetail, totalStudentCount))) | |
| .toList(); | |
| long averageAttendanceRate = calculateAverageWeekAttendanceRate(studyWeekStatisticsResponses); | |
| long averageAssignmentSubmissionRate = | |
| calculateAverageWeekAssignmentSubmissionRate(studyWeekStatisticsResponses); | |
| return StudyStatisticsResponse.of( | |
| totalStudentCount, | |
| studyCompletedStudentCount, | |
| averageAttendanceRate, | |
| averageAssignmentSubmissionRate, | |
| studyWeekStatisticsResponses); | |
| } |
🌱 관련 이슈
📌 작업 내용 및 특이사항
📝 참고사항
수료율의 경우, 스터디 수료 여부 필드를 이용하여 계산하였습니다.
스터디 평균 출석률과 과제 제출률은 각 주차별 출석률, 과제 제출률의 계산 결과를 평균하였습니다.
각 주차별 과제 제출률은 제출에 성공한 과제만 평균냈습니다.
평균을 낼 때 휴강주차는 평균 계산에서 제외하였습니다.
피그마에 화면 기획을 보면 '수료 스터디 인원 수' 와 '수료 가능한 스터디 인원 수' 정보가 필요한데, 혁님이 '수료 가능한 스터디 인원 수'는 스터디 전체 인원으로 고려하면 된다고 하셔서 response에 별도 필드를 만들지 않았습니다.
📚 기타
Summary by CodeRabbit
StudyStatisticsResponse및StudyWeekStatisticsResponse데이터 구조 도입.isCompleted()메소드 추가.isCompleted()메소드 추가.