Is this a good interface for a simple class?

I made a class that caches and downloads a string. I know that Kotlin is all about succinctness and I’m fairly new to Kotlin, so I don’t have the “muscle memory” yet to know if this is how such a class would be instantiated and used.

Class:

class Cacher (
    private val appl: Application,
    private val cacheKey: String,
    private val url: String,
) {
    suspend fun get(): String? {
        [...]
    }
}

Usage:

val s = Cacher(
    requireActivity().application,
    "example_cache",
    "https://example.com"
).get()

API is pretty simple, so it is hard to make it more idiomatic. My two cents are related to OOP in general.

1. It feels like a pretty heavy thing for just downloading a single string. You could consider creating a single cacher object that has a map key->url and then you use it like this:

cacher.get("example_cache")

2. If you want to keep it as it is, then I think this class really performs two unrelated things that could/should be separated: downloading with HTTP; and caching with suspending and synchronization. You could consider creating a more generic function for suspendable caching like this:

fun <T> suspendLazy(init: suspend () -> T): SuspendLazy<T> { TODO() }

interface SuspendLazy<T> {
    suspend fun get(): T
}

Then you can use it directly, ignoring your Cacher class entirely:

val cachedString = suspendLazy { http.get("https://example.com") }

Or you can create cacher on top of it:

fun cacher(appl: Application, cacheKey: String, url: String) = suspendLazy { http.get(url) }

You could also consider not creating SuspendLazy interface, but reusing Deferred. It is a little different as it allows to cancel the execution, so depending on your case it may be good or bad thing.

Also, do you really need Application there? Isn’t Context enough?

1 Like

The caching and the downloading are quite entangled. When the cache is cold, it just downloads the resource and stores it together with the Expires and Date header. When the resource is in the cache, and the Expires hasn’t passed it returns the cached value. If it has expired, it downloads the resource but this time with the Date header in If-Modified-Since. If it gets a 304 Not Modified it returns the cached value, otherwise it caches and returns the new resource.
It would be an interesting exercise to split the responsibilities but it also seems like false abstraction.

You’re right.

Ahh, ok, in that case it makes sense to use a separate object per string and suspendLazy() won’t give you too much.

One last suggestion. You assume the value may change with time and this is so important in your application logic that you even decided to implement this into your Cacher. If you know you may sometimes need to observe the value of cached string as it is being updated, then it would make sense to return StateFlow<String?> instead of just String (null is for unknown). With such flow it is extremely easy to both suspend for a single value and subscribe to observe changes. But I imagine it would be harder to implement.

I am actually using this cacher with a flow. It looks basically like this:

object ServerConnection {

    private val _stateUpdates = MutableSharedFlow<State>()
    val stateUpdates = _stateUpdates.asLiveData()

    init {
        CoroutineScope(Dispatchers.IO).launch {
            // Retry loop
            while (true) {
                try {
                    while(true) {
                        val value = cacher.get()
                        _stateUpdates.emit(value)
                        delay(60_000)
                    }
                }
            }
        }
    }
}

(I omitted some additional processing that happens with the value before it is emitted.)

If I remember correctly, the way I arrived at this code was: I knew I had to run it on the IO threadpool, but then I couldn’t emit to the main thread, so I added a Channel, but with a Channel only one of the receivers got the updated value, so I googled and found the BroadcastChannel, which was deprecated in favor of the MutableSharedFlow. I discovered that is has a very convenient .asLiveData() method, so that’s what I’m using now.

The UI then subscribes to updates with ServerConnection.stateUpdates.observe(viewLifecycleOwner, { ... })

I know I am late to this party, but thought I would pass on the Cache class I wrote. It uses lamdas to separate the caching from how the value is retrieved if it isn’t cached or cache has expired.

interface ICache<K, V> {
    /**
     * Cache a [value] with a given [key]
     */
    fun put(key: K, value: V?, expiration: TimeSpan)

    /**
     * Get the cached value of a given [key], or null if it's not cached or evicted.
     */
    operator fun get(key: K): V?

    suspend fun getOrUpdate(key: K, expiration: TimeSpan, creator: suspend () -> V): V

    /**
     * Retrieve values at once and if any do not exist or are expired, call the given creator
     * to create them
     * @param key The list of keys to retrieve
     * @param expiration The expiration time to put on newly created items
     * @param creator The function to call to create items that are missing
     * @return The map of the requested keys and values
     */
    suspend fun bulkGetOrUpdate(
        keys: Iterable<K>,
        expiration: TimeSpan,
        creator: suspend(Iterable<K>) -> Map<K, V?>
    ): Map<K, V?>

    /**
     * Remove the value of the [key] from the cache, and return the removed value, or null if it's not cached at all.
     */
    fun remove(key: K): V?

    /**
     * Remove all the items in the cache.
     */
    fun clear()
}
open class Cache<K, V>(private val dateTimeSource: DateTimeSource) : ICache<K, V> {
    private val cache = HashMap<K, Pair<V?, DateTime>>()

    override fun put(key: K, value: V?, expiration: TimeSpan) {
        cache[key] = value to dateTimeSource.utcDateTimeNow + expiration
    }

    override fun get(key: K): V? = getEntry(key)?.first

    override suspend fun getOrUpdate(key: K, expiration: TimeSpan, creator: suspend () -> V): V =
        this[key]
            ?: runCatching { creator() }
                .onSuccess { put(key, it, expiration) }
                .onFailure { remove(key) }

    override suspend fun bulkGetOrUpdate(
        keys: Iterable<K>,
        expiration: TimeSpan,
        creator: suspend(Iterable<K>) -> Map<K, V?>
    ): Map<K, V?> = buildMap {
        keys.filter { getEntry(it) != null }
            .forEach {
                this@Cache[it].run { put(it, this) }
            }

        keys.filterNot { containsKey(it) }
            .takeUnless { it.isEmpty() }
            ?.let { keysNotCached ->
                putAll(creator(keysNotCached).onEach { put(it.key, it.value, expiration) })
            }
    }

    override fun remove(key: K): V? = cache.remove(key)?.first

    override fun clear() = cache.clear()

    private fun getEntry(key: K) =
        cache[key]
            ?.takeUnless { dateTimeSource.utcDateTimeNow > it.second }
            .also { it ?: remove(key) }
}

DateTimeSource is just a simple class I use to be able to inject a source of date time for testing