Improving code quality - Session 46: Functions are not what they seem

The original article was published on October 17, 2024.

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.

Functions are not what they seem

The following FooItemRepository is a repository class for retrieving a data model called FooItem. This FooItem is obtained by querying FooItemStore, and the result is cached in cachedItems.

This FooItemRepository has two functions, getItem and getItemAsync, which return FooItem synchronously and asynchronously, respectively. However, getItem uses only cached values to avoid blocking the caller.

class FooItemRepository(
    private val fooItemStore: FooItemStore,
    ...
) {
    private val cachedItems: MutableMap<FooId, FooItem?> = ConcurrentHashMap()

    /**
     * Returns [FooItem] corresponding to the given [id] if it's already cached.
     *
     * If the item is not yet cached, this returns null even though the value exists in the local store.
     */
    fun getItem(id: FooId): FooItem? = cachedItems[id]

    /**
     * Returns a future object of [FooItem] corresponding to the given [id].
     *
     * This first finds a cached value, and queries to local store if it's not cached.
     * Note that if there is no such an item in the store, this function queries to the store every time.
     */
    fun getItemAsync(id: FooId): CompletableFuture<FooItem?> = CompletableFuture.supplyAsync {
        cachedItems.getOrPut(id) { fooItemStore.selectFooItem(id) }
    }
}

Note that the use of CompletableFuture in getItemAsync is for simplicity of discussion, and coroutines are more appropriate in Kotlin. Also, this code omits thread management and mutual exclusion for simplicity, assuming they are not problematic. (As an aside, even if you use ConcurrentHashMap, upcasting to MutableMap makes getOrPut non-atomic.)

Are there any issues with this code in other areas?

Consistency in naming and behavior

When defining multiple functions, types, or variables with similar names, it is important to maintain consistency between the differences in names and the differences in behavior. If there is a greater difference in behavior than in names, it can lead to bugs due to misunderstandings.

From the names getItem and getItemAsync, it may seem that the only difference in their behavior is synchronous versus asynchronous. However, there is a significant difference in how values are retrieved.

• getItem: Retrieves FooItem from the cache. Returns null if not present.
• getItemAsync: Retrieves FooItem from the cache. If not present, retrieves from fooItemStore.

If the caller of getItemAsync starts performing asynchronous processing, they might simply replace getItemAsync with getItem. As a result, a bug may occur where the value cannot be retrieved when it is not cached.

To solve this problem, it is advisable to either "change the behavior to match the name" or "change the name to match the behavior." The following code is an example of aligning behavior with the name. When making this change, you can satisfy the single source of truth by wrapping one function with the other or extracting common parts.

    /**
     * Returns [FooItem] corresponding to the given [id].
     *
     * This first finds a cached value, and queries to local store if it's not cached.
     * Note that if there is no such an item in the store, this function returns null.
     */
    fun getItem(id: FooId): FooItem? =
        cachedItems.getOrPut(id) { fooItemStore.selectFooItem(id) }

    /**
     * Returns a future object of [FooItem] corresponding to the given [id].
     *
     * This is async version of [getItem]. See the function documentation for more details.
     */
    fun getItemAsync(id: FooId): CompletableFuture<FooItem?> =
        CompletableFuture.supplyAsync { getItem(id) }

On the other hand, the following example changes the names to match the behavior. The names getCachedItem and getOrFetchItemAsync may seem awkward, but using such straightforward names can at least prevent the misunderstanding that they perform the "same behavior."

    /**
     * Returns [FooItem] corresponding to the given [id] if it's already cached.
     *
     * If the item is not yet cached, this returns null even though the value exists in the local store.
     */
    fun getCachedItem(id: FooId): FooItem? = cachedItems[id]

    /**
     * Returns a future object of [FooItem] corresponding to the given [id].
     *
     * This first finds a cached value, and queries to local store if it's not cached.
     * Note that if there is no such an item in the store, this function queries to the store every time.
     */
    fun getOrFetchItemAsync(id: FooId): CompletableFuture<FooItem?> = CompletableFuture.supplyAsync {
        cachedItems.getOrPut(id) { fooItemStore.selectFooItem(id) }
    }

In a nutshell

When defining with similar names, align the differences in names and specifications as much as possible.

Keywords: naming, consistency, principle of least astonishment

Read other articles on techniques for improving code quality

List of articles on techniques for improving code quality