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

[Feat] 7주차 필수과제 #13

Open
wants to merge 6 commits into
base: week6-basic
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link

Choose a reason for hiding this comment

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

저는 State, Event, SideEffect, 뿐만 아니라 SignIn, SignUp까지 다 분리하는 것이 좋다(?) 라고 생각하고 구현을 했었는데, 재원이 오빠도 그렇고 민재오빠도 이렇게 구현한 걸 보고 더 가독성이 좋고, 한눈에 보기 편하다는 생각이 들었어요! 주요 기능에 따라 이렇게 묶어둔 것 보고 리팩토링 때 이렇게 적용해보고자 해요 ㅎ 감삼둥

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 하나의 뷰모델을 여러 뷰에서 나누어서 사용하는 것은 그렇게 권장되는 방식은 아닙니다. 하나의 뷰 모델을 여러 뷰에서 나누어 사용하게 되면 해당 뷰모델을 공유하는 뷰들간 의존성이 커지게 됩니다. 여기서를 예로 들자면 SignIn에만 필요한 코드들이 SignViewModel에 들어가 있어 SignUp 뷰도 이에 대한 불필요한 의존성을 가지게 됩니다. 또한, 이를 합치게 되면서 ViewModel이 방대해지는 문제도 생기고요. (+ 그밖에 다양한 문제들도 있는데 생각해보시면 좋을 것 같습니다.) 따라서 특별한 경우가 아니라면 나누어서 사용하시는 것을 권장합니다.

Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package org.sopt.and.presentation.ui.auth.screen

import org.sopt.and.presentation.util.base.UiEvent
import org.sopt.and.presentation.util.base.UiSideEffect
import org.sopt.and.presentation.util.base.UiState

class AuthContract {
data class AuthUiState(
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 두 방식 다 장단점이 있을 것 같아요. 일단 지금 사용하신 방식은 데이터에 접근할 때에 한 층 더 depth가 있기 때문에 데이터에 접근하는 방식이 조금 더 복잡해집니다. 하지만, UI상태와 데이터를 더 명확히 관리할 수 있죠. (UIState에 따라 이에 맞는 데이터를 가지고 있을 수있으니) 반면, 해당 값을 상태에서 빼게 되면 데이터에 접근하는 것이 더 편리하지만 UI 상태와 데이터를 따로 관리해야 합니다. 각각의 이점에 따라 어떤 방식을 사용하면 좋을지 고민한 뒤 사용하시면 좋을 것 같아요!

val signInState: SignInState = SignInState.Idle,
val signInUsername: String = "",
val signInPassword: String = "",
val signUpState: SignUpState = SignUpState.Idle,
val signUpUsername: String = "",
val signUpPassword: String = "",
val signUpHobby: String = "",
) : UiState

sealed class AuthEvent : UiEvent {
data class UpdateSignInUsername(val signInUsername: String) : AuthEvent()
data class UpdateSignInPassword(val signInPassword: String) : AuthEvent()
Comment on lines +19 to +20
Copy link
Collaborator

Choose a reason for hiding this comment

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

1번에 대해서 말씀드리자면,
저는 값이 바뀌는 건 뷰모델에서 바로 사용하는 편입니다. 중간에 이벤트가 끼게 되면 코드가 길어지게 되어 가독성을 해친다구 생각해서요.

fun updateSignUpUsername(name){
 setState {
               copy(signUpUsername = name)
          }
}

와 같이 하나의 함수에서 처리할 수 있는 것 불필요한 객체를 참조한다고 여겨져서 혹시 Event에 사용 목적이 따로있으신가요

Copy link
Collaborator

Choose a reason for hiding this comment

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

값이 바뀔 때 다른 무언가 처리를 한다거나 해야한다면 이벤트를 쓰는 것도 좋다고 생각하는데 아직은 잘 안보여서요

data class OnSignInClicked(val signInState: SignInState) : AuthEvent()
data object ResetSignInState : AuthEvent()
data class UpdateSignUpUsername(val signUpUsername: String) : AuthEvent()
data class UpdateSignUpPassword(val signUpPassword: String) : AuthEvent()
data class UpdateSignUpHobby(val signUpHobby: String) : AuthEvent()
data class OnSignUpClicked(val signUpState: SignUpState) : AuthEvent()
data object ResetSignUpState : AuthEvent()
}

sealed interface AuthSideEffect : UiSideEffect {
data object NavigateToSignIn : AuthSideEffect
data object NavigateToSignUp : AuthSideEffect
data object NavigateToMain : AuthSideEffect
data class ShowToast(val message: String) : AuthSideEffect
}
}
Original file line number Diff line number Diff line change
@@ -1,73 +1,115 @@
package org.sopt.and.presentation.ui.auth.screen

import android.util.Log
import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.launch
import org.sopt.and.domain.model.User
import org.sopt.and.domain.repository.AuthRepository
import org.sopt.and.domain.repository.TokenRepository
import org.sopt.and.presentation.util.base.BaseViewModel
import javax.inject.Inject

@HiltViewModel
class AuthViewModel @Inject constructor(
private val tokenRepository: TokenRepository,
private val authRepository: AuthRepository,
) : ViewModel() {
private val _signInState = MutableStateFlow<SignInState>(SignInState.Idle)
val signInState: StateFlow<SignInState> = _signInState
) : BaseViewModel<AuthContract.AuthUiState, AuthContract.AuthSideEffect, AuthContract.AuthEvent>() {
override fun createInitialState(): AuthContract.AuthUiState = AuthContract.AuthUiState()

private val _token = MutableStateFlow("")
val token: StateFlow<String> = _token
override suspend fun handleEvent(event: AuthContract.AuthEvent) {
when (event) {
is AuthContract.AuthEvent.UpdateSignInUsername -> setState {
copy(signInUsername = event.signInUsername)
}
is AuthContract.AuthEvent.UpdateSignInPassword -> setState {
copy(signInPassword = event.signInPassword)
}
is AuthContract.AuthEvent.OnSignInClicked -> setState {
copy(signInState = event.signInState)
}
is AuthContract.AuthEvent.ResetSignInState -> setState {
copy(signInState = SignInState.Idle)
}
is AuthContract.AuthEvent.UpdateSignUpUsername -> setState {
copy(signUpUsername = event.signUpUsername)
}
is AuthContract.AuthEvent.UpdateSignUpPassword -> setState {
copy(signUpPassword = event.signUpPassword)
}
Comment on lines +33 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. 텍스트에 입력 받는 값을 상태로 두었으면 해당 상태도 지금처럼 handleEvent 함수 내부에서 업데이트 시켜주시는 것이 올바릅니다. MVI를 사용하는 이유 중 하나가 상태를 한 곳에서 관리해서 상태 변화를 효과적으로 관리하기 위함인데요. (세미나 자료를 참고하시면 좋을 것 같네요.) 이를 효과적으로 처리하기 위해서는 reducer를 통해 상태를 한 곳에서만 변경해주어야 합니다.

is AuthContract.AuthEvent.UpdateSignUpHobby -> setState {
copy(signUpHobby = event.signUpHobby)
}
is AuthContract.AuthEvent.OnSignUpClicked -> setState {
copy(signUpState = event.signUpState)
}
is AuthContract.AuthEvent.ResetSignUpState -> setState {
copy(signUpState = SignUpState.Idle)
}
}
}

private val _signUpState = MutableStateFlow<SignUpState>(SignUpState.Idle)
val signUpState: StateFlow<SignUpState> = _signUpState
fun updateSignInUsername(input: String) {
viewModelScope.launch {
Copy link
Contributor

Choose a reason for hiding this comment

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

viewModelScope로 감싼 이유가 있나요?

setEvent(AuthContract.AuthEvent.UpdateSignInUsername(signInUsername = input))
}
}

fun validateSignIn(username: String, password: String) {
fun updateSignInPassword(input: String) {
viewModelScope.launch {
_signInState.value = SignInState.Loading
val result = authRepository.login(username = username, password = password)
_signInState.value = result.fold(
setEvent(AuthContract.AuthEvent.UpdateSignInPassword(signInPassword = input))
}
}

fun validateSignIn() {
viewModelScope.launch {
setEvent(AuthContract.AuthEvent.OnSignInClicked(signInState = SignInState.Loading))
authRepository.login(username = currentState.signInUsername, password = currentState.signInPassword).fold(
onSuccess = { token ->
tokenRepository.setToken(token.token)
SignInState.Success(token)
setEvent(AuthContract.AuthEvent.OnSignInClicked(signInState = SignInState.Success(result = token)))
},
onFailure = {
SignInState.Failure(it.localizedMessage ?: "에러 발생")
setEvent(AuthContract.AuthEvent.OnSignInClicked(signInState = SignInState.Failure(errorMessage = it.localizedMessage?: "")))
}
)
}
}

fun validateSignUp(username: String, password: String, hobby: String) {
fun updateSignUpUsername(input: String) {
viewModelScope.launch {
setEvent(AuthContract.AuthEvent.UpdateSignUpUsername(signUpUsername = input))
}
}

fun updateSignUpPassword(input: String) {
viewModelScope.launch {
_signUpState.value = SignUpState.Loading
val result = authRepository.registerUser(
setEvent(AuthContract.AuthEvent.UpdateSignUpPassword(signUpPassword = input))
}
}

fun updateSignUpHobby(input: String) {
viewModelScope.launch {
setEvent(AuthContract.AuthEvent.UpdateSignUpHobby(signUpHobby = input))
}
}

fun validateSignUp() {
viewModelScope.launch {
setEvent(AuthContract.AuthEvent.OnSignUpClicked(signUpState = SignUpState.Loading))
authRepository.registerUser(
user = User(
username = username,
password = password,
hobby = hobby
username = currentState.signUpUsername,
password = currentState.signUpPassword,
hobby = currentState.signUpHobby
)
)
_signUpState.value = result.fold(
onSuccess = {
SignUpState.Success(it)
).fold(
onSuccess = { userNo ->
setEvent(AuthContract.AuthEvent.OnSignUpClicked(signUpState = SignUpState.Success(userNo)))
},
onFailure = {
SignUpState.Failure(it.localizedMessage ?: "에러 발생")
setEvent(AuthContract.AuthEvent.OnSignUpClicked(signUpState = SignUpState.Failure(it.localizedMessage ?: "")))
}
)
}
}

fun resetSignInState() {
_signInState.value = SignInState.Idle
}

fun resetSignUpState() {
_signUpState.value = SignUpState.Idle
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,8 @@ import androidx.compose.material3.ButtonDefaults
import androidx.compose.material3.Text
import androidx.compose.material3.VerticalDivider
import androidx.compose.runtime.Composable
import androidx.compose.runtime.collectAsState
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf
import androidx.compose.runtime.remember
import androidx.compose.runtime.setValue
import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip
Expand All @@ -36,10 +33,13 @@ import androidx.compose.ui.tooling.preview.Preview
import androidx.compose.ui.unit.dp
import androidx.compose.ui.unit.sp
import androidx.hilt.navigation.compose.hiltViewModel
import androidx.lifecycle.compose.LocalLifecycleOwner
import androidx.lifecycle.compose.collectAsStateWithLifecycle
import androidx.lifecycle.flowWithLifecycle
import org.sopt.and.R
import org.sopt.and.presentation.ui.common.WavveTextField
import org.sopt.and.presentation.ui.auth.component.SocialPlatformIconRow
import org.sopt.and.presentation.ui.auth.component.SocialPlatformList
import org.sopt.and.presentation.ui.common.WavveTextField
import org.sopt.and.presentation.util.showToast
import org.sopt.and.ui.theme.ANDANDROIDTheme

Expand All @@ -49,29 +49,60 @@ fun SignInRoute(
navigateToSignUp: () -> Unit,
navigateToMain: () -> Unit,
) {
val signInState by authViewModel.signInState.collectAsState()
val authUiState by authViewModel.uiState.collectAsStateWithLifecycle()
val lifecycleOwner = LocalLifecycleOwner.current
val context = LocalContext.current

LaunchedEffect(authViewModel.sideEffect, lifecycleOwner) {
authViewModel.sideEffect.flowWithLifecycle(lifecycle = lifecycleOwner.lifecycle)
.collect { authSideEffect ->
when (authSideEffect) {
is AuthContract.AuthSideEffect.NavigateToSignUp -> navigateToSignUp()
is AuthContract.AuthSideEffect.NavigateToMain -> navigateToMain()
is AuthContract.AuthSideEffect.ShowToast -> showToast(
context = context,
message = authSideEffect.message
)

else -> {}
}
}
}

LaunchedEffect(authUiState.signInState) {
when (authUiState.signInState) {
is SignInState.Success -> {
authViewModel.setSideEffect(AuthContract.AuthSideEffect.ShowToast(message = "로그인에 성공하였습니다."))
authViewModel.setEvent(AuthContract.AuthEvent.ResetSignInState)
authViewModel.setSideEffect(AuthContract.AuthSideEffect.NavigateToMain)
}

is SignInState.Failure -> {
authViewModel.setSideEffect(AuthContract.AuthSideEffect.ShowToast(message = "아이디와 비밀번호를 다시 확인해주세요."))
authViewModel.setEvent(AuthContract.AuthEvent.ResetSignInState)
}

else -> {}
}
}

SignInScreen(
signInState = signInState,
resetSignInState = { authViewModel.resetSignInState() },
onSignUpClick = navigateToSignUp,
onSignInClick = { username, password -> authViewModel.validateSignIn(username, password) },
navigateToMain = navigateToMain
authUiState = authUiState,
updateSignInUsername = { input -> authViewModel.updateSignInUsername(input) },
updateSignInPassword = { input -> authViewModel.updateSignInPassword(input) },
onSignInClick = { authViewModel.validateSignIn() },
onSignUpClick = { authViewModel.setSideEffect(AuthContract.AuthSideEffect.NavigateToSignUp) },
)
}

@Composable
fun SignInScreen(
signInState: SignInState,
resetSignInState: () -> Unit,
authUiState: AuthContract.AuthUiState,
updateSignInUsername: (String) -> Unit,
updateSignInPassword: (String) -> Unit,
onSignInClick: () -> Unit,
onSignUpClick: () -> Unit,
onSignInClick: (String, String) -> Unit,
navigateToMain: () -> Unit,
) {
val context = LocalContext.current
var inputEmail by remember { mutableStateOf("") }
var inputPassword by remember { mutableStateOf("") }

Column(
modifier = Modifier
.fillMaxSize()
Expand Down Expand Up @@ -105,8 +136,8 @@ fun SignInScreen(
Spacer(Modifier.height(40.dp))

WavveTextField(
value = inputEmail,
onValueChange = { newValue -> inputEmail = newValue },
value = authUiState.signInUsername,
onValueChange = updateSignInUsername,
modifier = Modifier
.fillMaxWidth()
.clip(shape = RoundedCornerShape(6.dp))
Expand All @@ -115,8 +146,8 @@ fun SignInScreen(
)
Spacer(Modifier.height(4.dp))
WavveTextField(
value = inputPassword,
onValueChange = { newValue -> inputPassword = newValue },
value = authUiState.signInPassword,
onValueChange = updateSignInPassword,
modifier = Modifier
.fillMaxWidth()
.clip(shape = RoundedCornerShape(6.dp))
Expand All @@ -128,7 +159,7 @@ fun SignInScreen(
Spacer(Modifier.height(30.dp))

Button(
onClick = { onSignInClick(inputEmail, inputPassword) },
onClick = onSignInClick,
modifier = Modifier.fillMaxWidth(),
colors = ButtonDefaults.buttonColors(Color(0xFF0F42C7))
) {
Expand Down Expand Up @@ -207,7 +238,8 @@ fun SignInScreen(
color = Color(0xFF5D5D5D)
)
Text(
text = "SNS계정으로 간편하게 가입하여 서비스를 이용하실 수 있습니다.\n기존 POOQ 계정 또는 Wavve 계정과는 연동되지 않으니 이용에 참고하세요",
text = "SNS계정으로 간편하게 가입하여 서비스를 이용하실 수 있습니다.\n" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

스트링 값 분리하면 더 좋읗 거 같아요

"기존 POOQ 계정 또는 Wavve 계정과는 연동되지 않으니 이용에 참고하세요",
modifier = Modifier.fillMaxWidth(),
fontSize = 10.sp,
lineHeight = 14.sp,
Expand All @@ -216,27 +248,6 @@ fun SignInScreen(
}

Spacer(modifier = Modifier.weight(1f))

when (signInState) {
is SignInState.Success -> {
showToast(
context = context,
message = "로그인에 성공했습니다."
)
resetSignInState()
navigateToMain()
}

is SignInState.Failure -> {
showToast(
context = context,
message = "아이디와 비밀번호를 다시 확인해주세요."
)
resetSignInState()
}

else -> {}
}
}
}

Expand Down
Loading