[REFACTOR] 7차 과제 완료 #12
Conversation
MinseoSONG
requested review from
Hyobeen-Park,
jihyunniiii,
beom84 and
wjdrjs00
| import org.sopt.and.data.remote.model.response.ResponseHobbyDto | ||
| import org.sopt.and.domain.model.Hobby | ||
|
|
||
| fun BaseResponse<ResponseHobbyDto>.toDomain(): Hobby { |
There was a problem hiding this comment.
저는 해당 mapper 함수는 BaseResponse보다는 데이터레이어의 엔티티를 도메인 엔티티로 변환하는 mapper라고 생각해서 dto의 확장함수로 만들었어요!! 민서님은 BaseReponse의 함수로 구현하셔서 이유가 궁금합니다!!
| _uiState.value = _uiState.value.reducer() | ||
| } | ||
|
|
||
| protected fun setSideEffect(builder: () -> SideEffect) { |
There was a problem hiding this comment.
오호 builder의 무엇인지!! 역할이 무엇인지 궁금해요!!
| data class UserIdChanged(val userId: String) : SignUpEvent() | ||
| data class UserPasswordChanged(val userPassword: String) : SignUpEvent() | ||
| data class UserHobbyChanged(val userHobby: String) : SignUpEvent() | ||
| object SignUpClicked : SignUpEvent() |
There was a problem hiding this comment.
오잉? 이 친구만 object네요!! data object가 아닌 그냥 object 사용하신 이유가 궁금해요!
|
|
||
| sealed class SignUpSideEffect : UiSideEffect { | ||
| data class ShowToast(val message: String) : SignUpSideEffect() | ||
| object NavigateToSignIn : SignUpSideEffect() |
| navController.navigate(Routes.SignIn.route) { | ||
| popUpTo(Routes.SignUp.route) { inclusive = true } |
There was a problem hiding this comment.
저는 navigator는 최대한 호이스팅해주려 하는 편입니다!! navigation 관련 로직은 screen에서보다는 navigator에서 해주는게 더 각각의 역할에 맞는 것 같아서요!!
There was a problem hiding this comment.
동의합니다 ㅎ.ㅎ 합동 세미나 리뷰 (아마 당근이었나,,)에도 비슷한 내용을 적어놓은 부분이 있으니 이를 읽어보셔도 좋을 것 같아요.
| fun BaseResponse<ResponseLoginDto>.toDomain(): Token { | ||
| return Token(token = this.result.token) | ||
| } |
There was a problem hiding this comment.
이렇게도 가능합니다!(이미 아시겠지만 그래도 쇽 남겨보고싶었어요..ㅎ)
| fun BaseResponse<ResponseLoginDto>.toDomain(): Token { | |
| return Token(token = this.result.token) | |
| } | |
| fun BaseResponse<ResponseLoginDto>.toDomain(): Token = | |
| Token(token = this.result.token) |
| val response = hobbyRemoteDataSource.getMyHobby(token) | ||
| response.toDomain() |
There was a problem hiding this comment.
단순 response보단 hobby를 나타낼수 있는 네이밍을 사용하면 좀 더 명확한 전달이 가능하지 않을까? 하는 생각이 들었습니다!!
| import org.sopt.and.data.remote.model.response.ResponseHobbyDto | ||
| import org.sopt.and.domain.model.Hobby | ||
|
|
||
| fun BaseResponse<ResponseHobbyDto>.toDomain(): Hobby { |
| override suspend fun handleEvent(event: MyEvent) { | ||
| when (event) { | ||
| is MyEvent.LoadUserHobby -> {} | ||
| } | ||
| } |
There was a problem hiding this comment.
이 함수에 어떤 것들을 담을지 고민해보시고 이를 바탕으로 로직을 수정해보시면 좋을 것 같아요.
| LaunchedEffect(Unit) { | ||
| signInViewModel.sideEffect.collectLatest { sideEffect -> | ||
| when (sideEffect) { | ||
| is SignInSideEffect.ShowSnackBar -> { |
There was a problem hiding this comment.
스낵바와 관련된 sideEffect를 통해서 화면 전환까지 함께 이루어지고 있는 것 같은데요. 이를 분리해보셔도 좋을 것 같아요. 지금은 하나의 함수가 여러 책임을 가지고 있는 것 같네요.
| navController.navigate(Routes.SignIn.route) { | ||
| popUpTo(Routes.SignUp.route) { inclusive = true } |
There was a problem hiding this comment.
동의합니다 ㅎ.ㅎ 합동 세미나 리뷰 (아마 당근이었나,,)에도 비슷한 내용을 적어놓은 부분이 있으니 이를 읽어보셔도 좋을 것 같아요.
Related issue 🛠
Work Description ✏️
Screenshot 📸
이전 과제와 같습니다 !
Uncompleted Tasks 😅
To Reviewers 📢
처음 배우는 내용인데 시간이 없어서 급하게 하느라 많이 부족한 것 같아요 ..
여러분들의 리뷰로 많이 배우고 공부하고 리팩해보겠습니다 !!