Improving code quality - Session 34: Craft brings nothing home

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

Craft brings nothing home

Suppose you are implementing the UI layout for a service that sends and receives messages. Within this layout, you need to implement various "buttons" (for example, send message button, profile edit button, ...). The following code is the implementation of the send message button SendMessageButton and the profile edit button ProfileStatusEditToggleButton. These buttons call onClicked when clicked.

class SendMessageButton(
    private val messageTextForm: TextForm,
    private val useCase: MessageServiceUseCase
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val message = Message(messageTextForm.text, ...)
        val action = MessageServiceAction.SendMessage(message)
        useCase.executeServiceAction(action)

        messageTextForm.clear()
    }
}

class ProfileStatusEditToggleButton(
    private val statusTextForm: TextForm,
    private val useCase: MessageServiceUseCase
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val isEditable = !button.isChecked
        button.isChecked = isEditable 
        
        statusTextForm.isEditable = isEditable
        if (!isEditable) {
            val statusText = statusTextForm.text 
            val action = MessageServiceAction.UpdateProfileStatus(statusText)
            useCase.executeServiceAction(action)
        }
    }
}

The actual behavior when the button is clicked is specified by MessageServiceAction, and MessageServiceUseCase processes it based on that value. The class structure was designed to prevent each Button from directly manipulating the repository through the layers of MessageServiceUseCase and MessageServiceAction.

class MessageServiceUseCase(
    private val messageRepository: MessageRepository,
    private val profileRepository: ProfileRepository,
    ...
) {
    fun executeServiceAction(action: MessageServiceAction) {
        when (action) {
            is MessageServiceAction.SendMessage -> action(messageRepository)
            is MessageServiceAction.UpdateProfileStatus -> action(profileRepository)
            ...
        }
    }

}

sealed interface MessageServiceAction {
    class SendMessage(
        private val message: Message
    ) : MessageServiceAction {
        operator fun invoke(messageRepository: MessageRepository) {
            // ... actual logic
        }
    }

    class UpdateProfileStatus(
        private val statusText: String
    ) : MessageServiceAction {
        operator fun invoke(profileRepository: ProfileRepository) {
            // ... actual logic
        }
    }

    ...
}

However, there are several issues with this code. What are they?

Strategy not to craft

The major issues are as follows:

• Integration and re-branching of conditions: Each Button defines its behavior by specifying a MessageServiceAction. Although these actions are gathered into a single flow in MessageServiceUseCase.executeServiceAction, they are re-branched within it. This structure makes it difficult to follow the flow of actions. For example, to check under what conditions SendMessage.invoke is called, you need to read all the code that calls executeServiceAction.
• Unnecessary sealed class: While sealed class is a convenient language feature that guarantees exhaustiveness in downcasting, in this case, the conditions can be organized without using sealed class. In other languages, unnecessary associated values or sum types should also be avoided.
• Incomplete logic layer separation: Although MessageServiceAction is intended to hide the Repository from the Button, invoke can be freely called. This makes it difficult to notice incorrect usage.

These issues are caused by the inappropriate use of the Strategy pattern. First, if the logic to be used is statically determined, the Strategy pattern is unnecessary. In this case, simply splitting the functions of MessageServiceUseCase is sufficient.

class MessageServiceUseCase(
    private val messageRepository: MessageRepository,
    private val profileRepository: ProfileRepository,
    ...
) {
    fun sendMessage(message: Message) {
        // ... actual logic
        // Use `messageRepository` here
    }

    fun updateProfileStatus(statusText: String) {
        // ... actual logic
        // Use `profileRepository` here
    }
}

class SendMessageButton(
    ...
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val message = Message(messageTextForm.text, ...)
        useCase.sendMessage(message)

        messageTextForm.clear()
    }
}

class ProfileStatusEditToggleButton(
    ...
) : MessageServiceButton() {
    override fun onClicked(button: Button) {
        val isEditable = !button.isChecked
        button.isChecked = isEditable 
        
        statusTextForm.isEditable = isEditable
        if (!isEditable) {
            useCase.updateProfileStatus(statusTextForm.text)
        }
    }
}

Even if the Strategy pattern becomes necessary, there is no need to make the Strategy class a sealed class to guarantee exhaustiveness. The Strategy pattern is implemented using mechanisms (such as dynamic dispatch) that dynamically determine logic, so branching to determine logic should not be necessary. (Of course, sealed class is effective in preventing new Strategies from being defined externally.)

In a nutshell

Avoid using the Strategy pattern if the logic to be used is statically determined.

Keywords: strategy pattern, conditional branch, sealed class

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality