[REF/#638] Feed ViewModel 내의 UI 스크롤 로직 분리#699
Conversation
nahy-512
requested review from
Daljyeong,
Hyobeen-Park,
angryPodo and
nhyeonii
angryPodo
left a comment
angryPodo
left a comment
There was a problem hiding this comment.
의도에 맞게 잘 구현한 것 같습니다! 그러나 PR 제목만 보고는 어떤걸 개선했는지 알기 어려웠어요. 좀더 의도를 담으면 좋을것 같아요.
그리고 코드 가독성과 명시성에서 생각할만한 코멘트 달았습니다.👍🏻 나현 판단하에 결정해주세요~
| firstVisibleItemScrollOffset = currentListState.firstVisibleItemScrollOffset | ||
| ) | ||
| Pair(currentListState.firstVisibleItemIndex, currentListState.firstVisibleItemScrollOffset) | ||
| } | ||
| .pairwise() | ||
| .collect { (previous, current) -> | ||
| val isScrollingDown = previous != null && ( | ||
| current.first > previous.first || | ||
| (current.first == previous.first && current.second > previous.second) | ||
| ) | ||
|
|
||
| if (currentListState.isScrollInProgress && | ||
| isScrollingDown(previous, current) && | ||
| isScrollingDown && |
There was a problem hiding this comment.
View의 관심사로 이동했네요!
다만 스크롤 방향을 판단하는 로직이 LaunchedEffect 내부에 직접 구현되어 있어서 가독성이 조금 떨어진다고 생각해요.
이게 인덱스인지 오프셋인지 Pair만 보고도 헷갈릴 수도 있다고 생각해요. 기존에 State클래스를 사용해서 필드로 접근했던 방식이 가독성 면에서는 더 좋았던 것 같아요. 해당 데이터 클래스를 Screen 내부나 최하단에 private로 정의하고 판단 로직을 내부함수로 분리해서 사용하는건 어떨까요? 그러면 LaunchedEffect내부 코드가 더 깔끔해지고 의도가 명확해질 것 같습니다.👍🏻
Hyobeen-Park
left a comment
Hyobeen-Park
left a comment
There was a problem hiding this comment.
확실히 지금이 더 각자의 관심사에 맞게 잘 구현된 것 같네요! 수고하셨습니다~!
| } | ||
| .pairwise() | ||
| .collect { (previous, current) -> | ||
| val isScrollingDown = previous != null && ( |
There was a problem hiding this comment.
null 처리는 ?.let { } 로도 처리할 수 있을 것 같은데 어떤가요??
nahy-512
changed the title
Daljyeong
left a comment
Daljyeong
left a comment
There was a problem hiding this comment.
넘 수고하셨습니다!!
이전에도 느꼈지만 UI와 ViewModel의 책임 범위는 항상 고민이 많이 되는 부분인 것 같네요 🥲
개인적으로 이번 PR은 로직이 크게 변화되었다기 보다는 책임이 '분리'된 성격에 가까워 보여서, 이번 이슈와 PR에서는 '분리'라는 표현을 사용하고, 추후 예정되어 있는 #698 관련 작업에서 실제 로직 변경이 들어갈 때 '개선'이라는 용어를 사용하면 이력 추적에 조금 더 도움이 될 것 같다는 생각이 들었는데 나현이의 의견도 궁금합니다 :)
| current.first > previous.first || | ||
| (current.first == previous.first && current.second > previous.second) |
There was a problem hiding this comment.
저도 이 부분을 보면서 기존의 FeedScrollState와 같은 data class를 유지하는 게 좋을 것 같다고 생각했어요. Pair를 사용하면 first, second로 접근해야 하기에 의미를 바로 파악하기 조금 어려운 것 같아 이전처럼 네이밍을 활용해주면 가독성과 의도 전달이 더 좋아질 것 같습니다.
nhyeonii
left a comment
nhyeonii
left a comment
There was a problem hiding this comment.
오호 스크롤 로직의 책임분리가 잘 이루어진 PR 같아요 ㅎㅎ 작업 너무 고생하셨습니다 😻
@Daljyeong |
nahy-512
changed the title
|
@angryPodo 말씀주신 방향으로 수정해보았는데, 확인 한번만 다시 부탁드리겠습니다:) |
angryPodo
left a comment
angryPodo
left a comment
There was a problem hiding this comment.
확인 완료! 어때요? 어떻게 하는지보다 뭘 하는지 보여주는 게 저는 좋다고 생각해요ㅎㅎ 앞으로도 생각하면서 코드 설계하면 더 좋은 코드 나올 거예요.
|
감사합니다 앞으로 설계 생각하며 코드 구현하는 습관을 들여보겠습니다!! 항상 많이 배워갑니다🙇🏻♀️ |
5 participants
PR chekList
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
FeedViewModel의readAllFeed를 유지할지에 대해 고민이 많았으나, 페이지네이션을 적용하게 된다면 해당 함수에서 커서 기반으로 API를 불러오는 함수를 호출하면 될 것 같아 남겨두었습니다.