Improving code quality - Session 30: The (transparent) red thread of fate

The original article was published on June 20, 2024.

Hello, I'm Munetoshi Ishikawa, a mobile client developer for the LINE messaging app.

This article is the latest installment of our weekly series "Improving code quality". For more information about the Weekly Report, please see the first article.

The (transparent) red thread of fate

Let's say we are implementing a feature to display "messages" sent by other users. The following MessageModel and MessageContent are data models representing such "messages". These data models are used in layers other than the UI, so it is inappropriate to change the definition just for the UI.

class MessageModel(
    val content: MessageContent,
    val timestampMillis: Long,
    val senderUserId: UserId,
    ...
)

sealed class MessageContent {
    data object Empty: MessageContent()

    data class Error(val errorType: ErrorType): MessageContent()

    data class Text(val text: String): MessageContent()

    data class Event(val eventType: EventType): MessageContent()
}

The UI layout for displaying messages is implemented as follows. (Note that error is a standard Kotlin function that throws a runtime error.)

fun bindToViews(message: MessageModel) {
    val isVisible = isContentValid(message.messageContent)
    layoutContainer.isVisible = isVisible
    if (!isVisible) {
        return
    }

    timestampTextView.text = formatter.format(message.timestampMillis)
    ... // Other view update logic
    contentTextView.text = getMessageText(message.content)
}

private fun isContentValid(content: MessageContent): Boolean = when (content) {
    MessageContent.Empty,
    is MessageContent.Error -> false
    is MessageContent.TextMessage,
    is MessageContent.EventMessage -> true
}

private fun getMessageText(content: MessageContent): String = when (content) {
    MessageContent.Empty,
    is MessageContent.Error -> error("Invalid message type as `messageText`")
    is MessageContent.TextMessage -> content.text
    is MessageContent.EventMessage -> content.eventType.toMessageString()
}

Is there any problem with this code?

In a nutshell

In the above code, the implementation of getMessageText is implicitly related to isContentValid, resulting in the following issues:

• It assumes that getMessageText can only be called when isContentValid returns true, but if violated, it can only be noticed by a runtime error.
• When types are added or updated, it is necessary to check whether the behavior between isContentValid and getMessageText is consistent.

Just by looking at the implementation of bindToViews, you may not notice the relationship between isContentValid and getMessageText. If you overlook this relationship, it is difficult to notice when you misuse getMessageText during refactoring or feature expansion.

When there are implicitly related functions, it can sometimes be resolved by combining the functions into one. In the current code, isContentValid is implemented as a filter, and getMessageText as a conversion, but the underlying logic is common. Therefore, let's integrate these two roles into getMessageText.

/**
 * Returns the displayable text representation of the message content [content].
 * However, if the content type is not displayable (error/empty), it returns null.
 */
private fun getMessageText(content: MessageContent): String? = when (content) {
    MessageContent.Empty,
    is MessageContent.Error -> null
    is MessageContent.TextMessage -> content.text
    is MessageContent.EventMessage -> content.eventType.toMessageString()
}

The caller will be as follows:

fun bindToViews(message: MessageModel) {
    val messageText = getMessageText(message.messageContent)
    layoutContainer.isVisible = messageText != null
    if (messageText == null) {
        return
    }

    timestampTextView.text = formatter.format(message.timestampMillis)
    ... // Other view update logic
    contentTextView.text = messageText
}

This time, we used null to indicate an invalid content type, but this specification may cause confusion if getMessageText becomes widely used. In such cases, it is good to redefine isContentValid. However, at that time, the implementation should make the relationship between isContentValid and getMessageText explicit. For example, isContentValid can be implemented using getMessageText instead of as an independent function.

private fun isContentValid(content: MessageContent): Boolean = 
    getMessageText(content) != null

/** ... */
private fun getMessageText(content: MessageContent): String? = when (content) {
    MessageContent.Empty,
    is MessageContent.Error -> null
    is MessageContent.TextMessage -> content.text
    is MessageContent.EventMessage -> content.eventType.toMessageString()
}

By doing so, it becomes a safe implementation to use getMessageText alone, and it ensures consistency between isContentValid and getMessageText.

In a nutshell

When there is an implicit relationship between functions, combine the functions into one or change to an implementation where the relationship is clear.

Keywords: conditional branch, function precondition, implicit dependency

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality