[Refactor] 앱잼 전 세미나 코드 리팩토링#22
Conversation
leeeyubin
left a comment
leeeyubin
left a comment
There was a problem hiding this comment.
코드 리팩토링 하느라 수고 많았어요!!! 최고~~!
리뷰 달린 거 한번씩 확인해주시구 개발 시작해봅시다!!
| @Provides | ||
| @Singleton | ||
| fun followerRetrofit(): Retrofit { | ||
| return Retrofit.Builder() | ||
| .baseUrl(FOLLOWER_URL) | ||
| .addConverterFactory(Json.asConverterFactory("application/json".toMediaType())) | ||
| .addConverterFactory(jsonConverterFactory) | ||
| .build() | ||
| } | ||
|
|
||
| inline fun <reified T> createBaseRetrofit(): T = baseRetrofit.create() | ||
| inline fun <reified T> createFollowerRetrofit(): T = followerRetrofit.create() | ||
| } | ||
|
|
||
| object ServicePool { | ||
| val authService = ApiFactory.createBaseRetrofit<AuthService>() | ||
| val followerService = ApiFactory.createFollowerRetrofit<FollowerService>() | ||
| @Provides | ||
| @Singleton | ||
| fun provideFollowerService(followerRetrofit: Retrofit): FollowerService { | ||
| return followerRetrofit.create(FollowerService::class.java) | ||
| } |
There was a problem hiding this comment.
현재 레포지토리 모듈과 서비스 모듈을 같은 파일에 넣어둔 것 같은데,
서로 다른 기능을 하는 모듈이면 다른 파일로 분류하는 게 좋을 것 같아요!
지금은 함수가 하나씩 존재하지만 여러 개의 함수가 있다면 헷갈릴 수 있으니까요!
| try { | ||
| val response = followerRepository.getUserList(0) | ||
| if (response.isSuccessful) { | ||
| val data = response.body()?.data | ||
| if (data != null) { | ||
| response.body()?.data?.let { data -> | ||
| _followerState.value = data | ||
| mapFollowersToFriendList(data) | ||
| } | ||
| _eventNetworkError.value = false | ||
| _isNetworkErrorShown.value = false | ||
| } else { | ||
| _eventNetworkError.value = true | ||
| } | ||
| } catch (networkError: IOException) { | ||
| _eventNetworkError.value = true | ||
| Log.e("HomeError", "${networkError.message}") |
There was a problem hiding this comment.
try~catch문 말고 세미나에서도 배운 runCatching을 사용해보는 건 어떨까요?!
코드의 가독성이 더 좋아진답니다~!
There was a problem hiding this comment.
참고로 터닝에서는 레포지토리 패턴을 사용하기 때문에 RepositoryImpl 파일에서 runCatching을 사용해준다면 뷰모델에서는 따로 사용해줄 필요가 없답니다!
There was a problem hiding this comment.
훨씬 좋아지는 것 같아요 ㅜㅜ 좋은 지적 감사합니다!
| private var _eventNetworkError = MutableLiveData(false) | ||
|
|
||
| private var _isNetworkErrorShown = MutableLiveData(false) |
There was a problem hiding this comment.
혹시 두 변수들이 어디서 쓰이고 있는 건가용..? 이 뷰모델에서 true 혹은 false로 바꿔주고 그 이후에 사용되는 부분을 못 찾아서요!!
There was a problem hiding this comment.
fetchFollowerList에서 네트워크 오류 발생시 쓰이는데, 사실 삭제해도 될 것 같아욥!!!
| fun HomeScreen(homeViewModel: HomeViewModel = hiltViewModel()) { | ||
| val followerState by homeViewModel.followerState.collectAsState() |
There was a problem hiding this comment.
flow를 사용한다면 collectAsStateWithLifecycle로 사용해주는 걸 습관화하면 좋을 것 같아요!
| return withContext(Dispatchers.IO) { | ||
| followerService.getUserList(page).execute() |
There was a problem hiding this comment.
withContext(Dispatchers.IO)는 어떤 역할을 하는 코드인가요...??
There was a problem hiding this comment.
서버로 네트워크 요청 보내서 파일 받아올 때, 메인 스레드가 아닌 별도 스레드에서 실행시키고자 작성했습니다!
| class FollowerRepository @Inject constructor( | ||
| private val followerService: FollowerService, |
There was a problem hiding this comment.
굿굿 좋아요! 다만 아직 클린아키텍처가 적용되어 있지 않은 것 같아서 domain 레이어를 추가했을 때 코드가 어떻게 수정될 지 한번 생각해보면 좋을 것 같아요!!
| _eventNetworkError.value = true | ||
| Log.e("HomeError", "${networkError.message}") |
| if (response.isSuccessful) { | ||
| val data = response.body()?.data | ||
| if (data != null) { | ||
| response.body()?.data?.let { data -> |
There was a problem hiding this comment.
제가 생각하기에 response가 success인 경우라면 null이 아닐 것 같은데 nullable로 선언해준 이유가 있나용...?
There was a problem hiding this comment.
성공적인 응답일 때에도 간혹 데이터가 없을 수 있어서 (post 요청 등....!) 일단 nullable로 처리했는데, 터닝에서는 굳이 필요 없을 것 같습니당..!!!!
boiledeggg
left a comment
boiledeggg
left a comment
There was a problem hiding this comment.
리드님이 코리를 열심히 다셔서 딱히 달게 없네요ㅎㅎ
리뷰 보완하면 완벽할 것 같습니다!!!
3 participants
🧩 Issue number
이슈 번호 : #21
✨ Summary
Hilt로 의존성 주입을 공부하면서 적용해보았습니다.
멀티모듈까지 적용하려 했으나,, 버전 이슈로 실행조차 안돼서
일단 힐트라도 집중해서 적용해보고자,,,,,했습니다 멀티모듈은 앱잼하면서 본격적으로 달려볼게요 😭
🔍 PR Type
📷 Screenshot
Screen_recording_20240701_210118.mp4
📔 Other Information