こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 38 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）
マスターキーは何処
以下の MessageUiAttributeMapping は、MessageId ごとに UI 情報 (メッセージテキスト・背景色・アイコン画像) を保持し、それを UI 要素に適用するクラスです。registerAttributes を呼び出すことで新規の情報を追加し、bindAttributes を呼び出すことでその情報を MessageUiLayout に反映させます。
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
}
...
}このクラスに改善点はありますか？
マスターキーを複製しない
messageTextMap ・ backgroundColorIntMap ・ iconImageMap の 3 つのマップには、「各要素は同じ MessageId で紐づけられている」という暗黙的な制約があります。たとえば、messageTextMap[42] が null でない値を返すならば、backgroundColorIntMap や iconImageMap も null でない値を返さなければなりません。しかし、それは型による制約付けがされていません。
仕様変更でオプショナルな値 (nullable な値) を取り扱うとき、これはバグの原因になります。例として、iconImageMap の型を Map<MessageId, Image?> に変更した場合を想定します。このとき、null は「登録されていない」という意味と「null が明示的に登録されている」という 2 つの意味を持ちます。これは containsKey で区別可能なのですが、ミスを誘うような挙動になってしまいます。
また、全ての MessageId に対して走査したい場合、キーの集合をどのように作成するかを考える必要があります。messageTextMap.keys も一つの手段ですが、そのような暗黙的な制約に基づくロジックは、仕様変更時に壊されがちです。度重なる更新の末、iconImageMap が固有の MessageId を持つようになっているかも知れません。
マップの要素間で、キーによる暗黙的な関連があるときは、それを明示する型を作るのが好ましいです。例えば Map<Key, Foo> と Map<Key, Bar> というマップに対しては、FooBarPair(val foo: Foo, val bar: Bar) というクラスを作り、マップを Map<Key, FooBarPair> として統合することを検討しましょう。この考え方を MessageUiAttributeMapping に適用すると以下のようになります。
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)
}このようにマップを統合することで、要素の関連が明示的になり、エラー処理の追加といった仕様変更に対しても頑健になります。
Note 1: マップ以外のコレクション
同様の考え方は、リストなどの他のコレクションにも適用できます。例えば、2 つのリスト List<Foo> と List<Bar> があり、それらの要素がリストのインデックスで関連付けられている状況なら、FooBarPair(val foo: Foo, val bar: Bar) というクラスを作り、List<FooBarPair> として統合できます。
Note 2: プロパティ以外への適用
MessageUiAttributeMapping ではプロパティを例に取り上げましたが、同じ議論は関数の引数同士や、引数と戻り値の関係にも適用できます。findElements(ids: List<Id>): List<Element> という関数で、Id と Element の関係をインデックスで結びつけるよりは、findElements(ids: Set<Id>): Map<Id, Element> という形で関連を明示するほうが好ましいです。
Note 3: パフォーマンスとのトレードオフ
要素のシーケンシャルアクセスがパフォーマンス面で非常に重要な場合、要素ごとに配列を分解する必要があるかもしれません。例えば、画像処理のために色ごとに配列を分解したり、統計情報計算のために特定の属性を分離することが考えられます。ただし、時期尚早な最適化は諸悪の根源である 点に注意してください。
最適化 のために暗黙な関係を必要とする場合は、その関係性を型やモジュール内に閉じ込める必要があります。
一言まとめ
キーやインデックスによる、要素同士の暗黙的な関係を避ける。
キーワード: key, collection, implicit relationship