(Hopefully) some improvements to Kotlin code and some general cleanup #7
Conversation
… are not needed. As far as I can see, this object is intended for internal use only, so it would only be used in Kotlin, and most of the fields can be declared as const values instead.
- Removed explicit android.support.annotation.NonNull annotation, as non-nullable Kotlin variables and parameters are annotated with org.jetbrains.annotations.NotNull, which should serve the same purpose.
- Moved constants into private companion object, so they would become actual static constants.
- Moved some stuff out of init{} block directly into property declarations. This removes the need for some fields being nullable, where they don't need to be and still behaves as if being set in the init{} block.
- Removed hungarian notation for private properties. Left it in for mTimeOutLeft and mTriesCountLeft for now, as they could potentially be intended to be part of the exposed API.
- Removed the need for the !! operator.
- Fixed spelling error with setPositive and deprecated setPostisive methods.
- Moved some stuff out of the Builder's init{} block directly into property declarations.
- Removed hungarian notation for private properties.
- Removed the need for the !! operator by making the context property not-nullable, lazily initializing the dialog, and not handling fallbacks for string properties directly in show() but directly where they are initialized.
- Removed hungarian notation. - Removed the need for the !! operator by using the elvis operator. - Made broadcastIntent property not-nullable by lazily initializing.
…ty for Java and Kotlin) - Removed hungarian notation. - Changed most private properties with extra getter functions into public ones with custom getters. - Changed isSdkVersionOk from function to lazily initialized property. - Changed FahManager reference to be immutable and initialize it immediately with either a correct reference or null. This removed the need for the !! operator or even ?. with proper null checks and implicit casts. - Added JvmName annotations to properties, where the implicit getter name didn't match the removed getter function. - Deprecated explicit getter functions, which should not be used anymore and set bogus JvmName annotations to avoid naming clashes with Java names.
… still using the old cleanTimeOut method, that method is now fully deprecated and implicit isTimeOutCleaned() method should be used instead.
…the getter instead of field.
…property this time, as that one wasn't needed.
…recated FahErrorType.Auth.AUTH_TO_MANY_RETRIES.
| @@ -280,16 +263,20 @@ internal class FahManager(@NonNull c: Context, l: FahListener?, keyName: String, | |||
| } | |||
| } | |||
|
|
|||
| val keyStore: KeyStore | |||
There was a problem hiding this comment.
can you please explain, what is the goal to create local variable instead reinit the property one?
There was a problem hiding this comment.
This was done to avoid unnecessary null checks. As the keyStor property is mutable (var instead of val) it could possibly change from being not-null to null. Agreed, this shouldn't really happen, but the compiler things it might, so this is just to avoid warnings, essentially.
There was a problem hiding this comment.
thanks for explanation, it little bit more clear right now for me :)
There was a problem hiding this comment.
Great! :)
I hope some day, I'll find a better way to do such things, but I'm kinda still learning Kotlin. ;)
| @@ -303,11 +290,11 @@ internal class FahManager(@NonNull c: Context, l: FahListener?, keyName: String, | |||
| } | |||
|
|
|||
| try { | |||
| mKeyStore!!.load(null) | |||
| keyStore.load(null) | |||
There was a problem hiding this comment.
and here you are "loading" to null local variable instead property variable. or I am missing the idea of this?
There was a problem hiding this comment.
As the local variable and the property are referencing the same object, this would also affect the property.
| mCancellationSignal = null | ||
| mIsListening = false | ||
| isActivityForeground = false | ||
| if (cancellationSignal != null) { |
There was a problem hiding this comment.
if (cancellationSignal != null) {
this one also can be changed for cancellationSignal?.le{
There was a problem hiding this comment.
Yes, you're completely right! Will change that, when I get to my station!
There was a problem hiding this comment.
no needed, i am already pull your request to the separate branch and add this improvements ;-)
|
this is little bit different theme, but maybe you have some ideas about this issue? |
|
thanks for your job :) |
|
No problem at all. This was fun! :) |
I'm currently adapting Kotlin myself and in our current project we needed Fingerprint authentication, so I've stumbled upon your library and noticed some things, that weren't Kotlin enough for me. ;)
Everything is up for discussion, of course.
You should get all the information from my commit messages, but overall what I did is:
ReplaceWithparameters for easy Alt+Enter replacements.FahSecureSettingsDialog.setPositive()methods, but keptsetPostisive()methods around with added deprecation annotations. Same goes forFahErrorType.Auth.AUTH_TOO_MANY_TRIES.Let me know what you think!