-
Notifications
You must be signed in to change notification settings - Fork 0
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
[FEAT] 4주차 과제 완료 #8
base: develop
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버 통신도 넘 잘 하셨네요! 수고하셨습니다ㅎㅎ
import retrofit2.Retrofit | ||
|
||
object ApiFactory { | ||
private const val BASE_URL: String = BuildConfig.BASE_URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BASE_URL을 바로 사용하지 않고 value로 저장하신 이유가 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
value로 저장하고 쓰는게 습관이 되어서 .. 보니까 저장 안하고도 쓸 수 있었군요 🥲
수정하겠습니다 !
|
||
@Serializable | ||
data class ResponseHobbySuccessDto( | ||
@SerialName("result") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
다른 api들에서도 response가 result로 감싸져서 오기 때문에 BaseReposne를 만들어서 사용하는 것도 고려해보면 좋을 것 같아요!
import kotlinx.serialization.Serializable | ||
|
||
@Serializable | ||
data class ResponseLoginSuccessDto( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response와 request를 같은 파일에서 관리하시는데 저는 따로 파일을 만들었거든요!! 나중에 어떤게 더 좋을지 이야기해보는 것도 좋을 것 같네요😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
분리하는게 더 좋을 것 같아요 ㅎㅎ .. 수정하겠습니다 !
val signUpViewModel: SignUpViewModel = viewModel() | ||
val signInViewModel: SignInViewModel = viewModel() | ||
val myViewModel: MyViewModel = viewModel() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
뷰모델을 navGraph에서 관리하시는 이유가 있나요? 각 뷰에서 관리하는게 더 좋을 것 같다는 생각이 드는데 혹시 다른 이유가 있으신지 궁금합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전에 데이터를 넘기는 방법으로 한 뷰에서 두개의 viewmodel을 부른 적이 있었는데 그때 navgraph에서 뷰모델을 관리했었어요 ! 근데 이제 데이터를 넘기는 로직을 변경했으니 viewmodel 로직도 각 뷰에서 관리하는 방법으로 변경해야겠네요 감사합니다 !!
fun getUserHobby(sharedPreferences: SharedPreferences) { | ||
val token = sharedPreferences.getString("token", null) | ||
if (token.isNullOrEmpty()) { | ||
Log.e("MyViewModel", "토큰 없음") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Timber에 대해서도 알아보면 좋을 것 같아요! 디버그 모드에서만 로그가 남도록 할 수 있답니다😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 맞다 .. ! 저번에도 말씀해주셨었는데 🥲 앞으로는 Timber .. !!
signInViewModel.updateUserInfo(userId, userPassWord) | ||
signInViewModel.performLogin() | ||
// signInViewModel.updateUserInfo(userId, userPassWord) | ||
// signInViewModel.performLogin() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
주석 지워주세요..!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아차차 ! 감사합니당 ~
username = userId, | ||
password = userPassWord, | ||
onSuccess = { tokenMessage -> | ||
signInViewModel._snackbarMessage.value = tokenMessage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_snackbarMessage 대신 snackbarMessage를 사용하는게 맞을 것 같네요..!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
허걱 그러네요 감사합니다 🫠
|
||
private val _snackbarMessage = MutableStateFlow<String?>(null) | ||
val _snackbarMessage = MutableStateFlow<String?>(null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요거는 private으로!! 아니면 private set 사용하면 변수를 나누지 않고 구현 가능합니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아 맞다맞다 private set으로 해볼게요 !!
val errorDto = errorBody?.let { Json.decodeFromString<ResponseErrorDto>(it) } | ||
val errorMessage = when (response.code()) { | ||
400 -> when (errorDto?.code) { | ||
"02" -> "로그인 정보가 올바르지 않습니다." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런 내용도 string 추출하는 것은 어떤가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
헉 viewmodel에서는 string 추출할 생각을 못했네요 해볼게요 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4주차 과제 고생 많으셨습니다!!
의존성을 수동으로 주입하신 게 인상이 깊습니다!!
fun getUserHobby(sharedPreferences: SharedPreferences) { | ||
val token = sharedPreferences.getString("token", null) | ||
if (token.isNullOrEmpty()) { | ||
Log.e("MyViewModel", "토큰 없음") | ||
return | ||
} | ||
|
||
hobbyService.getMyHobby(token).enqueue(object : Callback<ResponseBody> { | ||
override fun onResponse(call: Call<ResponseBody>, response: Response<ResponseBody>) { | ||
if (response.isSuccessful) { | ||
val successBody = response.body()?.string() | ||
val successDto = | ||
Json.decodeFromString<ResponseHobbySuccessDto>(successBody ?: "") | ||
viewModelScope.launch { | ||
_hobby.emit(successDto.result.hobby) | ||
} | ||
} else { | ||
val errorBody = response.errorBody()?.string() | ||
val errorDto = errorBody?.let { Json.decodeFromString<ResponseErrorDto>(it) } | ||
val errorMessage = when (response.code()) { | ||
401 -> "토큰이 없습니다." | ||
403 -> "유효하지 않은 토큰입니다." | ||
404 -> "잘못된 경로로 요청했습니다." | ||
else -> "알 수 없는 오류가 발생했습니다." | ||
} | ||
Log.e("MyViewModel", errorMessage) | ||
} | ||
} | ||
|
||
override fun onFailure(call: Call<ResponseBody>, t: Throwable) { | ||
Log.e("MyViewModel", "Network error: ${t.message}") | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버와의 통신에 관한 책임을 가지는 곳으로 한번 분리해보는 건 어떨까요?? (만약 이 viewmodel에서 통신이 많아지면 너무 비대해지지 않을까? 하는 생각이 들었습니다!!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아아 그러네요 !
repository를 생성해서 통신을 좀 분리해보겠습니당 감사해요 !!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
야무진 코드 잘봤습니다 4주차도 고생많으셨습니다!
context.getSharedPreferences("user_prefs", Context.MODE_PRIVATE) | ||
} | ||
|
||
LaunchedEffect(Unit) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewmodel에서 init 하는것보다 launchedEffect를 사용하는 이유가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어 사실 init과 launchedEffect의 차이를 잘 모르겠어서 그냥 매번 번갈아 사용했던 것 같아요
그래서 차이점을 좀 찾아보니까 .. !
- ViewModel의 init을 사용할 때:
- 데이터가 다양한 화면에서 공유되거나, 화면이 재구성되더라도 상태를 유지해야 하는 경우.
- 데이터 로직이 UI와 분리된 독립적인 방식으로 동작해야 할 때.
- LaunchedEffect를 사용할 때:
- 특정 화면에서만 일시적으로 필요한 데이터를 로드하거나, 화면 상태에 따라 동적으로 데이터를 로드해야 할 때.
- 로직이 화면 구성과 밀접하게 연결되어 있는 경우.
라고 하네요 ! 그래서 제 생각에는 여기서는 LaunchedEffect를 쓰는게 더 좋은 것 같아요 !!
if (response.isSuccessful) { | ||
val successBody = response.body()?.string() | ||
val successDto = | ||
Json.decodeFromString<ResponseHobbySuccessDto>(successBody ?: "") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오류 처리 야무지시네요..배워갑니다...
} | ||
} | ||
|
||
override fun onFailure(call: Call<ResponseBody>, t: Throwable) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이런식으로 errormeassage를 출력하시는군요..배워갑니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
고생하셨습니다! 합세도 파이팅이에요 ~
@Serializable | ||
data class UserData( | ||
@SerialName("no") | ||
val no: Int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
더 직관적인 이름을 사용해도 좋을 것 같아요!
단순히 no라고만 네이밍 하면 어떤 것을 의미하는지 모호해지니까요!
꼭 서버에서 내려오는 이름이랑 동일하게 가져가야할 필요는 없습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
무조건 서버에서 내려오는 이름이랑 동일하게만 사용했었는데 .. 직관적인 네이밍을 해도 되는거였군요 ! 수정해보겠습니다 !!
import retrofit2.http.GET | ||
import retrofit2.http.Header | ||
|
||
interface HobbyService { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Service를 나누시는 기준이 무엇인지 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 보통 기능별로 나누는 편입니다 !
userService는 사용자 관련 데이터, loginService는 로그인 관련 데이터, .. 이렇게 쓰다보니 hobby는 사용자 관련 데이터니까 userService에 넣는게 맞았던 것 같네요 🥲
수정하겠습니다 !!
myViewModel.getUserHobby(sharedPreferences) | ||
} | ||
|
||
val hobby by myViewModel.hobby.collectAsState() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectAsStateWithLifecycle과 collectAsState의 차이는 무엇일까요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectAsState와 collectAsStateWithLifecycle는 모두 flow를 state로 변환하는데,
collectAsState는 Lifecycle에 대한 인식을 하지 않아 flow를 계속 구독하고 있어 리소스가 비효율적으로 사용되는 반면에
collectAsStateWithLifecycle은 Lifecycle을 인식하고 그 상태에 따라 flow 구독을 자동으로 관리해요 !
이러한 장점 때문에 대부분의 UI 로직에서 collectAsStateWithLifeCycle을 더 선호한다고 하는 군요 ... !!! 감사합니다 ! 수정해볼게요
var userHobby by remember { | ||
mutableStateOf("") | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
viewModel에서 관리하는 것이 좋을 듯 합니다! Screen함수는 정말로 UI를 그려주는 역할만 담당하도록 하면 좋을 것 같네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵
screen은 UI를 그려주는 역할만 .. 로직은 viewmodel에서 .. !
Related issue 🛠
Work Description ✏️
→ username, password, hobby를 입력하는 구조
→ 조건을 만족하지 않았다는 것을 화면에 표시 (방식은 자율 → toast)
→ username, password를 받는 구조
(방식은 자율 → snackbar)
Screenshot 📸
4.mp4
Uncompleted Tasks 😅
To Reviewers 📢
토큰 정보를 sharedPreference로 넘겨줬는데 더 좋은 방법이 있을까요 ??