Skip to content
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

Step4 - 페이먼츠(카드 수정) #61

Open
wants to merge 9 commits into
base: eshc123
Choose a base branch
from

Conversation

eshc123
Copy link

@eshc123 eshc123 commented Sep 12, 2024

주요 내용

  • 카드 수정 기능 구현하였습니다.
  • 이전 PR에서 피드백 주신 내용 반영해보았습니다.
    • BankTypeUiModel 을 nullable 하지 않게 구현하는 것은 이번 PR에는 담지 못했는데 null 이 허용되는 것보다 기본 상태 혹은 선택되지 않은 상태를 통해 처리하는 것이 더 명확하게 어떤 상태인지를 파악할 수 있을 것 같다고 생각이 듭니다!

Copy link

@namjackson namjackson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 석준님!
4단계 고생하셨습니다! 리뷰가 조금 늦었네요!
끝이 얼마남지않았네요! 몇가지 추가적인 코멘트 남겼으니 확인해주세요!
끝까지 화이팅입니다!

Comment on lines +28 to +41
private val _cardNumber = MutableStateFlow(cardArgType.value.creditCardToEdit?.cardNumber ?: "")
val cardNumber: StateFlow<String> = _cardNumber.asStateFlow()

private val _expiredDate =
MutableStateFlow(cardArgType.value.creditCardToEdit?.expiredDate ?: "")
val expiredDate: StateFlow<String> = _expiredDate.asStateFlow()

private val _ownerName = MutableStateFlow(cardArgType.value.creditCardToEdit?.ownerName ?: "")
val ownerName: StateFlow<String> = _ownerName.asStateFlow()

private val _password = MutableStateFlow(cardArgType.value.creditCardToEdit?.password ?: "")
val password: StateFlow<String> = _password.asStateFlow()

private val _bankType = MutableStateFlow(cardArgType.value.creditCardToEdit?.bankType?.toUiModel())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 많은 state가 전달되고 있는건 아닐까요?
하나의 uiState로 외부에 노출하면 어떤 장점이 있을까요?

Comment on lines +23 to +26
val cardArgType = savedStateHandle.getStateFlow<CardArgType>(
CardArgType.MANAGE_CARD_TYPE_ARG,
CardArgType.AddCardArg
)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

매번 cardArgType.value.creditCardToEdit?을 통해 creditCard에 접근하고 있네요,
사실 필요한건 CardArgType 이 아닌 creditCard가 아닐까요?

@Parcelize
data class EditCardArg(val creditCard: CreditCard) : CardArgType()

val creditCardToEdit : CreditCard? by lazy {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이미 Add일땐 null이 반환되고 있기 때문에
ToEdit이라는 접미사가 불필요하진않을까요?

import nextstep.payments.ui.theme.PaymentsTheme


@OptIn(ExperimentalMaterial3Api::class)
@Composable
internal fun NewCardRouteScreen(
internal fun ManageCardRouteScreen(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

석준님이 생각하는 RouteScreen의 역할은 무엇일까요?
저는 개인적으로 데이터 전달, 상태관리, 네비게이션등의 책임을 전달하는 편인거같아요 :)
RouteScreen가 커진건 아닌지 고민해보셔도 좋을거같아요!

@Test
fun 카드_수정_화면에서_변경사항이_발생하지_않으면_저장_버튼을_클릭할_수_없다() {
//GIVEN
viewModel = ManageCardViewModel(

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

해당 테스트들이 ManageCardViewModel의 의존적이라고도 볼수 있을거같아요
Screen과 ViewModel을 분리해서 테스트하면 조금더 독립적인 테스트코드를 작성할수 있을거같아요 :)
( 이부분은 고민 만해주셔도 좋을거같아요!)

Comment on lines +48 to +49
val manageCardType by viewModel.cardArgType
.map { it.toManageCardType() }.collectAsStateWithLifecycle(ManageCardType.ADD)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toManageCardType는 Ui의 관심사와는 별개는 아닐까요?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants