Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: 스터디 통계 조회 API 추가 #809

Merged
merged 20 commits into from
Nov 1, 2024
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import com.gdschongik.gdsc.domain.study.dto.response.AssignmentResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyCurriculumResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyMentorAttendanceResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyStatisticsResponse;
import io.swagger.v3.oas.annotations.Operation;
import io.swagger.v3.oas.annotations.tags.Tag;
import jakarta.validation.Valid;
Expand Down Expand Up @@ -80,4 +81,11 @@ public ResponseEntity<List<StudyMentorAttendanceResponse>> getAttendanceNumbers(
List<StudyMentorAttendanceResponse> response = mentorStudyDetailService.getAttendanceNumbers(studyId);
return ResponseEntity.ok(response);
}

@Operation(summary = "스터디 통계 조회", description = "멘토가 자신의 스터디 출석률, 과제 제출률, 수료율에 대한 통계를 조회합니다. 휴강 주차는 계산에서 제외합니다.")
@GetMapping("/statistics")
public ResponseEntity<StudyStatisticsResponse> getStudyStatistics(@RequestParam(name = "studyId") Long studyId) {
StudyStatisticsResponse response = mentorStudyDetailService.getStudyStatistics(studyId);
return ResponseEntity.ok(response);
}
}
Original file line number Diff line number Diff line change
@@ -1,15 +1,25 @@
package com.gdschongik.gdsc.domain.study.application;

import static com.gdschongik.gdsc.domain.study.domain.AssignmentSubmissionStatus.SUCCESS;
import static com.gdschongik.gdsc.global.exception.ErrorCode.*;

import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.domain.study.dao.AssignmentHistoryRepository;
import com.gdschongik.gdsc.domain.study.dao.AttendanceRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyDetailRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyHistoryRepository;
import com.gdschongik.gdsc.domain.study.dao.StudyRepository;
import com.gdschongik.gdsc.domain.study.domain.Study;
import com.gdschongik.gdsc.domain.study.domain.StudyDetail;
import com.gdschongik.gdsc.domain.study.domain.StudyDetailValidator;
import com.gdschongik.gdsc.domain.study.domain.StudyHistory;
import com.gdschongik.gdsc.domain.study.domain.StudyValidator;
import com.gdschongik.gdsc.domain.study.dto.request.AssignmentCreateUpdateRequest;
import com.gdschongik.gdsc.domain.study.dto.response.AssignmentResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyCurriculumResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyMentorAttendanceResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyStatisticsResponse;
import com.gdschongik.gdsc.domain.study.dto.response.StudyWeekStatisticsResponse;
import com.gdschongik.gdsc.global.exception.CustomException;
import com.gdschongik.gdsc.global.util.MemberUtil;
import java.time.LocalDate;
Expand All @@ -27,6 +37,11 @@ public class MentorStudyDetailService {
private final MemberUtil memberUtil;
private final StudyDetailRepository studyDetailRepository;
private final StudyDetailValidator studyDetailValidator;
private final StudyHistoryRepository studyHistoryRepository;
private final AttendanceRepository attendanceRepository;
private final AssignmentHistoryRepository assignmentHistoryRepository;
private final StudyValidator studyValidator;
private final StudyRepository studyRepository;

@Transactional(readOnly = true)
public List<AssignmentResponse> getWeeklyAssignments(Long studyId) {
Expand Down Expand Up @@ -108,4 +123,83 @@ public List<StudyMentorAttendanceResponse> getAttendanceNumbers(Long studyId) {
.limit(2)
.toList();
}

@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::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);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

성능 최적화 및 방어 로직 추가 제안

다음과 같은 개선사항을 제안드립니다:

  1. studyHistories가 null일 경우를 대비한 방어 로직이 필요합니다.
  2. 데이터베이스 쿼리 최적화를 위해 studyHistoriesstudyDetails를 한 번의 쿼리로 조회하는 것이 좋습니다.
@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 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);
}
Comment on lines +155 to +181
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Null 체크 추가 및 변수명 개선 필요

  1. getCurriculum()getAssignment()의 null 체크가 필요합니다.
  2. isNotOpenedCurriculumisNotOpenedAssignment 변수명이 부정문을 사용하여 이해하기 어렵습니다.
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.


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);
}
Comment on lines +183 to +204
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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);
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,14 @@

import com.gdschongik.gdsc.domain.member.domain.Member;
import com.gdschongik.gdsc.domain.study.domain.AssignmentHistory;
import com.gdschongik.gdsc.domain.study.domain.AssignmentSubmissionStatus;
import com.gdschongik.gdsc.domain.study.domain.StudyDetail;
import java.util.Optional;
import org.springframework.data.jpa.repository.JpaRepository;

public interface AssignmentHistoryRepository
extends JpaRepository<AssignmentHistory, Long>, AssignmentHistoryCustomRepository {
Optional<AssignmentHistory> findByMemberAndStudyDetail(Member member, StudyDetail studyDetail);

long countByStudyDetailIdAndSubmissionStatusEquals(Long studyDetailId, AssignmentSubmissionStatus status);
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,4 +5,6 @@

public interface AttendanceRepository extends JpaRepository<Attendance, Long>, AttendanceCustomRepository {
boolean existsByStudentIdAndStudyDetailId(Long studentId, Long studyDetailId);

long countByStudyDetailId(Long studyDetailId);
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,8 @@ public void complete() {
public boolean isWithinApplicationAndCourse() {
return study.isWithinApplicationAndCourse();
}

public boolean isComplete() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public boolean isComplete() {
public boolean isCompleted() {

return studyHistoryStatus == COMPLETED;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.gdschongik.gdsc.domain.study.dto.response;

import io.swagger.v3.oas.annotations.media.Schema;
import java.util.List;

public record StudyStatisticsResponse(
@Schema(description = "스터디 전체 수강생 수") Long totalStudentCount,
@Schema(description = "스터디 수료 수강생 수") Long completeStudentCount,
@Schema(description = "평균 출석률") Long averageAttendanceRate,
@Schema(description = "평균 과제 제출률") Long averageAssignmentSubmissionRate,
@Schema(description = "스터디 수료율") Long studyCompleteRate,
@Schema(description = "주차별 출석률 및 과제 제출률") List<StudyWeekStatisticsResponse> 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);
}
Comment on lines +14 to +27
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

팩토리 메서드의 안전성 개선이 필요합니다.

현재 구현에서 다음과 같은 잠재적인 문제가 있습니다:

  1. completeStudentCount가 null일 경우 NPE 발생 가능
  2. completeStudentCounttotalStudentCount보다 큰 경우에 대한 검증 부재

다음과 같은 개선을 제안드립니다:

 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.

Suggested change
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);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package com.gdschongik.gdsc.domain.study.dto.response;

import io.swagger.v3.oas.annotations.media.Schema;

public record StudyWeekStatisticsResponse(
@Schema(description = "스터디 주차") Long week,
@Schema(description = "출석률") Long attendanceRate,
@Schema(description = "과제 제출률") Long assignmentSubmissionRate,
@Schema(description = "과제 휴강 여부") boolean isAssignmentCanceled,
@Schema(description = "수업 휴강 여부") boolean isCurriculumCanceled) {

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 isAssignmentCanceled, boolean isCurriculumCanceled) {
return new StudyWeekStatisticsResponse(week, 0L, 0L, isAssignmentCanceled, isCurriculumCanceled);
}

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);
}
}