The original article was published on August 15, 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.
Where is the master key?
The following MessageUiAttributeMapping class holds UI information (message text, background color, icon image) for each MessageId and applies it to UI elements. You can add new information by calling registerAttributes and reflect that information in MessageUiLayout by calling bindAttributes.
class MessageUiAttributeMapping {
private val messageTextMap: MutableMap<MessageId, String> = mutableMapOf()
private val backgroundColorIntMap: MutableMap<MessageId, Int> = mutableMapOf()
private val iconImageMap: MutableMap<MessageId, Image> = mutableMapOf()
fun bindAttributes(uiLayout: MessageUiLayout, messageId: MessageId) {
val messageText = messageTextMap[messageId] ?: return
uiLayout.messageUiElement.text = messageText
val backgroundColorInt = backgroundColorIntMap[messageId] ?: return
uiLayout.messageUiElement.backgroundColor = backgroundColorInt
val iconImage = iconImageMap[messageId] ?: return
uiLayout.iconUiElement.image = iconImage
}
fun registerAttributes(messageId: MessageId, messageText: String, backgroundColorInt: Int, iconImage: Image) {
messageTextMap[messageId] = messageText
backgroundColorIntMap[messageId] = backgroundColorInt
iconImageMap[messageId] = iconImage
}
...
}Are there any improvements for this class?
Do not duplicate the master key
The three maps messageTextMap, backgroundColorIntMap, and iconImageMap have an implicit constraint that "each element is linked with the same MessageId". For example, if messageTextMap[42] returns a non-null value, then backgroundColorIntMap and iconImageMap must also return non-null values. However, this is not constrained by type.
When handling optional values (nullable values) due to specification changes, this can cause bugs. For instance, consider changing the type of iconImageMap to Map<MessageId, Image?>. In this case, null can mean both "not registered" and "explicitly registered as null". This can be distinguished with containsKey, but it leads to error-prone behavior.
Additionally, when you want to iterate over all MessageIds, you need to consider how to create the set of keys. messageTextMap.keys is one way, but logic based on such implicit constraints is prone to break during specification changes. After repeated updates, iconImageMap might have its own unique MessageId.
When there is an implicit relationship between elements by key, it is preferable to create a type that makes it explicit. For example, for maps Map<Key, Foo> and Map<Key, Bar>, consider creating a class FooBarPair(val foo: Foo, val bar: Bar) and integrating the maps as Map<Key, FooBarPair>. Applying this idea to MessageUiAttributeMapping results in the following:
class MessageUiAttributeMapping {
private val attributesMap: MutableMap<MessageId, UiAttributes> = mutableMapOf()
fun bindAttributes(uiLayout: MessageUiLayout, messageId: MessageId) {
val attributes = attributesMap[messageId] ?: return
uiLayout.messageUiElement.text = attributes.messageText
uiLayout.messageUiElement.backgroundColor = attributes.backgroundColorInt
uiLayout.iconUiElement.image = attributes.iconImage
}
fun registerAttributes(messageId: MessageId, messageText: String, backgroundColorInt: Int, iconImage: Image) {
attributesMap[messageId] = UiAttributes(messageText, backgroundColorInt, iconImage)
}
...
private class UiAttributes(val messageText: String, val backgroundColorInt: Int, val iconImage: Image)
}By integrating the maps in this way, the relationships between elements become explicit, making it more robust against specification changes such as adding error handling.
Note 1: Collections other than maps
The same idea can be applied to other collections like lists. For example, if you have two lists List<Foo> and List<Bar> where elements are related by list index, you can create a class FooBarPair(val foo: Foo, val bar: Bar) and integrate them as List<FooBarPair>.
Note 2: Application beyond properties
While MessageUiAttributeMapping used properties as an example, the same discussion applies to relationships between function arguments or between arguments and return values. Instead of linking Id and Element by index in a function like findElements(ids: List<Id>): List<Element>, it is preferable to make the relationship explicit with findElements(ids: Set<Id>): Map<Id, Element>.
Note 3: Trade-offs with performance
If sequential access to elements is crucial for performance, you might need to decompose arrays per element. For example, decomposing arrays by color for image processing or separating specific attributes for statistical calculations. However, be aware that premature optimization is the root of all evil.
If implicit relationships are necessary for optimization, you need to encapsulate those relationships within types or modules.
In a nutshell
Avoid implicit relationships between elements by key or index.
Keywords: key, collection, implicit relationship