-
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
[#27] DetailHotelViewModel 추가 #29
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.
수고많으셨습니다. 몇개 코멘트를 달아놨으니 읽어보셔요
@@ -111,7 +111,8 @@ fun DetailIntroDTO.toHotelDetail(): HotelDetail = HotelDetail( | |||
checkInTime = checkInTime, | |||
checkOutTime = checkOutTime, | |||
subFacility = subFacility, | |||
reservationUrl = reservationUrl | |||
reservationUrl = reservationUrl, | |||
tel = infoCenterLodging |
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.
trailing comma 넣어 주시는 게 좋을 거 같아요
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.
data class
의 정의가 아닌 Mapper 함수라 따로 comma를 넣진 않았습니다.
} | ||
|
||
private fun getHotelDetail(contentId: String) { | ||
viewModelScope.launch { |
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.
왠지
viewModelScope.launch{ ... }
가 다른 곳에서도 반복되고 있을 거 같은데 혹시 그렇다면, launch 블록 안에서 실행될 함수를 파라메터로 받고 그 함수를 block 안에서 invoke 해 주는 메소드를 만들 수도 있을 거 같아요
import kr.ksw.visitkorea.presentation.detail.viewmodel.DetailViewModel | ||
import kr.ksw.visitkorea.presentation.ui.theme.VisitKoreaTheme | ||
|
||
@AndroidEntryPoint | ||
class DetailActivity : ComponentActivity() { | ||
private val viewModel: DetailViewModel by viewModels() | ||
private val hotelViewModel :DetailHotelViewModel by viewModels() |
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.
typo 를 수정하면 좋을 거 같습니다
} | ||
} | ||
|
||
val detail = intent.getParcelableExtra<DetailParcel>("detail") | ||
if(detail != 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.
detail 이 null 이라면 뷰모델이 init되지 않고 콜도 안 불릴 것 같은데, 여기에서의 예외처리를 해주면 좋을 거 같아요. 다이얼로그나 스낵바, 혹은 BottomSheet 등으로요.
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.
Error Screen을 따로 추가해봤습니다.
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.
수고하셨습니다
) { | ||
viewModelScope.launch { | ||
block() | ||
} |
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 fun getHotelRoomDetail(contentId: String) { | ||
viewModelLauncher { | ||
getHotelRoomDetailUseCase(contentId) | ||
.getOrNull()?.run { |
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.
UseCase에서의 Request 결과를 모두 getOrNull()
로 일단 필터링한 다음에 run { ... }
을 통해서 state 를 업데이트를 하고 있을까요 혹시?
만일 그렇다면 이 부분도 공통 core method 로 뺄 수 있을 거 같은데 생각 한 번 해봤으면 좋겠네요
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.
DetailViewModel과 HotelDetailViewModel에서만 이런식으로 사용하고 있습니다. 다른 ViewModel에서는 Null
인 경우 다른 분기를 타는경우도 있고, map이나 다른 함수로 연결되는 경우도 있어서 공통으로 뺀다면 우선 detail package 안에서 빼도 될 것같긴하네요
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.
왠지 null / non-null 인 경우를 생각해서 함수형으로 만들 수 있을 거 같다는 생각이 드네요
변경사항
문의사항