-
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
[#17] CollectionView 및 ViewModel 리팩토링 #18
base: main
Are you sure you want to change the base?
Conversation
letskooo
commented
Jan 2, 2025
- 기존 CollectionView를 CompositionalLayout과 Diffable Data Source를 도입하여 리팩토링을 진행했습니다.
- 뷰의 계층에 맞춰 뷰모델을 계층화하였습니다.
public var newPostData: [Post]? | ||
public var weeklyTopPostData: [Post]? | ||
public var customizedPostData: [Post]? | ||
public var editorRecommendationPostData: [Post]? |
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.
section이 늘어나면 데이터가 계속 추가될 거 같아요.
HomeSection에 연관값으로 해결해보면 어때요?
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.
현재 저는 HomeSection의 케이스가 늘어날 때마다 HomePostData의 프로퍼티가 늘어나는 것이 비효율적일 수 있다는 의미로 말씀해주신 것으로 이해를 했습니다. HomeSection 열거형 타입의 경우 앱 내 로직에서 사용되는 개념이고, HomePostData의 경우는 외부 API로부터 데이터를 매핑하는 DTO입니다. 현재 구조에서 HomePostData의 프로퍼티를 고정적으로 두지 않고 HomeSection의 케이스에 따라서 동적으로 형성되도록 한다면 기존 HomePostData는 쓰이지 않게 될 것 같습니다.
하지만 현재 구조에서 enum으로 동적으로 데이터를 매핑하는 것보다는 DTO를 따로 두는 것이 더 효율적일 수 있다는 생각을 했습니다. 제 의견에 대해서 멘토님의 피드백이 궁금합니다!
아니면 혹시 연관값으로 현재 문제를 해결할 수 있는 방법을 제가 캐치하지 못한 것인지도 궁금합니다.
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.
�넵 여기는 이렇게 두는게 맞겠네요.
View에서 Section을 다룰때 연관값을 사용하는건 어때요?
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.
#18 (comment)
이 부분에 코드를 보면 View layer에서 비즈니스 로직 모델에 접근하는 거 같아요.
View에서는 ViewModel만 알 수 있도록 개선이 필요할 거 같아요.
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.
말씀해주신대로 기존에는 HomeSection이 도메인 레이어에 정의되어 있었고 뷰에서 이것을 사용하고 있었는데, 현재 뷰 레이어 전용으로 사용되는 HomeSectionTemplate이라는 새로운 enum 타입을 추가하여 도메인 레이어는 ViewModel만 알게 하고 View는 ViewModel만을 바라보게끔 적용했습니다.
추가로 HomeSectionTemplate에서 연관값을 사용하여 View의 코드 개선도 적용했습니다!
// post는 제네릭 타입으로 주어진 HomePostViewTemplate의 인스턴스를 의미 | ||
collectionView, indexPath, post in | ||
/// 섹션의 순서를 가져온 배열. 섹션의 순서는 사용자마자 다를 수 있음. | ||
let section = HomeSection.allCases[indexPath.section] |
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.
HomeSection.allCases로 고정되면 가변적으로 동작할 수 있나요?
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.
말씀해주신 부분에서 가변적으로 동작하지 않는 오류를 발견했습니다!
HomeSection.allCases로 고정하는 것이 아니라 homeCVVM의 sectionIndex로 section을 가져오도록 수정하겠습니다!
switch section { | ||
case .todayPick: | ||
let cell = collectionView.dequeueReusableCell(withReuseIdentifier: HomeTodayPickCVC.identifier, for: indexPath) as! HomeTodayPickCVC | ||
cell.homePostViewData = post |
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.
모든 데이터가 HomePostViewTemplate 로 고정되는 문제가 있을거 같아요 어떻게 생각하시나요?
withReuseIdentifier: HomeContentsSectionHeaderView.identifier, | ||
for: indexPath) as! HomeContentsSectionHeaderView | ||
let section = HomeSection.allCases[indexPath.section] | ||
switch section { |
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.
section 연관값으로 데이터 모델이 있으면 코드를 개선할 수 있지 않을까요?
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.
위 코멘트에도 언급하였듯이 HomeSectionTemplate이라는 새로운 뷰전용 enum을 만들고 연관값을 적용시켜 코드를 개선시켜보았습니다!
func setHomeData(homePostData: HomePostViewTemplateGroup) { | ||
if let sectionIndex = homePostData.sectionIndex { | ||
self.sectionIndex = sectionIndex | ||
for sectioin in sectionIndex { |
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.
sectionIndex 는 integer 형태일 거 같은데 여기선 Collection type으로 사용되는 거 같아요.
조금 더 명시적인 이름으로 사용하는게 좋을 거 같아요.
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.
넵 sectionIndex보다는 sectionOrder라는 변수명으로 전체적으로 적용하겠습니다!
Quality Gate failedFailed conditions |