A new Android Pull Request #2
Conversation
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
|
|
||
| setContent { |
There was a problem hiding this comment.
18 : ViewModel should not be instantiated directly in the activity . ViewModel should be retrieved via ViewModel Provider to correctly handle lifecycle states and to share data between different components such as activities and fragments .
| Surface( | ||
| modifier = Modifier.fillMaxSize(), | ||
| color = MaterialTheme.colorScheme.background | ||
| ) { |
There was a problem hiding this comment.
24 : Could potentially crash due to Network On MainThread Exception . The :: load State method usually loads data from network or database which cannot be performed on Main Thread , so it should be executed within a coroutine scope .
| modifier = Modifier.fillMaxSize(), | ||
| color = Color(0xFFFFFFFF) | ||
| ) { | ||
| MainScreen(state, viewModel::loadState, ::openNerdyThings) |
There was a problem hiding this comment.
32 : Activity context being passed to the ViewModel in the prepare Text (this ) method . It might cause memory leaks if not handled properly . ViewModel should never have a reference to an activity , use application context instead .
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.launch | ||
|
|
||
| @OptIn(DelicateCoroutinesApi::class) |
There was a problem hiding this comment.
9 : " UserProfile Repository ()" instance is created directly in the ViewModel . Should use dependency injection for better test ability and maintain ability .
| GlobalScope.launch { | ||
| uiState.emit(UiState.Loading) | ||
| try { | ||
| val userProfile = retrofitService.service.getUserProfile().userProfile |
There was a problem hiding this comment.
30 - 34 : Pot entially leaking the context if it 's an Activity context . Context should be used carefully and not held longer than necessary to prevent memory leaks . In this case , context might be held longer than necessary through the label Provider .
| } catch (e: Exception) { | ||
| uiState.emit(UiState.Error) | ||
| } | ||
| } |
There was a problem hiding this comment.
35 : Global Scope used . In case the ViewModel is destroyed before this coroutine finishes , it can lead to memory leaks as it will keep the ViewModel instance alive until the coroutine completed .
|
|
||
| sealed class UiState { | ||
| data object Loading : UiState() | ||
|
|
There was a problem hiding this comment.
40 : Any error in the network call will only emit " Ui State .Error ", no debugging information or differentiation between types of errors .
| } | ||
|
|
||
| companion object { | ||
| lateinit var labelProvider: (UserProfile) -> String |
There was a problem hiding this comment.
47 : label Provider is defined in a companion object , which is essentially a static field that holds an instance to a function that uses context . If the context is an Activity , this can lead to serious memory leaks .
| companion object { | ||
| lateinit var labelProvider: (UserProfile) -> String | ||
| } | ||
| } |
There was a problem hiding this comment.
49 : No safe call or null check before using label Provider which will throw a null exception if it is not initialized before calling load State ().
No description provided.