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

[AN] 스터디 개설 뷰 리펙터링 #618

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

Conversation

inseonyun
Copy link
Member

😋 작업한 내용

  • Flow 코드 수정
    • Cold Flow -> Hot Flow
    • Collect 확장 함수 사용
  • ScrollView 적용
  • SharedFlow Event 적용
  • Retrofit BaseURL, End Point 수정

🙏 PR Point

  • Event에 Navigate 넣고 CreateStudyActivity에서 화면 전환을 수행하다보니 코드가 엉킨거 같은데 이 부분 조언 바랍니당 . 완벽해 보이나요?

👍 관련 이슈

@inseonyun inseonyun added android💚 안드 refactor 리팩터링 선희☀️ 써니 labels Nov 21, 2023
@inseonyun inseonyun self-assigned this Nov 21, 2023
Copy link

github-actions bot commented Nov 21, 2023

Test Results

9 tests   9 ✔️  0s ⏱️
3 suites  0 💤
3 files    0

Results for commit 77cc089.

♻️ This comment has been updated with latest results.

Copy link
Collaborator

@s9hn s9hn left a comment

Choose a reason for hiding this comment

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

Life Good TM

}
private fun collectCreateStudyState() {
createStudyViewModel.createStudyState
.collectLatestOnStarted(this) { createStudyState ->
Copy link
Collaborator

Choose a reason for hiding this comment

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

왜 collectLatest를 사용하셨나요?

Copy link
Member Author

Choose a reason for hiding this comment

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

State에 Loading과 Idle을 넣기 전에 중복 시도 시 이전 통신 값은 무시하고 마지막에 요청한거만 하도록 하려고 이렇게 했었슴다
지금은 collect든 collectLatest든 둘 다 동일한 동작을 수행할 거 같네용

@@ -112,15 +115,34 @@ class CreateStudyViewModel @Inject constructor(
)
createStudyRepository.createStudy(study)
.onSuccess { studyId ->
_createStudyUiState.emit(CreateStudyUiState.Success(studyId))
_createStudyEvent.emit(Event.CreateStudySuccess)
_createStudyState.emit(CreateStudyState.Success(studyId))
Copy link
Collaborator

Choose a reason for hiding this comment

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

세터말고 emit한 이유가 뭔가요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

update vs setter vs emit

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

setter랑 emit 차이는 없는걸로 알고 있었는데 update는 첨보네요 굳

else -> {
null
}
TAG_CREATE_STUDY_PEOPLE_COUNT -> PeopleCountBottomSheetFragment()
Copy link
Collaborator

Choose a reason for hiding this comment

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

프래그먼트들 미리 생성해놓는거 어떤가요

Copy link
Member Author

Choose a reason for hiding this comment

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

흐음 고민해보겠습니다
근데 지금 구조 그냥 계속 무조건 새로 만드네요 매우 별로네요

Copy link
Collaborator

@no1msh no1msh left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 써늬
몇가지 코멘트 남겼으니
읽어보시고 잇더즌메잌센스하다 싶은건 걸러주세요!

Comment on lines +130 to +145
sealed interface Event {
object CreateStudySuccess : Event
object CreateStudyFailure : Event
data class NavigateToBefore(val fragmentState: FragmentState) : Event
data class NavigateToNext(val fragmentState: FragmentState) : Event
}

sealed interface CreateStudyState {
class Success(val studyId: Long) : CreateStudyState

object Loading : CreateStudyState

object Failure : CreateStudyState

object Idle : CreateStudyState
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

아래 state에서는 어느 state인지 CrateStudyState라고 해주셨는데,
위의 Event는 그냥 Event라고 명시해주셨네요.

서로 다른 이유가 혹시 있을까요?

binding.onInputClickListener = ::setOnInputClick
private fun setupPeopleCount() {
createStudyViewModel.peopleCount.collectOnStarted(viewLifecycleOwner) { peopleCount ->
if (peopleCount == -1) return@collectOnStarted
Copy link
Collaborator

Choose a reason for hiding this comment

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

-1은 무엇을 뜻하나용?

Comment on lines 47 to 55
val isEnableFirstCreateStudyNext: StateFlow<Boolean> =
combine(peopleCount, studyDate, cycle) { peopleCount, studyDate, cycle ->
return@combine peopleCount != DEFAULT_INT_VALUE && studyDate != DEFAULT_INT_VALUE && cycle != DEFAULT_INT_VALUE
}
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(), false)

val isEnableSecondCreateStudyNext: Flow<Boolean> =
val isEnableSecondCreateStudyNext: StateFlow<Boolean> =
combine(studyName, studyIntroduction) { studyName, studyIntroduction ->
return@combine studyName.isNotBlankAndEmpty() && studyIntroduction.isNotBlankAndEmpty()
}
}.stateIn(viewModelScope, SharingStarted.WhileSubscribed(), false)
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 combine 블록 내부에
return@combine 구문이 필요한가요??
블럭 내부에 가장 마지막 줄이 return의 역할을 하지 않나요?
제가 놓치는 점이 있다면 꼭 말씀주세요! 궁금합니다.

'블록의 마지막 식이 블록의 결과'라는 규칙은 블록이 값을 만들어내야 하는 경우 항상 성립한다. - Kotlin in action

Comment on lines +130 to +135
sealed interface Event {
object CreateStudySuccess : Event
object CreateStudyFailure : Event
data class NavigateToBefore(val fragmentState: FragmentState) : Event
data class NavigateToNext(val fragmentState: FragmentState) : Event
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

제가 참고했던 구글의 프로젝트에선 Event가 아닌 Action으로 정의하고
아래 처럼 Action(행동)에 관한 Navigate관련 클래스 두개로만 관리하긴 합니다.

sealed class OnboardingNavigationAction {
    object NavigateToMainScreen : OnboardingNavigationAction()
    object NavigateToSignInDialog : OnboardingNavigationAction()
}

현재 코드는 행동과 이벤트(상황)가 혼합된 느낌이긴 합니다.
분리하는 것에 대해 어떻게 생각하시나요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android💚 안드 refactor 리팩터링 선희☀️ 써니
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[AN] 스터디 개설 뷰 리펙터링
3 participants