val b: Boolean? = ...
if (b == true) {
...
} else {
// `b` is false or null
}
We stumbled over this because IDEA told us to change
if (nullableCollection?.isEmpty() ?: true) { ... }
into
if (nullableCollection?.isEmpty() != false) { ... }
My main problem¹ with that is that
!(b == true),
b != true,
b == false, and
!(b != false)
are not all equivalent – but we expect them to be, because we expect to be dealing with boolean logic (which we aren’t) from what we see².
Therefore, I think that b ?: false is a lot clearer since it makes the non-boolean nature of b explicit and avoids confusion for readers who miss that b is nullable (it might have been declared far away!).
Thoughts? What is the rationale in defense of this idiom?
Also, <boolean expression> == <boolean constant> is redundant. (In fact, we used to make it a point to train our students not to write expressions like that! Of course, we don’t have booleans here, but it looks as if we did.
Readers used to Java’s type system might stumble even more because b == true wouldn’t even type check there: can’t compare null to a values of a primitve type.
is the clearest option because it presents the logic in the “correct” order. But that would be discussing the Elvis operator as a design/usage choice, which is not what I want to talk about here.
I personally dislike the someBoolean == true syntax. The problem I have with it is that I see it is just so infrequent that I need to spend a lot of time figuring out what will happen in case of null. Therefor I normally try to use the elvis operator instead and just disable the intention in idea.
That being said. If I have to deal with nullable collections I just create a simple extension:
fun <T>Collection<T>?.isNullOrEmpty() = this == null || this.isEmpty()
fun <T>Collection<T>?.isNotNullOrEmpty() = this != null && this.isNotEmpty()
Good thought about the extension – that would lead to the clearest code!
Nitpick: does isNotNullOrEmpty mean isNot(NullOrEmpty) or is(NotNull)OrEmpty? (It’s clear from the implementation, but from reading alone?)
Maybe isNonNullAndNonEmpty would be better – but man, what a mouthful! Or isNeitherNullNorEmpty? I think I’d go with !col.isNullOrEmpty().
If you think a little (even without looking on the implementation) it’s obviously the first, because the second contains redundant ‘or’: is(NotNull)OrEmpty is the same as just is(NotNull) and thus doesn’t make sense to be provided in that meaning.
Sure. The point is that I don’t want to be thinking for basic stuff like that. That’s just wasted mental resources.
Good syntax and naming enables me, after some training, to read code, similar to natural language or sheet music. As soon as I have to stop and think about what the method does (or what the word means, how I play that thing on my instrument) I have, kind of, lost.
My understanding is that syntax and idioms are there to facilitate reading in that sense. Hence my titular question: is this particular idiom universally useful in the reading comprehension sense?
(My argument being less that it is hard to understand, but that it’s easy to misunderstand.)
I don’t totally agree here. Although I might concede that the b == true syntax is the best possible solution to this problem I still don’t think it is a good solution. IMO this get’s used rarely if at all. In my last year of writing Kotlin code I had to use this a handful of times at most. Because of that, this syntax is confusing for me still. I understand it but every time I see it I have to stop and think about how it works.
I personally prefer b ?: false or best case not to use nullable booleans at all.
I have (I think). As I said I only used this syntax like 4 times in the past year. As far as I know I only implemented the 2 utility functions I posted earlier.
Yea, um. That’s the point. The case you forget because you think you’re dealing with booleans.
The intuition we apply is wrong. Of course. But for readability, being “correct” is not the only important thing. If it was, we might as well all write assembly.
Sure, that’s a possibility.
The question I’m asking is if whether the idiom – as an officially recommended way to code! – is helpful, that is whether it makes for clearer code. If not, that inspection should be there at all.
So far you have said nothing beyond stating the facts of Kotlin’s semantics, i.e. nothing to convince me that the idiom is useful.
I also don’t find that idiom useful. I prefer writing b?:false. Yes, I’m disabling the warning in Intellij. I find it valid to ask about it’s usefulness.
This idiom and the elvis operator are the primary alternatives. So it is useful in that it solves that problem.
The question is which is better than the other. I don’t find either better than the other so much that I would say everyone should use one or the other, but I probably lean on the side of the idiom because it uses only concepts familiar to programmers in any language, while the Elvis operator is slightly less familiar.
As I said if you disagree with the idiom you can turn that inspection off
Yes the elvis operator is unfamiliar to people new to kotlin but if you compare the language to java it is still the better choice IMO. nullBoolean == true leads to a NPE in java, therefor this syntax is even more confusing in Kotlin, because you have to learn another difference between the language.
The elvis operator on the other hand is such a basic concept (in kotlin) that you learn in very fast and therefor there are no problems understanding it.
This is a corner case after all so I think the syntax which is self explanatory is the better choice.
I was thinking about the impact of this idiom on bigger expressions, specifically whether associativity, distributivity, De Morgan’s laws, etc. still hold – that is, stuff we may apply to “simplify” expressions but in fact changing the meaning if null is involved.
However, I realized that larger expressions become a mess: using b == true or b ?: false instead of b will be next to unreadable either way.
Therefore, I think I tend towards resolving nullability up front:
val a = someBooleanOrNull(stuff) ?: true
val b = otherBooleanOrNull(things) ?: false
val c = justBoolean(material)
if (!(a || b) && c) { ... }
Compare with:
val a = someBooleanOrNull(stuff)
val b = otherBooleanOrNull(things)
val c = justBoolean(material)
// Idiom:
if (!((a != false) || (b == true)) && c) { ... }
// Elvis:
if (!((a ?: true) || (b ?: false)) && c) { ... }
While I’d still prefer the Elvis operator – note how we see the default, not its opposite! – both are not very readable.
This is all assuming that getting null into such expressions is a good idea. My gut feeling is that getting a null “control-flow boolean” (as opposed to data) would usually signal switching to a different code path, such as throwing an error. Then, smart-casting gives us what we want without any syntactic overhead:
if (a == null || b == null) {
throw ...
}
if (!(a || b) && c) { ... }
I think the question here should be whether it is useful to show a warning if the other viable solution is used.
I probably lean on the side of the idiom because it uses only concepts familiar to programmers in any language, while the Elvis operator is slightly less familiar.
That could as well be an argument for Elvis operator. I mean the warnings shown in Intellij are the perfect place to learn language idioms.
Besides, the Elvis operator is probably already known by the developer. Because the most likely reason for showing the warning was that the Elvis operator was used.
First, I found this thread because I was looking for confirmation of why IntelliJ IDEA suggested this change. I don’t think I found what I was looking for, but I suspect (though I can’t yet give a good example) that it may have something to do with the potential for programming errors related to the order of operations (== is lower precedence than ?:).
Also, I know this is kinda off topic, but for this particular case, this might be a good workaround:
if (nullableCollection.orEmpty().isEmpty()) { ... }
I think I’d go with 