`return` from within `when` looks funny

Language Design
1

The following doesn’t look right to me somehow. I think return null means to return from foo, not from when, but my gut doesn’t trust it:

fun foo(owner: User, startEvent: Event?, endEvent: Event?): Message {
    val m = Message(owner,
                    when {
                        endEvent != null   -> MsgType.END_DATE
                        startEvent != null -> MsgType.START_DATE
                        // RETURN FROM foo()? OR FROM when?
                        else               -> return null
                    })
    m.emailWorthy = true
    save(m)
    // This looks totally normal
    return m
}

The when statement really looks like a list of functions that take a Boolean and return a MsgType. Thinking code-as-data, I think that’s how the compiler sees it, so that’s a good thing. I’d say that I don’t like mixing data flow with evaluation, but with a throw, the code-as-data model works for understanding the when statement. It’s only when I see return there, it makes me uneasy somehow.

I feel that when reading the code, the return is hidden in a place my eye doesn’t want to look for it. Thoughts?

If this is a “bad” code smell, how should I rewrite the above?

2

It is valid code, but definitely not the best from the point of readability. I would not use when inside function argument anyway. Something like this should help:

    val messageType =  when {
                        endEvent != null   -> MsgType.END_DATE
                        startEvent != null -> MsgType.START_DATE
                        else -> return null
    }
    val m = Message(owner, messageType)

It could be further expanded by making messageType nullable and returning null from when, but I think it is OK as is.

1 Like
3

To me, the smelly part is the overall function - it creates a Message, except when it doesn’t, and it has two optional parameters, where I guess at least one should be present, but probably not both, because the first is ignored in that case.

It should just be

fun foo(owner: User, msgType: MsgType): Message

or

fun foo(owner: User, msgType: MsgType, event: Event): Message

I bet you’d find it easier to name, too… :slight_smile:

4 Likes
4

the else branch should be just “null” without the return

5

No, it is not. It would cause type violation. Try it.

6

I miss-interpreted the intent of it,
I though the intent of the when was:

val messageType: MsgType? =  when {
                        endEvent != null   -> MsgType.END_DATE
                        startEvent != null -> MsgType.START_DATE
                        else -> null
    }

but
Message() expects MsgType and not MsgType?

What I would do to make it more readable is:

    fun foo(owner: User, startEvent: Event?, endEvent: Event?): Message? =
        when {
            endEvent != null -> MsgType.END_DATE
            startEvent != null -> MsgType.START_DATE
            else -> null
        }?.let { msgType ->
            Message(owner, msgType).also { m ->
                m.emailWorthy = true
                save(m)
            }
        }

or if you rather have the explicit return:

    fun foo2(owner: User, startEvent: Event?, endEvent: Event?): Message? {
        val maybeMsgType = when {
            endEvent != null -> MsgType.END_DATE
            startEvent != null -> MsgType.START_DATE
            else -> null
        }
        return maybeMsgType?.let { msgType ->
            Message(owner, msgType).also { m ->
                m.emailWorthy = true
                save(m)
            }
        }
    }

I think that a when should be use as an expression OR a flow control directive, to mix both is just messy :slight_smile: my code above uses it as an expression.

I have to agree with @ mslenc that the main problem here is to have one function that receive 2 optional params and both of the same type, it would be too easy to code a bug by calling foo(u, null; event) instead of foo(u, event, null) also… if you call foo(u, event1, event2) what do you expect to happen??

You should not need to know about the implementation to properly use the interface, base on that principle, and taking into account that you may want to hide the MsgType from the caller, you may want to have:
fun sendStartEvent() and fun sendEvent() or something equivalent

2 Likes