[Feat] Week4 필수과제 #7
Conversation
There was a problem hiding this comment.
고생하셨습니다!
전체적으로 이야기하고 싶은 부분이 서버와의 소통에 대해서 공통적인 코드들이 많이 작성 되는 것 같아요, viewModel 에서 api 요청을 할 수 있는 케이스가 정말 많아지게 된다면 그에 따른 변수들과 상태 코드용 필드 변수가 많아질텐데 아마 그럴수록 문제가 생겼을 때 찾아내고 수정 하는 데에 시간을 많이 쏟을 것 같습니다!
이번 기회에 dataSource, respository 등의 개념을 통해서 데이터를 불러오는(서버, preference) 동작에 대해서 viewModel 에서는 단순히 가져오는 동작만 되도록 수정하면 좋겠습니다! 앞으로 더 복잡한 코드들을 쓰시게 될거라 한 번 코드 정리하면 좋을 것 같아요!
| val retrofit: Retrofit by lazy { | ||
| Retrofit.Builder() | ||
| .baseUrl(BASE_URL) | ||
| .client(client) | ||
| .addConverterFactory(Json.asConverterFactory("application/json".toMediaType())) | ||
| .build() | ||
| } |
There was a problem hiding this comment.
retrofit 객체는 전역으로 써야할 일이 있을까요?? 그게 아니라면 private 처리 해야할 것 같습니다!
| response: Response<ResponseMyHobby> | ||
| ) { | ||
| if (response.isSuccessful) { | ||
| response.body()?.result?.let { userViewModel.updateHobby(it.hobby) } |
There was a problem hiding this comment.
result 가 null 일 때에도 에러 처리 동작이 있으면 좋겠어요!
| private val _preferenceUserName = MutableStateFlow("") | ||
| private val _preferencePassword = MutableStateFlow("") | ||
| private val _preferenceHobby = MutableStateFlow("") | ||
| private val _token = MutableStateFlow("") | ||
|
|
||
| val preferenceEmail = _preferenceEmail.asStateFlow() | ||
| private val _isSignInSuccessful = MutableStateFlow(false) | ||
| private val _errorMessage = MutableStateFlow("") | ||
|
|
||
| val preferenceUserName = _preferenceUserName.asStateFlow() | ||
| val preferencePassword = _preferencePassword.asStateFlow() | ||
| val preferenceHobby = _preferenceHobby.asStateFlow() | ||
| val preferenceToken = _token.asStateFlow() | ||
| val isSignInSuccessful = _isSignInSuccessful.asStateFlow() | ||
| val errorMessage = _errorMessage.asStateFlow() |
There was a problem hiding this comment.
backing properties 와 실제 필드는 같이 위치해야 알아보기 더 쉬울 것 같아요!(_preferenceUserName, preferenceUserName)
| private var preferencesUserName = "" | ||
| private var preferencesPassword = "" | ||
| private var preferencesHobby = "" | ||
| private var token = "" |
| }) | ||
| } | ||
|
|
||
| private fun handleError(response: Response<ResponseSignIn>) { |
There was a problem hiding this comment.
해당 함수는 response.isSuccessful == false 일 때에만 실행하는 데 onFailure도 해당 error를 handle 할 여지가 있을 것 같아요!
| } | ||
| 403 -> "비밀번호가 일치하지 않습니다." | ||
| 404 -> "유효하지 않은 경로로 요청하셨습니다." | ||
| else -> "서버 오류가 발생했습니다. 상태 코드: ${response.code()}" |
There was a problem hiding this comment.
status code 중 400, 403, 404 에러 제외를 해도 서버 오류라고만 할 순 없을 것 같아요!
| private val _userState = mutableStateOf<ResponseSignIn?>(null) | ||
| val userState: State<ResponseSignIn?> get() = _userState |
There was a problem hiding this comment.
해당 필드는 값을 set 하지만 사용을 안 하는 것 같아요 제거하거나 사용하는 방식으로 변경하면 좋을 것 같습니다!
| import retrofit2.Response | ||
|
|
||
| class SignUpViewModel : ViewModel() { | ||
| private val userService by lazy { ServicePool.userService } |
There was a problem hiding this comment.
해당 Service 객체도 생성자에서 주입 받으면 좋을 것 같습니다! ref: DatastoreRepository
| @Serializable | ||
| data class ResponseSignUp( | ||
| @SerialName("result") | ||
| val result: ResponseSignUpResult, | ||
| ) { | ||
| @Serializable | ||
| data class ResponseSignUpResult( | ||
| @SerialName("no") | ||
| val no: Int, | ||
| ) | ||
| } |
There was a problem hiding this comment.
signin response와 signupresponse를 합쳐서 사용할 수 있는 방법도 있으니 참고해보시면 좋을 거 같아요!
There was a problem hiding this comment.
혹시 어떤 방법인지 여쭤봐도 될까요? Response 받는 값이 달라서 다르지 않나요?!
| val userProfileImg: Int = R.drawable.img_sample | ||
| private val _viewHistory = mutableListOf<ContentItem>() | ||
| val viewHistory: List<ContentItem> get() = _viewHistory | ||
| private val _interestContent = mutableListOf<ContentItem>() | ||
| val interestContent: List<ContentItem> get() = _interestContent |
| val userName by userViewModel.preferenceUserName.collectAsStateWithLifecycle() | ||
| val password by userViewModel.preferencePassword.collectAsStateWithLifecycle() | ||
| val isSignInSuccessful by userViewModel.isSignInSuccessful.collectAsStateWithLifecycle() | ||
| val errorMessage by userViewModel.errorMessage.collectAsStateWithLifecycle() |
There was a problem hiding this comment.
여기도 state로 관리해보면 더 좋을 거 같아요!
하나의 state로 관리하면 상태 관리가 한 곳에서 이루어져 전체 상태를 쉽게 파악하거나 나중에 수정할 때 더 좋을 거 같습니당
| @Serializable | ||
| data class ResponseError( | ||
| @SerialName("code") | ||
| val code: String | ||
| ) |
There was a problem hiding this comment.
에러 response 는 이렇게 받아오면 좋겠군요..!
| @Serializable | ||
| data class ResponseMyHobby( | ||
| @SerialName("result") | ||
| val result: ResponseMyHobbyResult, | ||
| ) { | ||
| @Serializable | ||
| data class ResponseMyHobbyResult( | ||
| @SerialName("hobby") | ||
| val hobby: String, | ||
| ) | ||
| } |
There was a problem hiding this comment.
이렇게 data class 내부에 data class 를 넣어도 되는군요
| @Serializable | ||
| data class ResponseSignUp( | ||
| @SerialName("result") | ||
| val result: ResponseSignUpResult, | ||
| ) { | ||
| @Serializable | ||
| data class ResponseSignUpResult( | ||
| @SerialName("no") | ||
| val no: Int, | ||
| ) | ||
| } |
There was a problem hiding this comment.
혹시 어떤 방법인지 여쭤봐도 될까요? Response 받는 값이 달라서 다르지 않나요?!
| val myViewModel: MyViewModel = viewModel() | ||
| val viewHistory = myViewModel.viewHistory | ||
| val interestContent = myViewModel.interestContent | ||
| myViewModel.getMyHobby(userViewModel) |
There was a problem hiding this comment.
뷰모델의 함수를 Composable 함수에서 직접 호출하면, 리컴포지션마다 getMyHobby가 재호출될 가능성이 있으므로 호출 위치를 옮겨주면 안전할 것 같습니다 !
예를 들면 viewModel의 init { ... } 문을 사용할 수도 있을 것 같아요 !!
There was a problem hiding this comment.
init 블록에서 무거운 작업(서버 통신, 데이터베이스 쿼리 등과 같은,,)을 진행하는 것이 좋지 않고, 가벼운 초기화 로직만 들어가는 게 좋습니다.
init 블록에서 가벼운 초기화 로직만 작성해야 하는 이유는 여러 가지가 있는데요,, 일단 첫번째로 init 블럭은 사실상 생성자의 일부로 볼 수 있어서 비동기로 처리되어야 하는 로직을 작성하는 게 좋지 않습니다. 그리고 init 블럭에서 함수를 호출하면 그 시점이 앱의 다른 생명주기 이벤트와 충돌할 수 있어 문제가 됩니다. (예를 들어,, 뷰모델이 액티비티보다 먼저 초기화 되었는데 그 부분에서 UI를 업데이트 시키는 경우 -> 이로 인한 NPE) 또, 뷰모델에서 의존성 주입을 하는 경우에는 의존성이 주입되기 전에 함수가 호출될 수 있어요!
추가로 저도 리뷰 달다가 본 글인데 (종명 오빠 땡큐 ㅎ.ㅎ) 이 글 읽어보셔도 좋을 것 같네요 ~
| onValueChange = { email = it } | ||
| value = userName, | ||
| placeholderText = "사용자 이름", | ||
| onValueChange = { userViewModel.updateUserName(it) } |
There was a problem hiding this comment.
| onValueChange = { userViewModel.updateUserName(it) } | |
| onValueChange = userViewModel::updateUserName |
이렇게 줄여서 사용해보는 것도 좋을 것 같아요 !
| ) | ||
| Spacer(modifier = Modifier.height(6.dp)) | ||
| GrayTextField( | ||
| value = password, | ||
| placeholderText = stringResource(R.string.password), | ||
| isPassword = true, | ||
| passwordHidden = passwordHidden, | ||
| onValueChange = { password = it }, | ||
| onValueChange = { userViewModel.updatePassword(it) }, |
| data class ResponseSignIn( | ||
| @SerialName("result") | ||
| val result: ResponseSignInResult, | ||
| ) { |
There was a problem hiding this comment.
공통되는 부분을 BaseResponse로 만들어도 좋겠네요!
|
|
||
| @GET("/user/my-hobby") | ||
| fun getMyHobby( | ||
| @Header("token") |
There was a problem hiding this comment.
여기서 넣는 게 아니라 인터셉터를 통해 넣을 수도 있습니다!
| val myViewModel: MyViewModel = viewModel() | ||
| val viewHistory = myViewModel.viewHistory | ||
| val interestContent = myViewModel.interestContent | ||
| myViewModel.getMyHobby(userViewModel) |
There was a problem hiding this comment.
init 블록에서 무거운 작업(서버 통신, 데이터베이스 쿼리 등과 같은,,)을 진행하는 것이 좋지 않고, 가벼운 초기화 로직만 들어가는 게 좋습니다.
init 블록에서 가벼운 초기화 로직만 작성해야 하는 이유는 여러 가지가 있는데요,, 일단 첫번째로 init 블럭은 사실상 생성자의 일부로 볼 수 있어서 비동기로 처리되어야 하는 로직을 작성하는 게 좋지 않습니다. 그리고 init 블럭에서 함수를 호출하면 그 시점이 앱의 다른 생명주기 이벤트와 충돌할 수 있어 문제가 됩니다. (예를 들어,, 뷰모델이 액티비티보다 먼저 초기화 되었는데 그 부분에서 UI를 업데이트 시키는 경우 -> 이로 인한 NPE) 또, 뷰모델에서 의존성 주입을 하는 경우에는 의존성이 주입되기 전에 함수가 호출될 수 있어요!
추가로 저도 리뷰 달다가 본 글인데 (종명 오빠 땡큐 ㅎ.ㅎ) 이 글 읽어보셔도 좋을 것 같네요 ~
Related issue 🛠
Work Description ✏️
Screenshot 📸
https://youtu.be/-tS9yaWhOpo
Uncompleted Tasks 😅
To Reviewers 📢
아직 MVVM을 정확하게 이해하지 못한 거 같아서 이것저것 수정해보긴 했는데 완벽하진 않은 것 같습니다😅 많은 피드백 부탁드립니당..