7주차 과제 #7
7주차 과제 #7
Conversation
| data class UpdateUsername(val username: String) : SignInIntent | ||
| data class UpdatePassword(val password: String) : SignInIntent | ||
| data object TrySignIn : SignInIntent | ||
| data object TryAutoSignIn : SignInIntent |
There was a problem hiding this comment.
Intent 의 이름은 View 에서 어떤 이벤트가 발생했는지에 대한 단순한 행동만을 나타내는게 좋은 것 같아요! 아래와 같이요! 행동에 대한 ViewModel 의 Action을 다르게 하는 느낌이면 좋을 것 같아요. "SignIn 버튼을 눌렀다" 정도만 알게 하고 나머지는 뷰모델에게 위임 한다는 느낌이에요!
- OnUsernameInputChanged
- OnPasswordInputChanged
- OnSignInClick
- OnAutoSignInClick
| if (errorResult != null) { | ||
| _effect.emit(SignInEffect.ShowSnackbar("Error")) | ||
| } |
There was a problem hiding this comment.
UiState 필드 중에 결과를 들고 있는거라면 SignInResult 를 제거하고, SignInEffect.ShowSnackbar 라는 SideEffect에 보여줄 snackbarMessage를� 넘겨서 처리하는 방법도 좋을 것 같아요 ! 그리고 ShowSnackbar, ShowToast, ShowProgress (Hide 도 동일) 같은 이펙트들은(다 쓰라는건 아니에요) 공통 SideEffect 와 같은 Interface 에 선언해두고 그걸 상속해두고 쓰면 편할 것 같긴합니다! 다른 SideEffect에서도 비슷한 Effect가 생길 가능성이 커서요! :)
There was a problem hiding this comment.
만일 위 리뷰를 반영하기 위해서 getString 을 viewModel 에서 하기 위해서 혹시나 @ApplicationContext context : Context 를 주입받으려고 하신다면 해당 Context 를 주입 받은 ResourceProvider 같은 클래스에서 getString 같은 동작을 해주는 클래스를 주입 받아보면 좋습니다! Context 는 사용할 수 있는 게 많지만 잘못 사용하면 에러가 날 가능성이 높아서요! 뷰모델에서는 크게 쓰일 일이 많지 않기 때문에 분리해도 좋을 것 같숩니닷
| } | ||
|
|
||
| sealed interface SignInEffect { | ||
| data class ShowSnackbar(val message: String) : SignInEffect |
There was a problem hiding this comment.
- 요렇게 바꾸면 좋을 것 같아요 ㅎㅎ SignUp 도요!
| data class ShowSnackbar(val message: String) : SignInEffect | |
| data class ShowSnackBar(val message: String) : SignInEffect |
| private val _effect = MutableSharedFlow<SignUpEffect>() | ||
| val effect = _effect.asSharedFlow() |
There was a problem hiding this comment.
여기에선 _effect 에 set 하는 부분이 없는 것 같아요 SignUpScreen 에서는 올 것을 기대하고 있구요!
| sealed interface SignUpResult { | ||
| data object Success : SignUpResult | ||
| data object UsernameInputEmpty : SignUpResult | ||
| data object PasswordInputEmpty : SignUpResult | ||
| data object HobbyInputEmpty : SignUpResult | ||
| data object InvalidUsername : SignUpResult | ||
| data object InvalidPassword : SignUpResult | ||
| data object InvalidHobby : SignUpResult | ||
| data object AlreadyExistUsername : SignUpResult | ||
| } |
| usernameInput = uiState.username, | ||
| passwordInput = uiState.password, | ||
| hobbyInput = uiState.hobby, |
There was a problem hiding this comment.
인자를 각각 넘겨주셨군요, 만약에 관리하게 되는 값이 많아진다면 코드가 너무 길어질 거 같아서 저는 uiState 자체를 넘겼는데 이부분에 대해서 고민을 한 번 해봐야할 거 같네요. 변수를 각각 넘겨준 이유가 따로 있는지 궁금합니다!
There was a problem hiding this comment.
uiState 자체를 넘겨주면 컴포넌트가 실제로 사용하지 않는 상태에도 접근 가능 해지기 때문에,
필요한 상태만 개별적으로 전달해준다면 리컴포지션을 방지할 수 있어서 이렇게 해주신 것이 맞을까요!?
| usernameInput = uiState.username, | ||
| passwordInput = uiState.password, | ||
| hobbyInput = uiState.hobby, |
There was a problem hiding this comment.
uiState 자체를 넘겨주면 컴포넌트가 실제로 사용하지 않는 상태에도 접근 가능 해지기 때문에,
필요한 상태만 개별적으로 전달해준다면 리컴포지션을 방지할 수 있어서 이렇게 해주신 것이 맞을까요!?
| sealed interface SignInIntent { | ||
| data class UpdateUsername(val username: String) : SignInIntent | ||
| data class UpdatePassword(val password: String) : SignInIntent | ||
| data object TrySignIn : SignInIntent | ||
| data object TryAutoSignIn : SignInIntent |
| sealed interface SignUpResult { | ||
| data object Success : SignUpResult | ||
| data object UsernameInputEmpty : SignUpResult | ||
| data object PasswordInputEmpty : SignUpResult | ||
| data object HobbyInputEmpty : SignUpResult | ||
| data object InvalidUsername : SignUpResult | ||
| data object InvalidPassword : SignUpResult | ||
| data object InvalidHobby : SignUpResult | ||
| data object AlreadyExistUsername : SignUpResult | ||
| } |
| data class SignInUiState( | ||
| val username: String = "", | ||
| val password: String = "", | ||
| val isLoading: Boolean = false, | ||
| val signInResult: SignInResult? = null | ||
| ) |
There was a problem hiding this comment.
State, Event, Effect, Intent, ..은 따로 예를 들어 SignInContract.kt 파일을 만들어서 따로 빼서 관리하는건 어떨까요?!
|
|
||
| sealed interface SignUpEffect { | ||
| data class ShowSnackbar(val message: String) : SignUpEffect | ||
| data object Success : SignUpEffect |
There was a problem hiding this comment.
SignUpEffect 랑 SignUpResult 안에 둘다 Success 가 있는데 이름을 다르게 지정하는게 어떨까요? 처음 보는 사람을 헷갈릴 수도 있을 것 같아요!
| onSignInButtonClicked = { | ||
| viewModel.handleIntent(SignInIntent.TrySignIn) | ||
| }, | ||
| onNavigateToSignUp = onNavigateToSignUp, | ||
| usernameInput = usernameInput, | ||
| passwordInput = passwordInput, | ||
| onUsernameInputChanged = viewModel::onUsernameInputChanged, | ||
| onPasswordInputChanged = viewModel::onPasswordInputChanged | ||
| usernameInput = uiState.username, | ||
| passwordInput = uiState.password, | ||
| onUsernameInputChanged = { | ||
| viewModel.handleIntent(SignInIntent.UpdateUsername(it)) | ||
| }, | ||
| onPasswordInputChanged = { | ||
| viewModel.handleIntent(SignInIntent.UpdatePassword(it)) | ||
| } |
There was a problem hiding this comment.
handleIntent 함수 자체만 넘길 수도 있어요! (세영이 PR처럼!) 알고 계시면 좋을 것 같습니다.
Work Description ✏️
To Reviewers 📢
아직 익숙하지가 않아서 어찌저찌 해놓긴 했는데, MVI가 맞는지도 모르겠고, 코드도 더 어려워진 거 같아요 ㅠㅠ
저는 아직까지는 MVVM이 훨씬 편한 거 같습니다...
MVI는 조금 더 공부를 해봐야겠어요...ㅎㅎ