Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove lateinit usage in Main.kt #156

Merged
merged 2 commits into from
Jul 6, 2017

Conversation

Mauin
Copy link
Collaborator

@Mauin Mauin commented Jul 5, 2017

Resolves #132

This follows the recommendation from the issue. But overall I'm not sure if its nicer to have 3 !! in the code compared to the lateinit var.

Probably also the null check in validateCli is not really useful as the project parameter is already required = true and thus JCommander will already throw an exception if the parameter is not specified.

@arturbosch
Copy link
Member

Yeah that is not pretty at all :), my other thought was to introduce a backing field so we do not need the !! everywhere?

@Mauin
Copy link
Collaborator Author

Mauin commented Jul 5, 2017 via email

@Mauin Mauin force-pushed the mr/remove_lateinit_in_main branch from e02c6f6 to 6751f5a Compare July 5, 2017 17:09
@@ -96,6 +99,9 @@ class Main {
private fun validateCli(cli: Main): List<String> {
val violations = ArrayList<String>()
with(cli) {
if (project == null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned this is not really necessary as JCommander already requires this parameter to be set. Let me know how you feel about this one also being here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then delete this lines and do a test run with the CLI and we are happy :)

@Mauin
Copy link
Collaborator Author

Mauin commented Jul 5, 2017

Updated the PR by using the backing field and making project private to prefer using the non-nullable field. Looks better for me.

@arturbosch arturbosch merged commit db9632f into detekt:master Jul 6, 2017
@MyDogTom
Copy link
Contributor

MyDogTom commented Jul 6, 2017

I'm just curious why lateinit is a bad practice? Especially when it's replaced with !!. For me this change means that UninitializedPropertyAccessException will be just replaced with NullPointerException.

@arturbosch
Copy link
Member

Because it misleads you not using a safer way to initialize a variable (constructor injection or delegation).

When I started programming in Kotlin I often used lateinit in spring-framework or just because I found using constructors in Kotlin difficult (always calling primary constructor in secondary constructors etc) and I think this is a common pitfall for newcomers.

Ofc in our case it seems kind of odd to replace one keyword with a backing field, but this due to the nature of many annotation heavy libraries (jcommander). Ofc if you know what you do lateinit is not that bad and in our solution we are also relying on jcommander to check the field, while the backing field looks unsafe ...

The tradeoff here is like Suppress annotation vs backing field :D

@arturbosch arturbosch modified the milestone: M13 Jul 9, 2017
mohammadkahelghi-grabtaxi pushed a commit to mohammadkahelghi-grabtaxi/detekt that referenced this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants