-
Notifications
You must be signed in to change notification settings - Fork 0
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
[DEV-18] Completed Credit #247
Conversation
major -> primaryMajor
졸업요건 계산 시점에서 생성 시점으로 변경
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #247 +/- ##
=============================================
- Coverage 84.22% 83.28% -0.95%
- Complexity 479 492 +13
=============================================
Files 133 141 +8
Lines 1883 1998 +115
Branches 64 65 +1
=============================================
+ Hits 1586 1664 +78
- Misses 261 297 +36
- Partials 36 37 +1
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
|
||
public interface GenerateOrModifyCompletedCreditPort { | ||
|
||
void generateOrModifyCompletedCredits(User user, List<CompletedCredit> completedCredits); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분만 분리되면 될거 같습니다! 수고하셨어요!
public void generateOrModifyCompletedCredit(User user, GraduationResult graduationResult) { | ||
List<CompletedCredit> completedCredits = findCompletedCreditPort.findCompletedCredit(user); | ||
List<DetailGraduationResult> detailGraduationResults = graduationResult.getDetailGraduationResults(); | ||
|
||
Map<DetailGraduationResult, Optional<CompletedCredit>> resultMap = detailGraduationResults.stream() | ||
.collect(Collectors.toMap( | ||
Function.identity(), | ||
detailGraduationResult -> completedCredits.stream() | ||
.filter(completedCredit -> completedCredit.getGraduationCategory().equals(detailGraduationResult.getGraduationCategory())) | ||
.findFirst() | ||
)); | ||
|
||
List<CompletedCredit> completedCreditModels = resultMap.keySet().stream() | ||
.map(completedCredit -> createCompletedCreditModel(completedCredit, resultMap.get(completedCredit))) | ||
.collect(Collectors.toList()); | ||
|
||
CompletedCredit chapelCompletedCreditModel = createChapelCompletedCreditModel(completedCredits, | ||
graduationResult); | ||
CompletedCredit normalCultureCompletedCreditModel = createNormalCultureCompletedCreditModel(completedCredits, | ||
graduationResult); | ||
CompletedCredit freeElectiveCompletedCreditModel = createFreeElectiveCompletedCreditModel(completedCredits, | ||
graduationResult); | ||
|
||
ArrayList<CompletedCredit> allCompletedCreditModels = new ArrayList<>(completedCreditModels); | ||
allCompletedCreditModels.addAll( | ||
List.of(chapelCompletedCreditModel, normalCultureCompletedCreditModel, freeElectiveCompletedCreditModel)); | ||
generateOrModifyCompletedCreditPort.generateOrModifyCompletedCredits(user, allCompletedCreditModels); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detailGraduationResult에서 GraduationCategory별로 Mapping을 해놓고 시작하면 더 좋을것 같다는 생각!
Map<GraduationCategory, DetailGraduationResult> 이런 방식으로?
그리고 CompletedCredit 리스트 순회하면서 GraduationCategory로 해당 맵에 있으면 업데이트하고 만약 없으면 따로 저장해놨다가 나중에 추가.
그러면 채플이나 일반, 자유 같은 경우는 굳이 completedCredits 순회 안돌고 바로 가져올 수 있고(키값으로 바로 가져오면 되니까), 가장 처음 resultMap만드는 것도 두번 안돌고, 로직더 가독성이 좋아질것 같은느낌?
수환이하고 경호 생각은 어때? 이 방법도 가능할 것 같아?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 고민을 좀 해봤었는데 Map<GraduationCategory, DetailGraduationResult> 로 매핑이 힘든게 일반교양, 자유선택, 채플 졸업 결과는 타입이 각각 다 달라서 맵으로 매핑을 해놓을 수가 없었습니다.. 현재로서는 전체적인 ClaculateGraduationUseCase의 수정 없인 위 형태로 사전에 매핑해두는게 어려울거 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반, 자유, 채플의 졸업결과도 결국 DetailGraduationResult에서 찾는거 아니야? DetailGraduationResult에 gradautionGategroy가 있는거고?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일반교양, 자유선택, 채플은 DetailGraduationResult가 아니라 NormalCultureGraduationResult, FreeElectiveGraduationResult, ChapelResult가 따로 있어요
private CompletedCredit createCompletedCreditModel(DetailGraduationResult detailGraduationResult, | ||
Optional<CompletedCredit> completedCredit) { | ||
return CompletedCredit.builder() | ||
.id(completedCredit.map(CompletedCredit::getId).orElse(null)) | ||
.totalCredit(detailGraduationResult.getTotalCredit()) | ||
.takenCredit(detailGraduationResult.getTakenCredit()) | ||
.graduationCategory(detailGraduationResult.getGraduationCategory()) | ||
.build(); | ||
} | ||
|
||
private CompletedCredit createChapelCompletedCreditModel(List<CompletedCredit> completedCredits, | ||
GraduationResult graduationResult) { | ||
return completedCredits.stream() | ||
.filter(completedCredit -> completedCredit.getGraduationCategory() == CHAPEL) | ||
.map(completedCredit -> CompletedCredit.builder() | ||
.id(completedCredit.getId()) | ||
.totalCredit(ChapelResult.GRADUATION_COUNT) | ||
.takenCredit(graduationResult.getChapelResult().getTakenCount()) | ||
.graduationCategory(CHAPEL).build()) | ||
.findFirst() | ||
.orElse(CompletedCredit.builder() | ||
.totalCredit(ChapelResult.GRADUATION_COUNT) | ||
.takenCredit(graduationResult.getChapelResult().getTakenChapelCredit()) | ||
.graduationCategory(CHAPEL).build()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
createCompletedCreditModel 메서드 류는 CompletedCreaditGenerator혹은 Creator로 해서 분리하는게 어때?
해당 클래스에서 switch문으로 GraduationCateory에 따라서 분기처리하는거지.
그리고 CompletedCredit을 생성하는 책임을 CompletedCredit에 넘겨주어도 좋을 것 같고. 만약에 CompeltedCredit 필드면 하나 수정되는 순간 바로 이 클래스까지 수정 범위가 커지기도 하니까!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 저도 생각해봤던건데 저 3가지 케이스를 switch문으로 분기처리하여 생성해주는게 과연 가독성이 더 좋을지는 잘 모르겠습니다. 다만 말씀대로 CompletedCredit 객체를 생성해주는 책임은 CompletedCredit에 두는게 훨씬 좋을거 같네요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저도 이 코드에 중복이 많아서 분기하는게 좋을지 따로 빼는게 좋을지 고민했는데, 개인적으로는 각각에 필요한게 조금씩 다르다보니 코드가 흐름의 중복은 조금 해결될지라도 전반적인 가독성이 해소가 될거 같지는 않다는 생각이 들었어요!
그리고 저도 생성은 CompletedCredit에게 넘기는게 좋아보입니다!!
import com.plzgraduate.myongjigraduatebe.core.meta.UseCase; | ||
import com.plzgraduate.myongjigraduatebe.graduation.application.usecase.CalculateGraduationUseCase; | ||
import com.plzgraduate.myongjigraduatebe.graduation.domain.model.GraduationResult; | ||
import com.plzgraduate.myongjigraduatebe.lecture.application.usecase.FindLecturesUseCase; | ||
import com.plzgraduate.myongjigraduatebe.lecture.domain.model.Lecture; | ||
import com.plzgraduate.myongjigraduatebe.takenlecture.application.port.SaveTakenLecturePort; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이거 지금 밑에 더 드래그가 안되네 SaveTakenLectureFromParsingText거든?
이 클래스가 takenLecture에 있는거 다시 생각해볼 있는것 같아. 이제 calculateGraduation 메서드와 generateOrModifyCompletedCredit가 추가가 되었으니까 더 이상 saveTakenLectures메서드가 아니게 된거니까.
그리고 calculateGraduation메서드가 추가가 되면서 takenLecture -> graduation으로 모듈간 의존성이 새로 생겼는데 기존 graduation -> takenLecture로 모듈 의존성이 었어가지고 모듈간의 양방향 참조가 되어버렸어.
이 부분만(현재 커진 saveTakenLectures메서드의 기능들) 상위 UseCase를 만들어서 하는게 좋을 것 같은데 수환 경호 생각은 어때?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분에 대해서는 저도 생각해보지 못했던 부분인데 짚어주셔서 감사합니다!
확실히 SaveTakenLectureFromParsingTextUseCase의 책임이 필요 이상으로 커진 거 같습니다! CalculateGraduationUseCase와 GenerateOrModifyCompletedCreditUseCase는 SaveTakenLectureFromParsingTextUseCase과는 무관하게 ParsingTextUseCase 수행 시 수행하는게 모듈간의 양방향 참조도 해결할 수 있을 거 같습니다!
추가로 떠오른 아이디어인데 성적표 pdf를 검토해봤는데 다음과 같이 이수 학점을 종합하여 계산해놓은 항목이 있는걸 발견했습니다! 각 이수구분별 정확하게 해당 이수 학점이 정확한지 체크를 더 해봐야겠지만 정확하다면 기존의 CalculateGraduationUseCase를 사용하지 않고 해당 이수 학점 텍스트만 파싱하여 CompletedCredit 로직을 수행할 수 있을거 같습니다! 그렇다면 앞서 리뷰해주셨던 GraduationResult로부터 CompletedCredit 객체를 생성하는데 우려했던 점들도 같이 SaveTakenLectureFromParsingText의 UseCase 범위 초과에 모듈간 의존성 문제도 한번에 해결될 거 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오.. 그러네요.! 수환형님 그럼 저 파싱데이터 사용하게 되면 이수 학점에 대한 전체 로직을 한번 더 안 돌고 프론트에게 넘겨줄 수 있게 되는걸까요??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그런걸로 예상했긴 했는데 살짝 문제되는 점이 해당 정보에선 전공필수와 선택이 나뉘지가 않네..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
근데 이거하고 우리하고 산정방식이 조금 다른부분이 뭐냐면 학교는 자유선택으로 옮기지는 않아. 우리는 전공 70학점이 맥스인데 72학점 들으면 2학점 자율전공으로 넘어가잖아. 저거는 안넘어감. 따로 계산을 해주긴 해야해.
또 걱정되는건 이제 올바르게 매핑을 학교가 할까?야. 저번에도 예외사항같은거 있었는데 학교는 그냥 카테고리 맞으면 집어넣지 않나?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
맞습니다.ㅜㅜ 이 방법은 결국 실현이 안되는게 정확도 문제도 있고 전공 필수/선택이 구분이 안되는 것과 복수전공의 경우는 학문기초교양도 주전공, 복수전공을 구분해야 하는데 그 부분도 구분이 안되더라고요.. 그래서 전체 졸업 계산은 필수적인거 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
일단 때에 따라서 insert/update가 분기처리 되는 부분에 있어서는 지금 처럼 해도 좋은 것 같아. 굳이 insert와 update를 나눌 필요가 없어보여.
개선할 점은 지금처럼 saveAll 메서드를 사용한다면 해당하는 CompleteCredit 칼럼에 대해 insert/update 쿼리가 개수만큼 날아가게 될텐데
이걸 Bulk Insert로 변경하면 더 성능개선 포인트가 될 수 있을 것 같아.(삽입해야하는 레코드가 많은 테이블이 아니고 앞으로도 많아질 요소는 거의 없어서 굳이라면 굳이긴하지만). 참고로 insert에 on duplicate key update를 추가하면를 DB레벨에서 insert/update처리할 수 있거든. 차후에 시도해보면 좋을 것 같아.
CompletedCredit 모델을 생성할 때 Service클래스에 로직이 집중되어 있는 문제에 대한 내 의견은 위의 커멘트에 달아놨어.
수환이 말대로 우리의 테스트 방식도 따로 문서화 해놓고 통일시키면 좋을듯!
시간에 좀 쫒기느라 Fixture사용하는것도 애매해진 포인트들도 있고, AutoIncrement와 Transaction조합으로 주의할 점도 있으니!
도메인 <-> Jpa엔티티 간의 변환 메서드들도 그냥 한번에 싹다 만들어놔도 좋을 것 같고.
insert(generate)와 update(modify) UseCase를 분리하는것을 고려한 건 ParsingText를 통해 TakenLecture가 업데이트 되는 경우는 generate와 modify가 동시에 일어날 가능성이 존재하지만, 수강과목 커스텀을 통해 TakenLecture가 업데이트 되는 경우는 modify만 수행되기 때문에 UseCase의 재활용성을 생각해서 분리하는 방향으로 생각했던 것이었습니다! 어떻게 생각하시나요??
확인했습니다!
👍 |
오 saveAll은 내부적으로 save가 여러번 나가지만 bulk는 하나의 쿼리로 여러 insert가 가능하군요.! 새로운거 알고 갑니다.! https://velog.io/@joonghyun/Spring-JPA-Save-vs-SaveAll-vs-Bulk-Insert
이 부분에 대해 확실히 생각하지 못한 부분인데 집고 넘어가주셔서 저녁에 코드 보면서 다시 생각해보고 의견 남기겠습니다! |
근데 사실 희박하긴하지만 저학년일 경우에는 커스텀만으로도 CompleteCredit이 생성될수 있지 않아?? |
수강과목 커스텀(마이페이지)을 하기 위해선 무조건 성적표 업데이트를 완료한 이후라 반드시 CompletedCredit이 생성되어 있기 때문에 해당 케이스는 고려하지 않았습니다! |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
수고하셨습니다!
Issue
Close #246
✅ 작업 내용
🤔 고민 했던 부분
🔊 도움이 필요한 부분!!