[FEAT/#12] 7주차 과제 #13
Conversation
hwidung
requested review from
a team,
Marchbreeze,
SYAAINN,
cacaocoffee and
jangsjw
and removed request for
a team
SYAAINN
left a comment
SYAAINN
left a comment
There was a problem hiding this comment.
시험기간에 어려운 MVI 과제까지 하시느라 고생하셨습니다! 아무래도 잠시 과제를 쉬었다가 하다보니 코드 보기가 힘들었을 것 같아요. 전체적으로 변수명이나 함수명 같은 네이밍들을 조금 더 일관성 있게 가져가기 위해 노력하면 가독성이 훨씬 좋아질 수 있을것 같습니다😀
| when (intent) { | ||
| is SignInIntent.EnterUsername -> updateState { it.copy(email = intent.username) } | ||
| is SignInIntent.EnterPassword -> updateState { it.copy(password = intent.password) } | ||
| is SignInIntent.TogglePasswordVisibility -> togglePasswordVisibility() |
There was a problem hiding this comment.
[3]
바로 람다식으로 들어가지 않고 함수 분리를 진행한 것은 가독성을 위함인가요?!
|
|
||
| result.onSuccess { tokenEntity -> | ||
| sharedPreferencesHelper.saveToken(tokenEntity.token) | ||
| updateState { it.copy(isLoading = false, isSuccess = true) } |
There was a problem hiding this comment.
[2]
서버통신에 대한 State를 각각의 변수로 관리하기보다는 하나의 State로 묶어서 관리하는 것은 어떨까요? 관리의 효율성을 챙길 수 있을 것 같습니다!
| signInViewModel.sideEffect.collect { effect -> | ||
| when (effect) { | ||
| is SignInSideEffect.ShowToast -> { | ||
| snackbarHostState.showSnackbar(effect.message) |
| } | ||
|
|
||
| is SignInSideEffect.NavigateToMain -> { | ||
| navController.navigate(AuthNavItem.Main.route) |
There was a problem hiding this comment.
[3]
이 부분은 상위 NavGraph로 구현부분을 옮기고 navigateToMin으로 받아와서 사용하면 일관성이나, 관리 측면에서 더 효율적으로 진행될 수 있을 것 같아요!
| private fun validateSignUp(username: String, password: String, hobby: String): Boolean { | ||
| return username.length <= 8 && password.length <= 8 && hobby.length <= 8 | ||
| } |
There was a problem hiding this comment.
[3]
이 부분은 서버에서 담당하는 것으로 알고있기에, 직접 이 작업을 해주기보단 이 조건을 어떻게 사용자에게 보여줄 지를 고민해보면 될 것 같습니다!
cacaocoffee
left a comment
cacaocoffee
left a comment
There was a problem hiding this comment.
긴시간 동안 수고하셨어요!
멘토를 담당하면서 많이 도움을 못 드린 것 같아 아쉬움이 남네요
코드가 깔끔하고 보기 좋은 거 같아요
string 값들과 관련 된 부분들만 분리하는 것 좀 더 신경쓰면 더 좋을 것 같습니다.
수고많으셨어요, 연말잘보내시구,앱잼 화이팅하세요!
| override suspend fun getHobby(token: String): Response<HobbyResponseDto> { | ||
| return hobbyService.getHobby(token) | ||
| } |
There was a problem hiding this comment.
요 부분은 아래처럼 줄일 수 있을 거 같아요
override suspend fun getHobby(token: String)= hobbyService.getHobby(token) | value = signUpViewModel.username, | ||
| onValueChange = { signUpViewModel.username = it }, | ||
| value = state.email, | ||
| onValueChange = { signUpViewModel.processIntent(SignUpIntent.EnterUsername(it)) }, |
There was a problem hiding this comment.
SignUpIntent.EnterUsername 와 같은 이름보단 Update~~ 로 하는 게 이름이 좀더 적합한거 같아요
| Result.success(response.body()!!) | ||
| val body = response.body() | ||
| if (body != null) { | ||
| Result.success(body); |
There was a problem hiding this comment.
| Result.success(body); | |
| Result.success(body) |
| when (intent) { | ||
| is SignInIntent.EnterUsername -> updateState { it.copy(email = intent.username) } | ||
| is SignInIntent.EnterPassword -> updateState { it.copy(password = intent.password) } | ||
| is SignInIntent.TogglePasswordVisibility -> togglePasswordVisibility() |
Related issue 🛠
Work Description ✏️
Screenshot 📸
2024-12-19.5.17.54.mov
Uncompleted Tasks 😅
To Reviewers 📢