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 17 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,81 @@ 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 isCanceledWeek = !studyDetail.getCurriculum().isOpen();
Copy link
Member

Choose a reason for hiding this comment

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

그리고 개인적인 생각인데 '스터다 주차' 라고 하면 수업과 과제 모두를 포괄하는 느낌이라서, 뭔가 좀 어색한 것 같네요. '해당 주차가 취소되었는지' 보다는 '해당 주차 수업 / 과제가 취소되었는지' 로 나타내는게 더 정확하지 않나 싶네요 (cc @Sangwook02 )

Copy link
Member Author

Choose a reason for hiding this comment

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

week 가 포괄적인 느낌이라는데 공감합니다. 변수명 고민을 좀 더 해봐야겠네요..

Copy link
Member

Choose a reason for hiding this comment

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

isCanceled 메서드가 있는데 isOpen을 쓰신 이유가 있나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

저도 그걸로 고민을 했었는데, 지금 존재하는 상태가 open, cancel, none 이렇게 3가지가 있어서, isCanceled 를 사용하면 none 으로 설정된 studyDetail 도 열린 주차로 인식하는 문제가 발생할 것 같았어요. 그래서 cancel, none 타입인 경우 모두 canceled week 로 간주하도록 했습니다.

Copy link
Member

Choose a reason for hiding this comment

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

아 open이 아닌지만 확인하면 되는거였군요
그럼 변수명을 수정하면 좋을 것 같아요!

boolean isCanceledAssignment = !studyDetail.getAssignment().isOpen() || isCanceledWeek;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

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


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 =
Copy link
Member

Choose a reason for hiding this comment

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

제출 성공인 과제의 수를 count하는 것 같은데 변수명을 보고는 알기 어려운 것 같습니다!

assignmentHistoryRepository.countByStudyDetailIdAndSubmissionStatusEquals(studyDetail.getId(), SUCCESS);
long assignmentRate = Math.round(assignmentCount / (double) totalStudentCount * 100);

return StudyWeekStatisticsResponse.opened(studyDetail.getWeek(), attendanceRate, assignmentRate);
kckc0608 marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

calculateWeekStatistics 메서드의 가독성 향상을 위한 리팩토링 제안

현재 calculateWeekStatistics 메서드에서 여러 return 문이 존재하여 흐름을 따라가기 어려울 수 있습니다. 가독성을 높이기 위해 조건 분기를 구조화하거나, 메서드를 분리하여 코드의 명확성을 높이는 것을 고려해보세요.


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

}
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,27 @@
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 isCanceledWeek) {

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);
}
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. week가 null이거나 음수인 경우
  2. attendanceRateassignmentSubmissionRate가 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.

}