Convert RetrofitCallback to Kotlin and some refactoring #11
Conversation
| protected abstract fun observableLiveData(): MutableLiveData<T?> | ||
| override fun onResponse(call: Call<T?>, response: Response<T?>) { | ||
| if (response.isSuccessful) { | ||
| if (responseCallback != null) responseCallback!!.onSuccess(response.body()) |
There was a problem hiding this comment.
can use ?.let operator instead of null check or !!
| private const val DEFAULT_ERROR_CODE = -1 | ||
| private const val ERROR_CODE_UNAUTHORIZED = 1017 | ||
|
|
||
| abstract class RetrofitCallback<T> : Callback<T?> { |
There was a problem hiding this comment.
not required to mark generic Optional. While casting, it will anyway become an optional.
There was a problem hiding this comment.
Generics enforced me to make it nullable. But I didn't put much effort to remove that nullable part. Pls remove it if not required.
| this.responseCallback = responseCallback | ||
| } | ||
|
|
||
| protected abstract fun observableLiveData(): MutableLiveData<T?> |
There was a problem hiding this comment.
can instead make this a part of constructor, since that is the contract anyway
There was a problem hiding this comment.
No, there are cases where I don't want a callback/progress bar. e.g. Search. It was kept optional for that only.
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| var errorCode = DEFAULT_ERROR_CODE | ||
| if (response.code() in arrayOf(HTTP_FORBIDDEN, HTTP_UNAUTHORIZED)) { |
There was a problem hiding this comment.
Good way of checking :) +1
| it.setResult(Activity.RESULT_OK) | ||
| it.finish() | ||
| }) | ||
| viewModel.userVerificationResponse.observe( |
There was a problem hiding this comment.
should be outside this observer. Nesting observers leads to reregistering of observers everytime the parent observer observes an emission
| is Success -> { | ||
| activity?.let { | ||
| it.setPinCreated(true) | ||
| activity?.let { fragmentActivity -> |
There was a problem hiding this comment.
instead just do activity?.setPinCreated() or activity?.setResult, activity?.setFinish(). let creates a scoped function, so might not be useful considering the observer below has to be moved out.
There was a problem hiding this comment.
I have just added existing logic inside .let everywhere. If the said change is required, it was required before also.
| } | ||
| } | ||
| }) | ||
| if (context?.getConsentPinCreationAPIintegrationStatus()!!){ | ||
| if (context?.getConsentPinCreationAPIintegrationStatus()!!) { |
There was a problem hiding this comment.
can write this as if(context?.getConsentPinCreationAPIintegrationStatus() == true){...} to remove the force unwrapping
There was a problem hiding this comment.
== true why? It will anyway go inside if it's true. Instead we can extract out the variable outside so it is unwrapped only once. But it's just one occurrence.
There was a problem hiding this comment.
if you have a boolean that is nullable and you need to check it, rather than !! force unwrapping or using the let block, you can evaluate that against a true / false value.
context?.getConsentPinCreationAPIintegrationStatus() gives a Boolean? because context is nullable here. if you don't want to force unwrap, you could write this as
context?.getConsentPinCreationAPIintegrationStatus()?.let { if(it){...}}
or
if(context?.getConsentPinCreationAPIintegrationStatus() == true){...}
This doesn't always evaluate true because
if the value is null, null != true.
if the value is false, false == true
if the value is true, true == true
| @@ -102,7 +102,7 @@ class PatientAccountsFragment : BaseFragment(), ItemClickCallback, PatientAccoun | |||
| viewModel.linkAccountsResponse.observe(this, linkAccountsObserver) | |||
| } | |||
|
|
|||
| private val linkAccountsObserver = Observer<LinkAccountsResponse> { _ -> | |||
| private val linkAccountsObserver = Observer<LinkAccountsResponse?> { _ -> | |||
There was a problem hiding this comment.
try to remove optionals in generics.
| var linkedAccountsResponse = liveDataOf<LinkedAccountsResponse>() | ||
| var createAccountResponse = liveDataOf<CreateAccountResponse>() | ||
| var myProfileResponse = liveDataOf<MyProfile>() | ||
| var linkedAccountsResponse = liveDataOf<LinkedAccountsResponse?>() |
| @@ -18,9 +18,9 @@ class RegistrationViewModel(private val repository: AuthenticationRepository) : | |||
| const val MOBILE_IDENTIFIER_TYPE = "mobile" | |||
| } | |||
|
|
|||
| var requestVerificationResponse = liveDataOf<RequestVerificationResponse>() | |||
| var requestVerificationResponse = liveDataOf<RequestVerificationResponse?>() | |||
I have ran unit and UI tests. Since UI tests are running fine there shouldn't be any problems with this (unless UI tests were missing).
I would still request you to do a round of sanity after merging this PR.