Improving code quality - Session 12: Set discount

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.

Set discount

Let's assume you are creating a class, SampledDataRecorder, that records debugging data in chronological order. This class records data passed to recordData, filters it based on DataImportance, and samples it at intervals specified by the dataCountPerSampling property.

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++
        }
    }
}

Data passed to recordData is recorded when the following three conditions are met:

• isActive is true
• The importance argument is greater than or equal to minImportanceToRecord
• The recordData call is the dataCountPerSampling * n-th time (where n is a positive integer)

Here, the three properties isActive, minImportanceToRecord, and dataCountPerSampling must be reassignable to allow changes from the debugging UI.

Are there any improvements needed for this SampledDataRecorder?

Individual changes are costly

Since isActive, minImportanceToRecord, and dataCountPerSampling can be changed independently, it can lead to confusion. By combining these properties into one class and updating them all at once, you can simplify the behavior.

The following points illustrate the potential confusion in the above code:

• When isActive is changed from false to true, the previous values of minImportanceToRecord and dataCountPerSampling continue to be used.
  • If you change minImportanceToRecord or dataCountPerSampling and then set isActive to true, doing so in the wrong order can result in using old filtering or sampling values.
• currentDataCount is reset when dataCountPerSampling changes, but not when isActive or minImportanceToRecord changes.
• If property setters are called asynchronously, you need to be careful about race conditions, even in a single-threaded environment.

To solve this problem, you can combine minImportanceToRecord and dataCountPerSampling into a single class, SamplingPolicy, and represent isActive as 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
    )
}

This code has the following advantages:

• It ensures that dataCountPerSampling and minImportanceToRecord are always updated together.
• It prevents bugs where an old policy is unintentionally used when recording resumes.
• The conditions for resetting currentDataCount are clear.
• Handling asynchronous updates becomes easier. If you want to make this class thread-safe, you can limit the changes.

Summary

When changing states, sometimes it's better to create an interface that limits changes rather than directly changing individual properties. This is especially beneficial when the properties are not orthogonal or when you need to change multiple properties at once. For more information see slide 15-32 of Code readability: Session 4.

---

One-line summary

It is sometimes better to provide an interface that limits the timing and combination of state updates.

Keywords: mutability, updating interface, state object