Conversation
Test Results190 tests 190 ✅ 26s ⏱️ Results for commit 9115650. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9115650370
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Map<MajorScope, List<GraduationCategory>> groupedReallocateTargets = graduationCategories.stream() | ||
| .filter(this::isReallocateTarget) | ||
| .collect(groupingBy(GraduationCategory::majorScope)); |
There was a problem hiding this comment.
Send double-major overflow to the shared GENERAL category
If a double-major student needs surplus 복필/복선 credits from the secondary major to satisfy the general-credit requirement, grouping reallocation targets by majorScope makes that overflow land in a synthetic GENERAL(SECONDARY) bucket instead of the real shared GENERAL(PRIMARY) bucket. GraduationChecker.buildDoubleMajorCriteria() only loads one set of non-major criteria, and CompletedCourseDto.determineMajorScope() marks non-major courses as PRIMARY, so the primary GENERAL row is the only one that should absorb this carry. In that flow the response can still show GENERAL(PRIMARY) short and keep canGraduate=false even though policy says those extra major credits should count toward general credits.
Useful? React with 👍 / 👎.
goldm0ng
left a comment
There was a problem hiding this comment.
교양 정책이 확실하지 않아서 고민 많으셨을텐데, 고생 많으셨습니다!!!
덱스님 의견에 동의해서 그 부분에 대한 코멘트 하나 남겼습니당
| if (!hasGeneral) { | ||
| general = GraduationCategory.createEmptyGraduationCategory( | ||
| majorScope, | ||
| CategoryType.GENERAL | ||
| ); | ||
| } |
There was a problem hiding this comment.
저도 이 부분 코멘트 남깁니다!
GENERAL은 사용자의 전공 유형에 따라 달라지는 이수구분은 아니라고 이해하고 있습니다.
기준 데이터 기준으로 보면, 전공 영역 이수구분인 전필/전선/복필/복선은 MajorScope에 따라 구분되지만, 교양 영역은 전공 유형과 무관하게 PRIMARY로 적재되어 있습니다!
그래서 현재 코드처럼 GENERAL까지 MajorScope 기준으로 함께 나누어 처리하는 방식은 조금 우려가 됩니당
- 단일 전공일 경우, 문제가 없다고 판단됩니다!
- 복수 전공일 경우, 우려 되는 지점이 있습니다.
EX)
A. 주전공에서 전필 -> 전선 -> 교양 순으로 학점 이월이 발생하는 경우
B. 복수전공에서 복필 -> 복선 -> 교양 순으로 학점 이월이 발생하는 경우
즉, 주전공 및 복수전공에서 모두 교양으로의 이월 학점이 존재할 경우입니다!
현재처럼 MajorScope별로 나누어 계산하면, 최종적으로 교양 학점이 하나의 공통 GENERAL(PRIMARY)로 합산되지 않고 scope별로 따로 처리될 가능성이 있어 보여요.
그렇게 되면 실제로는 교양 부족분을 메워야 하는 학점이 분산되어 들어가거나, 의도하지 않은 fallback category가 생길 수도 있을 것 같습니다.
개선 방향을 생각해보았는데, 만약 위에 언급드린 내용이 문제가 된다면!!!
->
전필/전선(복필/복선)까지만 기존대로 scope별로 계산한 뒤,
각 scope에서 최종적으로 남는 초과 학점을 마지막에 공통 GENERAL(PRIMARY)로 합산하는 방식으로 개선해볼 수 있을 것 같습니다!!!
모두들 어떻게 생각하시는지 궁금합니당
There was a problem hiding this comment.
아 헐 그러네~~~~~~~~~~~~~~~
한 사이클에 전필 -> 전선 -> 교양 보다는 전필 -> 전선 / 전선 -> 교양 이렇게 사이클 끊는게 낫겠다싶네요
그런데 하나 더 궁금한게 있는데, 기존에 GENERAL은 어떤식으로 됐던건가요? 제 성적표에 '교양' 이 있는데, 응답에 GENERAL 필드가 없었던 것 같아서요
|
이외에도 피알 본문에 적어주신 내용, 더 생각나는 논의 지점에 대해 추가로 코멘트 남깁니다! 1. fallback 카테고리의 requiredCredits=0 처리 방식이 괜찮은지?
수민님 말씀대로, 리팩 지점으로 두고 리팩에서도 우선순위가 높은 작업으로 두면 좋겠습니다!! 지금은 응답을 만드는 단계에서 재배분 계산을 하고 있어서, 예를 들어 실제로는 전선을 9학점 채워야 하는 학생이 있다고 가정했을 때, 따라서, 장기적으로는 이 로직이 실제 기준 학점( 2. 학번별로 교양 카테고리 구조가 다른 점도 고려가 필요해 보입니다!현재 로직은 전선 초과 학점을 일괄적으로
위와 같은 경우, 전선 초과 학점의 최종 귀속 대상도 학번에 따라 달라질 수 있을 것 같습니다! [확인이 필요한 것] 3. 재배분 로직을
|
작업 내용
기존에는 전공필수(MAJOR_REQUIRED) 초과 학점을 전공선택(MAJOR_ELECTIVE)으로만 넘겨주고 있었습니다.
이번 작업에서는 여기에 더해, 전공선택도 초과할 경우 교양(GENERAL)으로 넘겨주는 로직을 추가했습니다.
즉 현재 보정 순서는 아래와 같습니다.
MAJOR_REQUIRED->MAJOR_ELECTIVEMAJOR_ELECTIVE->GENERAL재배분 대상은 MAJOR_REQUIRED, MAJOR_ELECTIVE, GENERAL 3개입니다.
findCategory()는 현재 결과 카테고리 목록에서 해당 타입이 실제로 존재하는지 확인합니다.
없으면 null을 반환하고, 이월 학점을 계산해야 하는 경우에만 createEmptyGraduationCategory()로 계산용 fallback 객체를 생성합니다.
이 fallback은 학점을 담기 위한 임시 객체이고, 현재는 requiredCredits=0으로 생성됩니다.
즉 “실제 기준 학점이 0”이라기보다는, “기준 정보를 별도로 조회하지 않았기 때문에 0으로 채워둔 상태”입니다.
보정 후에는:
원래 존재하던 카테고리는 그대로 유지하고
원래 없던 카테고리라도 실제 이월 학점이 생긴 경우에만 결과 DTO에 추가합니다
반대로 이월 후에도 값이 없으면 DTO에는 추가하지 않습니다
예를 들어 전필 초과분이 전선으로는 넘어가지만, 전선에서 다시 교양으로 넘길 초과분이 없다면 GENERAL은 응답에 추가하지 않습니다.
그런데 다음과 같은 문제가 있습니다.
현재 로직은 응답 DTO 생성 단계에서 동작합니다.
그래서 카테고리가 원래 없을 경우, fallback 객체는 만들 수 있어도 실제 requiredCredits 기준은 알 수 없습니다.
예를 들어 전필을 매우 많이 이수했고 전선/교양 이력이 아예 없는 경우:
전필 초과분을 전선으로 넘기는 것은 가능하지만
전선 기준학점까지 초과했는지 정확히 판단하려면 실제 기준 정보 조회가 필요합니다
즉 이 문제를 완전히 해결하려면 CreditCriterion 등 실제 기준 학점을 함께 참조하는 방향의 리팩토링이 필요할 수 있습니다.
테스트
아래 케이스를 테스트로 추가했습니다.
로컬 테스트 결과는 다음과 같습니다. 제 성적표에서 교선 3학점 짜리 두개, 단순 교양 한개를 전선으로 바꾸고 돌려봤을때, 기존에는 하단과 같이 23학점 이수였던 교선 학점이,

아래와 같이 17학점으로 줄었습니다.

그리고 GENERAL 필드가 생기면서 9학점으로 계산이 되었습니다.

고민 지점과 리뷰 포인트
테스트도 추가 했는데, 그럼에도 정말 꼼꼼꼼꼼꼼꼼한 리뷰가 필요합니다.