Thank you for the discussion, folks.
I did a bunch of tests on this and will be using @kyay10’s ensure inline function:
inline fun ensure(condition: Boolean, exit: () -> Nothing) {
contract {
returns() implies condition
}
if(!condition) exit()
}
Even though it puts { return null } everywhere in the code, I do believe lined ensured conditionals are easier to read than if statements, and it doesn’t seem to impact performance at all.
Maybe you could organize your code a little differently so you don’t need to do that. I dont think I’ve ever been in that situation in Kotlin.
I agree with @brill , compare OP’s original:
fun visit(): MyObj? {
return if (
condition1 &&
condition2 &&
condition3 &&
....
) {
MyObj(...)
} else {
null
}
}
with
fun visit(): MyObj? {
return if (doCreate()) {
MyObj(...)
} else {
null
}
}
fun doCreate() =
condition1 &&
condition2 &&
condition3 &&
Thanks to @brill and @mbeimcik, but our little tests with our team disagree with you. Every time we tested stacked if conditions against ensure-like functions, the if-based code lost. Both when asking team members to tell which code was easier to read and when checking with students how well they understood the code seconds after being presented with it. The ensure is about 3x faster than the if.
The addition of the doCreate-like check functions worsened our tests. It just created another layer of redirection before the dev fully understood the code, which made him/her slower in doing so.
I hope somebody else out there can run similar tests to see if our results are just us or if they match.
I believe this happened for two reasons:
To be very clear, everyone in the team found
fun visit(): MyObj? {
ensure(condition1) { return null }
ensure(condition2) { return null }
ensure(condition3) { return null }
return MyObj(...)
}
to be far superior than
fun visit(): MyObj? {
return if (doCreate()) {
MyObj(...)
} else {
null
}
}
fun doCreate() =
condition1 &&
condition2 &&
condition3 &&
I agree @vitorpamplona . It does read better assuming the conditions are well named and not written on the spot.
fun visit(): MyObj? {
ensure(userIsLoggedIn) { return null }
ensure(userHasPermission) { return null }
ensure(accountIsActive) { return null }
ensure(subscriptionIsValid) { return null }
ensure(profileIsComplete) { return null }
ensure(emailIsVerified) { return null }
ensure(phoneIsVerified) { return null }
ensure(locationIsAllowed) { return null }
ensure(deviceIsTrusted) { return null }
ensure(noOutstandingPayments) { return null }
return MyObj(...)
}
I just wish we didn’t have to spell out the { return null } in each line.
And, for some reason, this code here didn’t do well.
fun visit(): MyObj? {
if (condition1) else return null
if (condition2) else return null
if (condition3) else return null
return MyObj(...)
}
I think the presence of the else makes devs wonder if one of these lines is misindented enough to break the conditions. Which leads to more time reviewing the code.
Hopefully one day we get a language with an extra jump expression on top of break, continue and return. ensure is a bit “pompy”. Maybe require is a good keyword?
fun visit() {
require (condition1)
require (condition2)
require (condition3)
...
doSomething()
}
it’s one of the reasons, I sometimes miss a good “macro” feature.
Also, if everything would be a function (no statements), everything would become easier.
For your problem, why not combine both worlds.
The main advantage of “ensure” seems to be its descriptive nature.
Ad hoc I would write it like this:
class EnsureResult(val cond: Boolean) {
operator fun plus(cond2: EnsureResult) = EnsureResult(cond && cond2.cond)
operator fun <T> plus(result: () -> T) : T? = if (cond) result() else null
}
fun ensure(test: () -> Boolean) : EnsureResult = EnsureResult(test())
data class MyObj(val text: String)
fun visit(x: String): MyObj? =
ensure { x.contains("a") } +
ensure { x.contains("b") } +
ensure { x.contains("c") } +
{ MyObj("has a,b,c") }
fun main() {
println( "abc: ${visit("abc")}" )
println( "abx: ${visit("abx")}" )
println( "xyz: ${visit("xyz")}" )
}
the last line could also be descriptive:
fun visit(x: String): MyObj? =
ensure { x.contains("a") } +
ensure { x.contains("b") } +
ensure { x.contains("c") } +
result { MyObj("has a,b,c") }
you might like other operators
Maybe one day we get a macro keyword that simply always inlines the function instead of the inline which the compiler is not required to inline.
This would solve the issues with return expression ambiguity (returning from which functions) of inline functions.
Maybe the compiler can even force a return@caller when jump expressions are used inside macro functions. Something like:
macro fun <return T> ensure(condition: Boolean) {
if (!condition) return@caller null
}
Then
fun visit(): MyObj? {
ensure(condition1)
ensure(condition2)
ensure(condition3)
return MyObj()
}
The compiler is actually required to inline those functions AFAIK, or at least it does inline it in pretty much all cases
In practice, yes. In theory, I think, by the spec, it is still marked “undefined behavior”.
We need a force inline to make sure the jump expressions work as inline.
Sorry for being late to the party, but I’ve got a couple of suggestions.
First suggestion: why not just use the existing require from the Kotlin stdlib?
fun visit(): MyObj? {
require(condition1) { return null }
require(condition2) { return null }
require(condition3) { return null }
return MyObj(...)
}
It’s the same as the already-mentioned ensure solution but require already exists so you don’t have to write a separate ensure method.
Second suggestion: in response to your reservations about this solution:
fun visit(): MyObj? {
if (condition1) else return null
if (condition2) else return null
if (condition3) else return null
return MyObj(...)
}
The ‘else’ immediately after ) seems to be a problem, so how about this:
val ok = Unit
fun visit(): MyObj? {
if (condition1) ok else return null
if (condition2) ok else return null
if (condition3) ok else return null
return MyObj(...)
}
(Instead of ok, use “fine”, “hunky_dory” or whatever you prefer.)
The issue with else’s in returns is that any missing null from there might start cascading the ifs. Any missing else will invert the clause. Any missing return will not exit.
It’s very easy to find/understand the pattern ok else return null when condition1, condition2, and condition3 have the same amount of chars. In real conditions of varying lengths, it becomes a brain exercise to find all the else’s, returns, and nulls.
Adding the OK just adds another thing that maybe or may not be there to confuse the reader.
This should be fixed by the Unused return value checker KEEP, in the sense that the compiler should show clear warnings if you just have a dangling Boolean like that that goes nowhere
Yes, but “trusting the compiler will protect you from bugs” is not a good mindset. Devs know to be skeptical of that and that skepticism decreases the readability of the code.