Improving code quality - Session 8: Reality and illusion

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.

Reality and illusion

The following UserNameMapLoader manages user IDs and names obtained through apiClient. The data is not retrieved all at once but in "chunks".

To simplify the discussion, let's introduce the following assumptions:

1. This code runs only on a specific thread. (Thread safety doesn't need to be considered.)
2. The data obtained doesn't change, and the chunks are sorted by UserId. (Inconsistencies over time don't need to be considered.)

class UserNameMapLoader(private val apiClient: ApiClient) {
    private val userNameMap: MutableMap<UserId, String> = mutableMapOf()
    private var lastChunkIndex: Int = -1

    val loadedUserNameMap: Map<UserId, String>
        get() = userNameMap

    val loadedUserIds: Set<UserId>
        get() = userNameMap.keys

    val decoratedUserNameMap: Map<UserId, String>
        get() = userNameMap.mapValues { (_, value) -> "**$value**" }

    ...

    fun loadNextChunk() {
        val response = apiClient.requestChunk(lastChunkIndex + 1) as? Success
            ?: return

        userNameMap += response.chunk.toMapEntries()
        lastChunkIndex++
    }
}

Are there any points in this code that need improvement?

Stones roll on their own

If you execute the following code, it may appear that the values of userNameMap and decoratedUserNameMap are not consistent.

val userNameMap = loader.loadedUserNameMap
val userIds = loader.loadedUserIds
val decoratedUserNameMap = loader.decoratedUserNameMap

loadNextChunk()

println(userNameMap) // Shows {3=Alice, 5=Bob, 7=Charlie, 11=Dave}
println(userIds) // Shows [3, 5, 7, 11]
println(decoratedUserNameMap) // Shows {3=**Alice**, 5=**Bob**}

The three local variables are Map or Set, so they are all read-only (unmodifiable/readonly). However, there are differences in mutability.

• loadedUserNameMap: A reference to the original Map. It can change by loader.
• loadedUserIds: A view of the keys of the original Map. It can change by loader.
• decoratedUserNameMap: A copy of the Map created at the time of property access. It doesn't change by any loader operations.

There are several options to solve this problem.

Option 1: Allow only element access

By not returning Map or Set and instead returning names for individual UserId, you don't have to worry about subsequent updates.

class UserNameMapLoader(private val apiClient: ApiClient) {
    private val userNameMap: MutableMap<UserId, String> = mutableMapOf()

    fun getName(userId: UserId): String? = userNameMap[userId]

    ...

However, to implement this, you need to manage the list of UserId separately and change loadNextChunk() to loadNamesFor(userIds: List<UserId>).

Option 2: Return only copies

In some cases, you may want to return collections for subsequent processing. In that case, always return a copy.

class UserNameMapLoader(...) {
    private val userNameMap: MutableMap<UserId, String> = mutableMapOf()

    fun createUserNameMapSnapshot(): Map<UserId, String> =
        userNameMap.toMap() // `toMap` is to create a copy of the map.

    ...

By adding "create", "snapshot", "copy", "clone", "newInstance" to the function name, you can clearly indicate that a copy is returned. Also, by commenting on toMap or toList, you can prevent other developers from accidentally deleting the code that creates the copy.

However, this method may affect performance, so be careful. (If there is a copy-on-write mechanism like Swift's Array, it is good to use it.)

Option 3: Return only direct references

If other options cannot be applied, it may be necessary to return the reference to the original collection as is. However, be aware that the collection may change during processing. Therefore, this point should be mentioned in the documentation.

class UserNameMapLoader(...) {
    private val userNameMap: MutableMap<UserId, String> = mutableMapOf()

    /**
     * A map with UserId as the key and usernames.
     *
     * This map is read-only, but the contents can be changed through
     * operations on this `UserNameListLoader`.
     */
    val userNameMap: Map<UserId, String> = mutableUserNameMap

    ...

To adopt this option, functions that traverse the collection (filter, map, flatMap...) must be done at the caller. If you need to generalize the logic, organize dependencies by implementing utility functions instead of implementing them as members of the callee.

---

Summary

Clarify whether the return value of a function can change after retrieval.

Keywords: view of collection, copied value, mutability