コード品質向上のテクニック: 第 12 回（セット割引）

こんにちは。コミュニケーションアプリ「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