4단계 - 회원가입(리팩터링) #152
4단계 - 회원가입(리팩터링) #152
Conversation
There was a problem hiding this comment.
전반적으로 잘 분리를 해주시고 SignupButton 테스트 코드도 깔끔하게 해주셨습니다!
이제는 마지막 step이므로 디테일적으로 챙겨볼만한 것들이랑, 리컴포지션 관련해서도 추가로 확인해볼만한 사항이 있어서 리뷰를 드렸습니다! 혹시나 확인하다가 어려우신 부분이 생기면 언제든 연락 부탁드립니다!
SignupScreen 내부에 상태 제어를 위한 코드 구현부가 조금 장황하다고 느껴지는데 개선할 만한 아이디어가 있을까요? / 혹은 Compose 사용하면서 어떤 방식으로 풀어내시는지 궁금합니다.
SignupScreen 내부에서 상태 제어를 하는 코드가 있는 것이 아무래도 지금 겨우 이정도 View인데도 장황해지니 조금 어색하실수도 있는데요.
상태를 저장하는 Holder를 만들기도 하는데요 이것을 StateHolder라고도 부릅니다.
관련해서 간단한 미디엄 글 하나 추천 드립니다.
| class SignupButtonTest { | ||
| @get:Rule | ||
| val composeTestRule = createComposeRule() |
There was a problem hiding this comment.
지극히 개인적인 의견입니다.
SingUpButton의 관점에서 모든 입력폼이 유효한지 , 모든 입력폼이 유효하지 않은지 가 관심사는 아니라고 생각을 해요. 이 관심사는 SingUpScreen 이 갖고 있다고 생각을 하는데요. 그럼 여기 있는 테스트 코드들은 SignUpScreen의 테스트 코드인지 SignUpButton의 테스트 코드인지 한 번 고민해보면 좋을거 같네요.
테스트 코드 자체는 더할나위 없이 완벽합니다.
There was a problem hiding this comment.
그리고 번외로 Snackbar가 뜨는지도 테스트해보면 좋을거 같네요!
| val validationResults = mapOf( | ||
| USER_NAME to NameValidator.validate(username), | ||
| EMAIL to EmailValidator.validate(email), | ||
| PASSWORD to PasswordValidator.validate(password), | ||
| PASSWORD_CONFIRM to PasswordMatchValidator.validate(password, passwordConfirm) | ||
| ) |
There was a problem hiding this comment.
이렇게 map으로 관리하면서 어떤 이점이 있을까요?
username이 변경이 될때도 validationResults 이 새롭게 map이 생성이 되는 것을 볼 수가 있는데요.
이 부분을 확인하시고 싶다면
EmailValidator.validate내부 로그를 넣어두고 Username만 변경해보시면 됩니다.
이 부분을 한 번 고민해시고 리팩토링 해보시면 좋을거 같습니다!
| SnackbarHost( | ||
| hostState = snackBarHostState, | ||
| snackbar = { data -> | ||
| Snackbar( | ||
| snackbarData = data, | ||
| actionColor = Color.Red | ||
| ) | ||
| } | ||
| ) |
There was a problem hiding this comment.
Scaffold에는 SnackbarHost를 등록하는 자리가 존재합니다.
Scaffold에 SnackbarHost를 등록했을 때의 장점이 여러가지가 있기에 적용해보시는것을 추천드립니다.
| @@ -17,17 +16,18 @@ import nextstep.signup.ui.theme.SignupBlue | |||
| @Composable | |||
| fun SubmitButton( | |||
| onClick: () -> Unit = {}, | |||
| enabled: Boolean = true, | |||
There was a problem hiding this comment.
모든 필드가 에러 없이 채워진 경우에만 Sign up 버튼을 활성화한다.
이 조건을 보면 기본값은 false가 맞아 보이는데 확인 부탁드려요!
| @Composable | ||
| fun EmailTextField( | ||
| inputValue: String = "", | ||
| onInputChange: (String) -> Unit = {}, | ||
| validResult: ValidationResult, | ||
| modifier: Modifier = Modifier, | ||
| ) { |
There was a problem hiding this comment.
https://blog.simprasuite.com/jetpack-compose-respect-the-contract-of-modifiers-ecbbe8ce03db
보편적인 Composable 필수/선택 파라미터들에 대한 순서인데 이제는 이런 디테일도 한 번씩 신경써서 코딩해보는건 어떨까요!
| inputValue: String = "", | ||
| onInputChange: (String) -> Unit = {}, |
There was a problem hiding this comment.
TextField 들에서 이 값들은 전부 필수적으로 받아야한다. 생각하는데 기본값을 주신 이유가 있을까요?
| Scaffold { innerPadding -> | ||
| SignupScreen( | ||
| modifier = Modifier.padding(innerPadding) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Scaffold 사용 좋습니다! Snackbar 쪽에 리뷰를 달아뒀는데 Scaffold 파라미터로 SncakbarHost를 등록이 가능합니다!
Overview
아래 요구사항을 구현하였습니다.
[v] 모든 SignupForm을 하나로 관리하지 않고 역할을 분리한다.
[v] 모든 필드가 에러 없이 채워진 경우에만 Sign up 버튼을 활성화한다.
[v] Sign up 버튼을 클릭하면 회원가입 완료 스낵바가 노출된다.
Question