Refactor: Move name validation to OnboardingViewModel#125
Refactor: Move name validation to OnboardingViewModel#125harrydbarnes wants to merge 1 commit into2.0from
Conversation
Moved the user name validation logic from OnboardingScreen (Composable) to OnboardingViewModel. Changed validation from isNotEmpty() to isNotBlank() to prevent saving whitespace-only names. This improves separation of concerns and testability.
Summary of ChangesHello @harrydbarnes, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the name validation process within the onboarding flow. It extracts the responsibility of validating user input from the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the user name validation logic from the OnboardingScreen composable to the OnboardingViewModel, which is a good architectural improvement. It also correctly changes the validation from isNotEmpty() to isNotBlank(). However, the implementation introduces a logic flaw where an invalid name submission fails silently, allowing the user to proceed through the onboarding flow without a valid name. My review includes a fix to provide feedback from the ViewModel to the UI, ensuring the user flow is handled correctly.
| fun saveName() { | ||
| viewModelScope.launch { | ||
| userPreferencesRepository.saveUserName(name.value) | ||
| if (name.value.isNotBlank()) { | ||
| viewModelScope.launch { | ||
| userPreferencesRepository.saveUserName(name.value) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Moving the name validation to the ViewModel is a great refactoring. However, the current implementation has a significant flaw: it fails silently.
When saveName() is called with a blank name, it does nothing. The calling code in OnboardingScreen.kt is unaware of this failure and proceeds to the next onboarding step. This allows the user to complete the name entry step without actually providing a valid name.
To fix this, saveName() should communicate the validation result back to the UI. A simple approach is to return a Boolean.
After making the suggested change below, you'll need to update OnboardingScreen.kt to use the return value to control the flow:
// In OnboardingScreen.kt's Button onClick handler
if (currentStep == 0) {
if (!viewModel.saveName()) {
Toast.makeText(context, "Name cannot be blank", Toast.LENGTH_SHORT).show()
return@Button // Prevent advancing to the next step
}
}
if (currentStep < totalSteps - 1) {
currentStep++
} else {
// ...
}This ensures the user cannot proceed with an invalid name.
fun saveName(): Boolean {
if (name.value.isNotBlank()) {
viewModelScope.launch {
userPreferencesRepository.saveUserName(name.value)
}
return true
}
return false
}
Moved the user name validation logic from OnboardingScreen (Composable) to OnboardingViewModel. Changed validation from isNotEmpty() to isNotBlank() to prevent saving whitespace-only names. This improves separation of concerns and testability.