-
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
Week4 사람 리스트 불러오는 api 붙이기 #3
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.
//.addConverterFactory(Json.asConverterFactory("application/json".toMediaType())) | ||
.addConverterFactory(GsonConverterFactory.create()) |
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.
Converter를 Gson으로 변경하신 이유가 있을까요!!?
navController: NavController, | ||
) { | ||
val viewModel: DoAndroidViewModel = viewModel() | ||
val usersState = viewModel.users.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.
lifecycle-runtime 의존성을 추가해서 collectAsStateWithLifecycle
를 사용하면 생명주기에 상태를 안전하게 관리할 수 있을 것 같습니다! 아니면 이미 orbit 라이브러리 쓰시니 orbit collectAsState api를 사용해도 좋을 것 같아요!
dependencies {
implementation "androidx.lifecycle:lifecycle-runtime-compose:[version]"
}
https://developer.android.com/jetpack/androidx/releases/lifecycle
package org.sopt.do_sopt_compose.ui.pages.doandroid | ||
|
||
sealed class DoAndroidSideEffect { | ||
data class ToastMessage(val message: String) : DoAndroidSideEffect() |
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.
해당 데이터 클래스가 프로퍼티로 message를 가지고 있어서 ToastMessage라는 클래스 네이밍은 함수형 프로그래밍에 있어서 조금 어색하다고 생각해요!!
예를 들어서,
- 메시지가 하나가 아닌 여러 메시지를 전송한다면?
- 프로퍼티 네이밍이 message 에서 messages 로 네이밍이 변경된다.
- 클래스 네이밍도 ToastMessage 에서 ToastMessages 로 변경되어야 하는가?
그래서 다른 클래스 네이밍을 고민해보는 건 어떠신가요!? 👻👻👻
} | ||
postSideEffect(DoAndroidSideEffect.ToastMessage("데이터 로드 실패")) | ||
} | ||
e.printStackTrace() |
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.
printStackTrace는 에러가 난 근원지를 찾아 에러를 뽑아주는데, bad practice라는 의견이 있더라구요!
단순한 예외처리에 대한 trace는 노상관일 것 같지만요.. 읽어보시면 좋을 것 같은 자료 올려두겠습니다.. 이해해서 저도 좀 알려주세요 rein님! 😎😎
https://stackoverflow.com/questions/7469316/why-is-exception-printstacktrace-considered-bad-practice
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.
긋긋티비
fun DoAndroidScreen( | ||
navController: NavController, | ||
) { | ||
val viewModel: DoAndroidViewModel = 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.
개인적으로 뷰모델은 파라미터로 두는 것이 여러가지로 이점이 있다고 생각합니다이 :)
To Reviewers
Screen_recording_20240428_210835.mp4