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 callsuper.bind
to update the header and footer. However, ifsuper.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 theabstract
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 overriddenbind
is expected to implement only the display of the message list. However, whenbind
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:
- 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 (likeActivity.onCreate
in Android) are exceptions. - 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