こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。
この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 46 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）
関数は見かけによらず
以下の FooItemRepository は、FooItem というデータモデルを取得するためのレポジトリクラスです。この FooItem は FooItemStore にクエリすることで取得されるのですが、その結果を cachedItems にキャッシュしています。
この FooItemRepository には getItem と getItemAsync の 2 つの関数があり、それぞれ FooItem を同期的・非同期的に返します。ただし、getItem は呼び出し元をブロックしないようにするため、キャッシュされた値のみを使います。
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) }
}
}なお、getItemAsync で CompletableFuture を使っている理由は議論を単純にするためであり、Kotlin では coroutine の方がより適切です。また、このコードではスレッドの管理や排他制御を省略していますが、これらは議論を単純化であるため、問題ないと仮定してください。(余談ですが、たとえ ConcurrentHashMap を使っていても、MutableMap にアップキャストすると getOrPut はアトミックな操作ではなくなります。)
他の部分について、このコードに問題点はありますか？
名実相伴わせる
似たような名前をもつ複数の関数・型・変数を定義するときは、名前の差異と振る舞いの差異に一貫性をもたせる ことが重要です。名前の違い以上に仕様の違いがあると、勘違いによるバグの原因になります。
getItem と getItemAsync という名前からは、これらの関数の動作の違いは同期・非同期だけであるように見えるでしょう。しかし実際には、値の取得方法に大きな違いがあります。
getItem: キャッシュからFooItemを取得する。ない場合はnullを返す。getItemAsync: キャッシュからFooItemを取得する。ない場合はfooItemStoreから取得する。
もし、getItemAsync の呼び出し元が非同期処理を行うようになったとすると、単純に getItemAsync を getItem に置き換えてしまうかもしれません。その結果、キャッシュされていないときに値を取得できないというバグを発生させます。
この問題を解決するためには、「名前に沿うように動作を変更する」か「動作に沿うように名前を変更する」とよいでしょう。以下のコードは、動作を名前に合わせた例です。この変更を行う際は、一方の関数がもう一方の関数をラップしたり、共通部分を抽出したりすることで 信頼できる唯一の情報源 (single source of truth) を満たせます。
/**
* 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) }一方で、以下の例では名前を動作に合うように変更しています。getCachedItem と getOrFetchItemAsync という名前は不格好に見えるかもしれませんが、このような率直な名前を使うことで、少なくとも「同じ動作をする」という勘違いを防ぐことができます。
/**
* 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) }
}一言まとめ
似たような名前を使って定義するときは、名前の差異と仕様の差異をできる限り一致させる。
キーワード: naming, consistency, principle of least astonishment