This can be an opportunity for the discussion if we want to gradually migrate ThirtyInch to Kotlin.
This PR is a low-hanging fruit which lets us have thirtyinch-logginginterceptor as the first fully Kotlin module (apart from thirtyinch-kotlin).
As arguments for converting ThirtyInch to Kotlin are (as general for such conversions):
much less code and better readable
much less bugs (especially null related)
faster and easier bugfixing/adding new features in the future
last but not least, much more fun and easier working on the code :)
Against converting:
we have to ship Kotlin stdlib and runtime with ThirtyInch even for Java consumers who don't need them otherwise
it's quite a lot of work overall (but we can go class by class, module by module)
we might miss some Java specifics which in Kotlin don't matter and have to be explicitly annotated (method overloads, companion object/util classes JVM names etc.)
there are subtle differences in behavior regarding different language APIs which have to be taken into account, e.g. how Kotlin and Java treat varargs, sometimes it's hard to determine if a parameter passed from Java may or may not be nullable etc.
Regarding this PR:
Obviously I have left the Java tests for the conversion to make sure nothing got broken when refactoring. I have even added a few more tests with more obscure "array/list containing nulls" cases which previously were not tested.
I was able to use joinToString() in place of looped StringBuilders for logging method parameters and lists/arrays, which made the code much shorter and more readable. Also using string templates improved readability.
Also thanks to clearer nullability declarations I was able to get rid of some unnecessary null checks.
There were also small inefficiencies in the original class, like reassigning StringBuilder instance in parseParams() when trimming, which could be improved in Kotlin.
The only API this class exposes to consumers are its constructors. The @JvmOverloads annotation assures that all constructors are available to Java consumers as they were before, with exactly the same functionality. No-arg constructor defaults to TiLog.TI_LOG, one-arg constructor with a null parameter defaults to TiLog.NOOP. Because of integrating all Java constructors into one Kotlin constructor in the class definition, also all the constructor JavaDoc comments had to be integrated into the class comment. Please also note that a null logger as one-arg constructor parameter was not documented in the Java code previously, so I have left it this way as well (only explicit passing of TiLog.NOOP as constructor parameter was documented).
Also the handleInvocation() method, which is indirectly exposed internally, is annotated with @Throws for calls from Java. Its arguments/return type nullability is marked appropriately as per usage analysis in library Java code.
Private static methods toString(Method, Array<Any?>) and parseParams(...) could obviously be moved out as file-level functions.
Overall the class file got shorter from 192 lines to 128 lines.
Please test it also with the sample app to make sure logging works correctly in production.
This PR is independent of #179 and can be merged as a stand-alone PR.
This can be an opportunity for the discussion if we want to gradually migrate ThirtyInch to Kotlin.
This PR is a low-hanging fruit which lets us have
thirtyinch-logginginterceptoras the first fully Kotlin module (apart fromthirtyinch-kotlin).As arguments for converting ThirtyInch to Kotlin are (as general for such conversions):
Against converting:
Regarding this PR: Obviously I have left the Java tests for the conversion to make sure nothing got broken when refactoring. I have even added a few more tests with more obscure "array/list containing nulls" cases which previously were not tested. I was able to use
joinToString()in place of loopedStringBuilders for logging method parameters and lists/arrays, which made the code much shorter and more readable. Also using string templates improved readability. Also thanks to clearer nullability declarations I was able to get rid of some unnecessary null checks. There were also small inefficiencies in the original class, like reassigningStringBuilderinstance inparseParams()when trimming, which could be improved in Kotlin.The only API this class exposes to consumers are its constructors. The
@JvmOverloadsannotation assures that all constructors are available to Java consumers as they were before, with exactly the same functionality. No-arg constructor defaults toTiLog.TI_LOG, one-arg constructor with anullparameter defaults toTiLog.NOOP. Because of integrating all Java constructors into one Kotlin constructor in the class definition, also all the constructor JavaDoc comments had to be integrated into the class comment. Please also note that anulllogger as one-arg constructor parameter was not documented in the Java code previously, so I have left it this way as well (only explicit passing ofTiLog.NOOPas constructor parameter was documented).Also the
handleInvocation()method, which is indirectly exposed internally, is annotated with@Throwsfor calls from Java. Its arguments/return type nullability is marked appropriately as per usage analysis in library Java code.Private static methods
toString(Method, Array<Any?>)andparseParams(...)could obviously be moved out as file-level functions.Overall the class file got shorter from 192 lines to 128 lines.
Please test it also with the sample app to make sure logging works correctly in production.
This PR is independent of #179 and can be merged as a stand-alone PR.