Improving code quality - Session 36: "Tautology is tautology" is a tautology

The original article was published on August 1, 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.

"Tautology is tautology" is a tautology

Let's assume you are implementing a certain groupware. This groupware has a task management feature and a messaging feature for users to communicate with each other. Additionally, when there is an update to a task or a new message is received, there is a feature to notify with a notification dot (notificationDot). The implementation of this notification dot is as follows. However, let's assume there are no issues with this logic and data model, including the calculation cost of any.

        notificationDot.isVisible = channelModels.any(ChannelModel::hasUnreadMessage) ||
            directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage) ||
            taskListStatus.hasNewlyAssignedTask ||
            taskListStatus.hasNewlyReopenedTask ||
            taskListStatus.hasUpdateOnAssignedTask

        ...

The calculation logic for isVisible is a bit long and not very clear. Therefore, this calculation logic was extracted into a private function shouldShowNotificationDot as follows.

        notificationDot.isVisible = shouldShowNotificationDot(
            channelModels,
            directMessageRoomModels,
            taskListStatus
        )

        ...
    }

    private fun shouldShowNotificationDot(
        channelModels: List<ChannelModel>,
        directMessageRoomModels: List<DirectMessageRoomModel>,
        taskListStatus: TaskListStatus
    ): Boolean = channelModels.any(ChannelModel::hasUnreadMessage) ||
        directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage) ||
        taskListStatus.hasNewlyAssignedTask ||
        taskListStatus.hasNewlyReopenedTask ||
        taskListStatus.hasUpdateOnAssignedTask

By doing this, the caller was simplified. However, it is hard to say that this is the best in terms of readability. Is there a better way?

Make it a meaningful phrase

Reading the line notificationDot.isVisible = shouldShowNotificationDot( as natural language, it becomes "display the notificationDot when it should be displayed". This provides limited information to the reader of the code.

If you want to know what this code does, you can stop reading at the point of =. Just by seeing notificationDot.isVisible =, you can understand that "this code updates the display state of notificationDot". Conversely, if you want to read the right side of = (and continuation lines), it means you want to know "under what conditions the notificationDot is displayed". However, even if you read the right side of = and the continuation lines, you only understand that "the display state of notificationDot depends on channelModels and others". To understand the actual conditions, you need to read the definition of shouldShowNotificationDot. However, this means that the right side of = and the continuation lines do not hold much information.

To make the code more informative (increase the information content of the code), it is a good option to give a slightly more detailed name to the extracted code. In the following code, even if you stop reading at notificationDot.isVisible =, you can know "what it does". On the other hand, if you need to understand the summary of the conditions, reading after ...isVisible = is sufficient, and you can understand that "there are unread messages or unread tasks".

        notificationDot.isVisible =
            hasAnyUnreadMessage(channelModels, directMessageRoomModels) ||
                hasAnyUnreadTask(taskListStatus)
    }

    private fun hasAnyUnreadMessage(
        channelModels: List<ChannelModel>,
        directMessageRoomModels: List<DirectMessageRoomModel>,
    ) = channelModels.any(ChannelModel::hasUnreadMessage) ||
            directMessageRoomModels.any(DirectMessageRoomModel::hasUnreadMessage)

    private fun hasAnyUnreadTask(taskListStatus: TaskListStatus): Boolean =
        taskListStatus.hasNewlyAssignedTask ||
            taskListStatus.hasNewlyReopenedTask ||
            taskListStatus.hasUpdateOnAssignedTask

In this example, the right-hand side performs || operations, but such calculations or operations are not always necessary. Even if it were a simple variable assignment, the same idea can be applied. Compare the following two assignments.

// having little information
notificationDot.isVisible = shouldShowDot

// having more information
notificationDot.isVisible = hasUnreadMessage

Of course, this idea can be applied to various cases, not just assignments. The following code is an example applied to conditional branching.

// having little information
if (shouldShowDot) {
    notificationDot.show()
}

// having more information
if (hasUnreadMessage) {
    notificationDot.show()
}

However, this idea of "giving a slightly more detailed name to the extracted code" is not something that should always be applied. When creating APIs or libraries, or when modularizing or layering code, where the details of dependencies should be hidden, names may be given based on "what the dependent should do". Similarly, in cases where the behavior is not determined, such as event handling interfaces for generic UI elements (e.g., onClicked), the code inevitably becomes "call onClicked when a click occurs". Keep in mind that giving a slightly more detailed name is just one of the options.

In a nutshell

When extracting code, consider giving it a name that increases the information content.

Keywords: naming, abstraction, extraction

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality