Mvvm/login #3
Mvvm/login #3
Conversation
| ): Response<BaseResponse<User>> | ||
|
|
||
| companion object { | ||
| operator fun invoke(networkConnectionInterceptor: NetworkConnectionInterceptor): LmsApi { |
There was a problem hiding this comment.
@iamsannyrai we have decided to create network module isn't it?
| import retrofit2.Response | ||
|
|
||
| class UserRepository(private val lmsApi: LmsApi) { | ||
| suspend fun userLogin(username: String, password: String): Response<BaseResponse<User>> { |
There was a problem hiding this comment.
@iamsannyrai create wrapper to unwrap data rather than doing Response<BaseResponse<User>> .
| authListener!!.onStarted() | ||
| when { | ||
| username.isNullOrEmpty() -> { | ||
| authListener?.onFailure(0, "Username is Empty") |
There was a problem hiding this comment.
@iamsannyrai what is zero and one. Don't use these kinds of ambiguous constants. Give a proper name for constants .
There was a problem hiding this comment.
It was code to be sent in order to distinguish whether username field or password field was empty. However i have tried to give proper name now.
|
|
||
| import com.incwelltechnology.lms.data.model.User | ||
|
|
||
| interface AuthListener { |
There was a problem hiding this comment.
@iamsannyrai do we need interface we have discussed to use LiveData to communicate with the view rather than interface.
| else -> { | ||
| //when credential fields are not empty or null | ||
| Coroutine.main { | ||
| try { |
There was a problem hiding this comment.
@iamsannyrai is this aware of the activity life cycle? Because it may crash app once we make a network request and pressed back to the previous activity instantly. Hint: a ViewModel Scope.
There was a problem hiding this comment.
No it doesn't crash and since it is in viewmodel itself, it must be aware of activity lifecycle.
| } | ||
| } catch (e: NoInternetException) { | ||
| withContext(Dispatchers.Main){ | ||
| authListener?.onFailure(2, e.message!!) |
There was a problem hiding this comment.
@iamsannyrai we don't need to withContext if we use liveData to communicate
| } | ||
| } | ||
|
|
||
| //function to translate layout from down to up |
There was a problem hiding this comment.
@iamsannyrai name function in such a way that we don't need to write comments for further explanation.
There was a problem hiding this comment.
@iamsannyrai proper name may be translateUp()
There was a problem hiding this comment.
@iamsannyrai we can use animation in theme as well to make consistent over the whole application. Think about that as well. Make animation consistent for push and pop of activity if you want to keep it in theme.
There was a problem hiding this comment.
this animation function is only for login page. Anyway i will think of your suggestion.
| override fun onCreate(savedInstanceState: Bundle?) { | ||
| super.onCreate(savedInstanceState) | ||
| setContentView(R.layout.activity_dashboard) | ||
| val toolbar: Toolbar = findViewById(R.id.toolbar) |
There was a problem hiding this comment.
@iamsannyrai do we need findViewById to reference element?
There was a problem hiding this comment.
@iamsannyrai do we need findViewById to reference element?
No we don't need to. This activity was just created and left untouched. I haven't worked upon this activity :)
| setSupportActionBar(toolbar) | ||
|
|
||
| val fab: FloatingActionButton = findViewById(R.id.fab) | ||
| fab.setOnClickListener { view -> |
There was a problem hiding this comment.
@iamsannyrai we are not doing anything there except a snack bar. Keep the codes only required to complete user stories otherwise, it will create confusion for a reviewer.
I have tried to apply koin. Please go through it once