Improving code quality - Session 19: Child safety lock

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.

Child safety lock

Let's say there's an abstract class MessageData as a data model for messages. There are several types of messages, and these types are represented by defining subclasses of MessageData.

Now, to display a list of subclasses T of MessageData on the screen, an abstract class MessageListPresenter<T : MessageData> is defined as follows. However, since the display logic for messages varies by type, the list display logic is not implemented in MessageListPresenter.bind. The parent class's bind only handles the display of the header and footer, expecting the list display to be implemented by overriding in the subclasses. (In Kotlin, open is a modifier that means the function can be overridden.)

abstract class MessageListPresenter<T : MessageData>(
    ...
) {
    private val headerView: ... = ...
    private val footerView: ... = ...

    open fun bind(messageList: List<T>) {
        updateHeader(messageList.size)
        updateFooter(messageList)

        // The display of `messageList` is not implemented here.
        // Override this function to implement the display of `messageList`.
    }

    private fun updateHeader(messageCount: Int) {
        headerView.text = ...
        headerView....
    }

    private fun updateFooter(messageList: List<T>) {
        footerView....
    }
}

The following SomeSpecificMessageListPresenter is an implementation example of MessageListPresenter. It specifies SomeSpecificMessageData as the type parameter and defines the list display logic specific to that class in bind.

class SomeSpecificMessageListPresenter(
    ...
) : MessageListPresenter<SomeSpecificMessageData>(
    ...
) {
    override fun bind(messageList: List<SomeSpecificMessageData>) {
        super.bind(messageList)

        ... // Display `messageList`
    }
}

Are there any issues with this code?

Preventing mischief in subclasses

In many cases, the need to explicitly call super indicates that the range of the overridable function is too broad (exceptions are discussed later). In this code, the broad range of the overridable function can lead to the following implementation mistakes:

• Forgetting to call super: All subclasses are expected to call super.bind to update the header and footer. However, if super.bind is forgotten, a bug occurs where the header and footer are not updated. Moreover, since no error occurs at that time, the bug is easy to overlook.
• Forgetting to override bind: The requirement to override when inheriting is only explained in an inline comment. In situations where there are other functions to override, it's easy to forget to override, leading to bugs. To enforce overriding, you might want to use the abstract modifier, but it's difficult when the header and footer update logic is mixed in.
• Unexpected bind implementation: As stated in the inline comment, the overridden bind is expected to implement only the display of the message list. However, when bind is called, the header and footer are also displayed. In other words, the responsibility of the override and the function's responsibility do not match. This can lead to misunderstandings about the override's responsibility, resulting in code outside the responsibility being implemented in subclasses.

To solve this problem, instead of making the bind function open, separate the function that updates the list, updateMessageList, and make it abstract as an abstract method (or pure virtual function).

abstract class MessageListPresenter<T : MessageData>(
    ...
) {
    ...

    fun bind(messageList: List<T>) { // Not `open`
        updateHeader(messageList.size)
        updateFooter(messageList)
        updateMessageList(messageList)
    }

    protected abstract fun updateMessageList(messageList: List<T>)

    ...
}

The subclass would look like this:

class SomeSpecificMessageListPresenter(
    ...
) : MessageListPresenter<SomeSpecificMessageData>(
    ...
) {
    override fun updateMessageList(messageList: List<SomeSpecificMessageData>) {
        ... // Display `messageList`
    }
}

This way, you can make the flow of "updating the header, footer, and message list" in bind unchangeable while allowing the message list implementation to vary by subclass. Limiting the range that subclasses can change is important for making the code robust.

Super is too super

To avoid similar problems, it's good to avoid the following situations:

1. Calling an overridden parent class function (explicitly using super). However, lifecycle-related functions (constructors, destructors, etc.) and cases where it's necessary due to platform or library constraints (like Activity.onCreate in Android) are exceptions.
2. Implementing a "flow of functions" in each subclass even though it's common across subclasses.

If these situations apply, try to limit the range of overridable functions.

Aside: Private virtual in C++

In C++, you can define private virtual functions. These functions can only be called in the parent class, but the call is dispatched dynamically. By using private virtual functions, you can effectively prevent changes to the overall flow (callingPrivateVirtual) while allowing the logic (privateVirtual) to vary by subclass.

class Base
{
public:
    void callingPrivateVirtual()
    {
        ... // Other logic
        privateVirtual(); // Can only be called in `Base`
    }

private:
    virtual void privateVirtual() {}
};

class Derived : public Base
{
private:
    void privateVirtual() override
    {
        // Can change the implementation of `privateVirtual`.
    }
};

---

Summary

Limit the range of overridable functions as much as possible.

Keywords: override, super, inheritance

Read other articles on code quality improvement techniques

List of articles for the "Improving code quality" series