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

[Refactor] Home 리팩토링 #80

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

[Refactor] Home 리팩토링 #80

wants to merge 12 commits into from

Conversation

LeeJoEun-01
Copy link
Contributor

@LeeJoEun-01 LeeJoEun-01 commented Jan 26, 2025

📌 Summary

  • HomeView UI 수정
    • 매개변수가 필요한 UI는 -> Struct로 제작
    • 간단한 UI(매개변수가 필요 X) -> private extension에서 변수로 제작
  • HomeReducer 제작하고 HomeView에 적용해두었습니다.

✍️ Description

💡 PR Point

  • 추후 Rusaint 프로젝트 코드 컨벤션을 작성할 필요가 있을 것 같아요.

  • Sendable과 관련해서 질문 있어서 Comment 남겨두었습니다.

  • 기존에 GradeClient에서 reportCard 정보를 가져오는 부분과 CoreData에 저장하는 부분이 분리되어 있었는데 굳이 분리할 필요가 없다고 판단하여 합쳐두었습니다.

  • 이제 HomeViewModel, HomeRepository는 삭제해도 되겠죠?

📚 Reference

🔥 Test

@LeeJoEun-01 LeeJoEun-01 added the refactor🪄 리팩토링 label Jan 26, 2025
@LeeJoEun-01 LeeJoEun-01 self-assigned this Jan 26, 2025
Comment on lines +44 to +63
setTotalReportCard: {
let session = try await studentClient.createSaintSession()
let courseGrades = try await CourseGradesApplicationBuilder().build(session: session).certificatedSummary(courseType: .bachelor)
let graduationRequirement = try await GraduationRequirementsApplicationBuilder().build(session: session).requirements()
let requirements = graduationRequirement.requirements.filter { $0.value.name.hasPrefix("학부-졸업학점") }
.compactMap { $0.value.requirement ?? 0}
if let graduateCredit = requirements.first {
let context = coreDataStack.taskContext()
createTotalReportCard(gpa: courseGrades.gradePointsAvarage, earnedCredit: courseGrades.earnedCredits, graduateCredit: Float(graduateCredit), in: context)

context.performAndWait {
do {
try context.save()
} catch {
print("update [TotalReportCard] error : \(error)")
}
}
}

return
Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존에 GradeClient에서 reportCard 정보를 가져오는 부분과 CoreData에 저장하는 부분이 분리되어 있었는데 굳이 분리할 필요가 없다고 판단하여 합쳐두었습니다.

Copy link
Contributor

@jayden000106 jayden000106 Jan 31, 2025

Choose a reason for hiding this comment

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

회의 때 원래대로 돌리기로 결정했습니다.

var fetchAllSemesterGrades: @Sendable () async throws -> [SemesterGrade]
var fetchGrades: @Sendable (_ year: Int, _ semester: SemesterType) async throws -> [ClassGrade]


var getTotalReportCard: () async throws -> TotalReportCard
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sendable은 데이터를 여러 스레드 간 안전하게 전달하기 위해 사용하는 프로토콜이라고 알고 있습니다.
근데 위해 예시처럼 데이터에 접근하여 따로 어떤 액션이 발생하는 것이 아니라 단순히 정보를 가져오기만 하는 클로저에도 @Sendable 키워드를 붙이는게 맞을까요?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dependency에 대해 완벽하게 이해하진 못해서, 틀릴 수도 있는 부분인데,
@Sendable는 구조체와 같은 Value Type을 여러 스레드 간 전달 등 안정성을 위해서 사용하는 프로토콜로 알고 있습니다.
이전에 CoreData 타입에서는 작성하지 않았던 이유는 해당 타입이 클래스라 그런 것이라, 여기에서는 사용하는게 적절하지 않을까요?

@LeeJoEun-01 LeeJoEun-01 changed the title [Refactor] [Refactor] Home 리팩토링 Jan 26, 2025
})
}
// MARK: - GradeSection
struct GradeInfo: View {
Copy link
Contributor

Choose a reason for hiding this comment

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

GradeInfo 부분도 Setting에서 이야기했던 것처럼 파일을 분리하는 건 어떻게 생각해요?
꽤 큰 View이고, 추후에 해당 화면에 기능이 늘어나면 위젯별로 파일을 나누는게 가독성이나 관리 측면에서 좋을 것 같아서 제안드려요!

Comment on lines +82 to +89
VStack(spacing: 0) {
HStack {
Text("내 성적")
.font(YDSFont.title3)
Spacer()
}
.padding(.leading, 4)
.padding(.top, 20)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VStack(spacing: 0) {
HStack {
Text("내 성적")
.font(YDSFont.title3)
Spacer()
}
.padding(.leading, 4)
.padding(.top, 20)
VStack(alignment: .leading, spacing: 0) {
Text("내 성적")
.font(YDSFont.title3)
.padding(.leading, 4)
.padding(.top, 20)

HStack을 사용하지 않고, 상위 VStack의 alignment로 위치를 조정하는 방법도 있어요!
아래 화면들이 모두 전체를 차지해서 이 방법도 가능합니다.

.resizable()
.clipShape(Circle())
.frame(width: 48, height: 48)
.padding(.leading, 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.padding(.leading, 0)

해당 padding의 역할이 있나요?

Comment on lines +91 to +104
HStack {
Image("ppussung")
.resizable()
.clipShape(Circle())
.frame(width: 48, height: 48)
.padding(.leading, 0)
VStack(alignment: .leading) {
Text("전체 학기")
.font(YDSFont.body2)
.padding(.bottom, -2.0)
Text("성적 확인하기")
.font(YDSFont.subtitle1)
}
.padding(.leading, 10)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
HStack {
Image("ppussung")
.resizable()
.clipShape(Circle())
.frame(width: 48, height: 48)
.padding(.leading, 0)
VStack(alignment: .leading) {
Text("전체 학기")
.font(YDSFont.body2)
.padding(.bottom, -2.0)
Text("성적 확인하기")
.font(YDSFont.subtitle1)
}
.padding(.leading, 10)
HStack(spacing: 14) {
Image("ppussung")
.resizable()
.clipShape(Circle())
.frame(width: 48, height: 48)
.padding(.leading, 0)
VStack(alignment: .leading) {
Text("전체 학기")
.font(YDSFont.body2)
.padding(.bottom, -2.0)
Text("성적 확인하기")
.font(YDSFont.subtitle1)
}

padding 대신 spacing을 활용할 수 있을 것 같아요! default 값이 4였던 것 같아서 14로 넣었습니다

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor🪄 리팩토링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Refactor] Home 리팩토링
2 participants