Skip to content

Refactor SignInViewModel for better state management here #12

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.

Sign up for GitHub

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

Open
junior-od wants to merge 1 commit into
base: negus
Choose a base branch
from
Open

Refactor SignInViewModel for better state management here #12

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
67 changes: 16 additions & 51 deletions app/src/main/java/com/example/notesapp/ui/auth/signin/SignInViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,25 +32,18 @@ class SignInViewModel(
data class Error(val message: String) : SignInUi()
}

private var _signInUiState = MutableStateFlow<SignInUi?>(null)
// Issue 1: Mutable state exposed directly
var _signInUiState = MutableStateFlow<SignInUi?>(null)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

critical

Exposing MutableStateFlow as a public var breaks encapsulation. It allows any part of your app to not only change the state directly but also reassign the StateFlow object itself, leading to unpredictable behavior and bugs that are hard to trace. The backing property should be private val. Additionally, the public property signInUiState should expose an immutable StateFlow to prevent external modifications.

Suggested change
var _signInUiState = MutableStateFlow<SignInUi?>(null)
private val _signInUiState = MutableStateFlow<SignInUi?>(null)

val signInUiState get() = _signInUiState

private var _email = MutableStateFlow(
""
)

// Issue 2: No backing field protection
var _email = MutableStateFlow("")
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

critical

The _email MutableStateFlow is exposed as a public var. This breaks encapsulation and is not a safe way to handle UI state. It should be a private val, and the public email property should expose an immutable StateFlow.

Suggested change
var _email = MutableStateFlow("")
private val _email = MutableStateFlow("")

val email get() = _email

private var _password = MutableStateFlow(
""
)

private var _password = MutableStateFlow("")
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

medium

The backing property _password is declared as var. Since the MutableStateFlow instance itself is never reassigned (only its internal value), this should be a val for immutability.

Suggested change
private var _password = MutableStateFlow("")
private val _password = MutableStateFlow("")

val password get() = _password

private var _passwordVisible = MutableStateFlow(
false
)

private var _passwordVisible = MutableStateFlow(false)
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

medium

The backing property _passwordVisible is declared as var. It should be a val because the MutableStateFlow object is not being reassigned.

Suggested change
private var _passwordVisible = MutableStateFlow(false)
private val _passwordVisible = MutableStateFlow(false)

val passwordVisible get() = _passwordVisible

/**
Expand All @@ -67,39 +60,27 @@ class SignInViewModel(
_signInUiState.value = SignInUi.Error(message)
}

/**
* update email
*
* @param email: expects email
* */
fun updateEmail(email: String) {
_email.value = email
}

/**
* update password
*
* @param password: expects password
* */
fun updatePassword(password: String) {
_password.value = password
}

/**
* update password visibility
* */
fun updatePasswordVisible() {
_passwordVisible.value = !_passwordVisible.value
}

/**
* sign in user
* */
// Issue 3: Unnecessary suspend function + nested viewModelScope.launch
suspend fun signInUser(){
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

high

The signInUser function is marked as suspend, but it immediately launches a new coroutine on viewModelScope. This is redundant. A ViewModel function that starts a fire-and-forget coroutine should not typically be suspend. The caller in SignInScreen also wraps this in another launch, creating an unnecessary nested coroutine.

Suggested change
suspend fun signInUser(){
fun signInUser(){

viewModelScope.launch {
// update state as loading
_signInUiState.value = SignInUi.Loading

// Issue 4: No validation before sign in
signInUseCase(
email = _email.value,
password = _password.value,
Comment on lines 84 to 86
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

high

The sign-in process is initiated without validating the email and password. This can lead to unnecessary network requests with invalid data. You should add checks to ensure the fields are not empty before proceeding.

            if (_email.value.isBlank() || _password.value.isBlank()) {
                _signInUiState.value = SignInUi.Error("Email and password cannot be empty.")
                return@launch
            }

            signInUseCase(
                email = _email.value,
                password = _password.value,

Expand All @@ -108,72 +89,57 @@ class SignInViewModel(
result, isSuccessful ->

if (isSuccessful) {
// Issue 5: Unnecessary nested launch
viewModelScope.launch {
// observe changes and send to ui
// update state as sign up successful
_signInUiState.value = SignInUi.Success
}
Comment on lines 93 to 95
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

medium

This nested viewModelScope.launch is unnecessary. The onResultCallback is already executing within the coroutine launched by signInUser. You can update the _signInUiState value directly.

                            _signInUiState.value = SignInUi.Success

} else {
// observe changes and send to ui
// update state as Error occurred
_signInUiState.value = SignInUi.Error(result)
}

}
)
}

}

/**
* sign in user with google
* */
// Issue 6: No error handling for exceptions
suspend fun signInUserWithGoogle(credential: Credential){
viewModelScope.launch {
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

high

The code inside this viewModelScope.launch should be wrapped in a try-catch block. Any exception from signInUseCase (e.g., network error, invalid credential) will currently be unhandled at this level, which could crash the app. A catch block should update the UI state to show an error, for example: _signInUiState.value = SignInUi.Error(e.message ?: "An unknown error occurred").

// update state as loading
_signInUiState.value = SignInUi.Loading

signInUseCase(
credential = credential,
signInMethod = UserConstants.SignUpMethods.GOOGLE,
onResultCallback = {
result, _ ->

// observe changes and send to ui
// update state as Error occurred
_signInUiState.value = SignInUi.Error(result)

},
onGoogleSignInCallBack = {
user, _ ->
viewModelScope.launch {

// save user info to db
// Issue 7: Force unwrapping with !! could cause crash
saveUser(
userId = user?.uid.toString(),
userId = user!!.uid.toString(),
user = User(
id = "",
firstName = user?.displayName.toString(),
email = user?.email.toString()
firstName = user.displayName!!,
email = user.email!!
)
)

// observe changes and send to ui
// update state as sign up successful
_signInUiState.value = SignInUi.Success
Comment on lines 123 to 132
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

critical

Using the not-null assertion operator (!!) is unsafe and can cause a NullPointerException if the user object from Google Sign-In is null, or if its displayName or email properties are null. This will crash your app. You should handle the nullable values safely using safe calls and null checks.

                        user?.let { firebaseUser ->
                            saveUser(
                                userId = firebaseUser.uid,
                                user = User(
                                    id = "",
                                    firstName = firebaseUser.displayName ?: "",
                                    email = firebaseUser.email ?: ""
                                )
                            )
                            _signInUiState.value = SignInUi.Success
                        } ?: run {
                            _signInUiState.value = SignInUi.Error("Failed to retrieve user information.")
                        }

}
}
)
}

}

/**
* save user into db
*
* @param userId expects user id generated by firebase on sign in successful
* @param user expects user new record template
* */
// Issue 8: No error handling for database operations
private suspend fun saveUser(userId: String, user: User){
withContext(Dispatchers.IO){
createUserUseCase(
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

high

This call to createUserUseCase performs a database operation without any error handling. If the operation fails (e.g., due to a database constraint), it will throw an exception that will propagate up and crash the coroutine. You should wrap this call in a try-catch block to handle potential exceptions gracefully, at least by logging them.

Expand All @@ -182,6 +148,5 @@ class SignInViewModel(
signUpMethod = UserConstants.SignUpMethods.GOOGLE
)
}

}
}
}
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot Jan 8, 2026

Choose a reason for hiding this comment

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

medium

The file is missing a final newline character. It's a standard convention and good practice to end files with a newline. Many tools and editors expect this.