Conversation
kangyuri1114
requested review from
Jokwanhee,
chattymin,
haeti-dev and
stellar-halo
| //.addConverterFactory(Json.asConverterFactory("application/json".toMediaType())) | ||
| .addConverterFactory(GsonConverterFactory.create()) |
| navController: NavController, | ||
| ) { | ||
| val viewModel: DoAndroidViewModel = viewModel() | ||
| val usersState = viewModel.users.collectAsState() |
There was a problem hiding this comment.
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.
해당 데이터 클래스가 프로퍼티로 message를 가지고 있어서 ToastMessage라는 클래스 네이밍은 함수형 프로그래밍에 있어서 조금 어색하다고 생각해요!!
예를 들어서,
- 메시지가 하나가 아닌 여러 메시지를 전송한다면?
- 프로퍼티 네이밍이 message 에서 messages 로 네이밍이 변경된다.
- 클래스 네이밍도 ToastMessage 에서 ToastMessages 로 변경되어야 하는가?
그래서 다른 클래스 네이밍을 고민해보는 건 어떠신가요!? 👻👻👻
| } | ||
| postSideEffect(DoAndroidSideEffect.ToastMessage("데이터 로드 실패")) | ||
| } | ||
| e.printStackTrace() |
There was a problem hiding this comment.
printStackTrace는 에러가 난 근원지를 찾아 에러를 뽑아주는데, bad practice라는 의견이 있더라구요!
단순한 예외처리에 대한 trace는 노상관일 것 같지만요.. 읽어보시면 좋을 것 같은 자료 올려두겠습니다.. 이해해서 저도 좀 알려주세요 rein님! 😎😎
https://stackoverflow.com/questions/7469316/why-is-exception-printstacktrace-considered-bad-practice
| fun DoAndroidScreen( | ||
| navController: NavController, | ||
| ) { | ||
| val viewModel: DoAndroidViewModel = viewModel() |
There was a problem hiding this comment.
개인적으로 뷰모델은 파라미터로 두는 것이 여러가지로 이점이 있다고 생각합니다이 :)
3 participants
To Reviewers
Screen_recording_20240428_210835.mp4