こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 “Weekly Report” 共有の第 12 回です。Weekly Report については、第 1 回の記事を参照してください。
セット割引
デバッグ用のデータを時系列で記録するクラス、SampledDataRecorder を作成していることを想定します。このクラスはrecordData に渡されたデータを記録しますが、DataImportance に応じてフィルタリングを行い、プロパティ dataCountPerSampling で指定された間隔でサンプリングをします。
enum class DataImportance(val level: Int) { TRIVIAL(0), NORMAL(1), IMPORTANT(2) }
class SampledDataRecorder<T>(private val record: DataRecord<T>) {
var isActive: Boolean = false
var minImportanceToRecord: DataImportance = DataImportance.NORMAL
@IntRange(from = 1)
var dataCountPerSampling: Int = 1
set(value) {
currentDataCount = 0
field = value.coerceAtLeast(1)
}
private var currentDataCount: Int = 0
fun recordData(importance: DataImportance, data: T) {
if (!isActive || importance.level < minImportanceToRecord.level) {
return
}
if (currentDataCount + 1 >= dataCountPerSampling) {
currentDataCount = 0
record.store(data)
} else {
currentDataCount++
}
}
}recordData に渡されたデータが記録されるのは、以下の 3 つの条件が満たされたときです。
isActiveがtrue- 引数の
importanceがminImportanceToRecord以上 recordDataの呼び出しがdataCountPerSampling* n 回目 (n は正整数)
ここで、 isActive/minImportanceToRecord/dataCountPerSampling の 3 つのプロパティは、デバッグの UI から変更できるようにするため、再代入可能であるという制約は必須とします。
この SampledDataRecorder で、改善すべき点はありますか？
個別変更は高くつく
isActive/minImportanceToRecord/dataCountPerSampling の 3 つが独立して変更可能になっているため、混乱を招きやすくなっています。これらのプロパティを、1 つのクラスにまとめ、一度に更新させる ことで、動作を単純にしやすくなります。
上記のコードの混乱しやすい点としては、以下が挙げられます。
isActiveをfalseからtrueに変えたとき、以前のminImportanceToRecordとdataCountPerSamplingの値が継続して使われる。minImportanceToRecordやdataCountPerSamplingを変更してisActiveをtrueをするとき、誤ってisActiveの変更を先に行ってしまうと、古いフィルタリングやサンプリングを行ってしまう。
currentDataCountはdataCountPerSampling変更時にリセットされるが、isActiveやminImportanceToRecordの変更ではリセットされない。- プロパティセッターが非同期に呼ばれたとき、たとえシングルスレッドでも競合状態に気をつけなければならない。
この問題を解決するためには、minImportanceToRecord と dataCountPerSampling を 1 つのクラス SamplingPolicy としてまとめ、isActive は null で表現すればよいでしょう。
class SampledDataRecorder<T>(private val record: DataRecord<T>) {
/**
* Current policy of ...(snip)...
* `null` value represents this recorder is inactive.
*/
private var policy: SamplingPolicy? = null
private var currentDataCount: Int = 0
fun startRecording(dataCountPerSampling: Int, minImportanceToRecord: DataImportance) {
policy = SamplingPolicy(dataCountPerSampling.coerceAtLeast(1), minImportanceToRecord)
currentDataCount = 0
}
fun finishRecording() {
policy = null
}
fun recordData(importance: DataImportance, data: T) {
val currentPolicy = policy ?: return
if (importance.level < currentPolicy.minImportanceToRecord.level) {
return
}
if (currentDataCount + 1 >= currentPolicy.dataCountPerSampling) {
currentDataCount = 0
record.store(data)
} else {
currentDataCount++
}
}
private class SamplingPolicy(
@IntRange(from = 1) val dataCountPerSampling: Int,
val minImportanceToRecord: DataImportance
)
}このコードでは、以下のような利点があります。
dataCountPerSamplingとminImportanceToRecordが必ず同時に更新されることが保証される。- 記録再開時に、意図せず古いポリシーが使われるというバグを防げる。
currentDataCountがリセットされる条件が明確。- 非同期の更新の取り扱いが容易になる。このクラスをスレッドセーフにしたい場合も、変更箇所を限定できる。
まとめ
状態を変更するとき、個々のプロパティを直接変更させるよりも、変更を限定させるインターフェースを作ったほうが良い場合があります。プロパティの関係が非直交な場合 (参照: https://speakerdeck.com/munetoshi/code-readability-session-4-ver-2-ja?slide=14 ~ slide=32) や、複数のプロパティの変更を一度に行う必要がある場合は、特に利点が大きいです。
一言まとめ
状態の更新タイミングや値の組み合わせを制限するインターフェースを用意したほうがよい場合がある。
キーワード: mutability, updating interface, state object