コード品質向上のテクニック: 第 18 回（関数を見て関係を見ず）

こんにちは。コミュニケーションアプリ「LINE」のモバイルクライアントを開発している石川です。

この記事は、毎週木曜の定期連載 “Weekly Report” 共有の第 18 回です。Weekly Report については、第 1 回の記事を参照してください。

関数を見て関係を見ず

コードを書く上で、しばしばループのネストが必要になることがあります。「ページ」や「チャンク」で分割されたデータを処理することは、その典型例の 1 つに数えられます。

複数の Item によってチャンクが作られ、更にそのチャンクが集まってページを構成するという状況を考えます。ページのデータモデル Page は、以下のように定義されています。各 Page は、index によって順序付けられ、後続の Page が存在するならば (つまり、この Page が最後でないならば)、hasNext が true になります。

class Page(val index: UInt, val chunks: List<Item>, val hasNext: Boolean)

この Page を使って、すべての Item のメタデータを保存する関数 saveAllItemMetadata が以下のように実装されました。この関数では、page.hasNext が true であり続ける限り、次の Page を取得し、そこに含まれる Item のメタデータを保存しています。

fun saveAllItemMetadata() {
    var index = 0u
    while (true) {
        val page = requestPage(index) ?: return
        for (item in page.chunks) {
            val itemMetadata = calculateItemMetadata(item)
            repository.saveMetadata(item.id, itemMetadata)
        }
        if (!page.hasNext) {
            return
        }
        index = page.index + 1u
    }
}

この関数は、while と for によってネストされたループがあり、読みやすいとは言えません。そこで、以下のように内側のループを private 関数として抽出したとします。

fun saveAllItemMetadata() {
    var index = 0u
    while (true) {
        val page = requestPage(index) ?: return
        saveMetadataInPage(page)
        if (!page.hasNext) {
            return
        }
        index = page.index + 1u
    }
}

private fun saveMetadataInPage(page: Page) {
    for (item in page.chunks) {
        val itemMetadata = calculateItemMetadata(item)
        repository.saveMetadata(item.id, itemMetadata)
    }
}

この抽出によって、読みやすさが劇的に改善したとは言えません。より良いリファクタリングの方法はあるでしょうか？

関係から森を見る

先述のリファクタリングで読みやすさがあまり改善されていない理由の 1 つに、関数の境界と意味のまとまりの境界が一致していないことが挙げられます。saveAllItemMetadata が行うことをまとめると、以下の 2 つで表せます。

すべての Item を取得する
その Item のメタデータを保存する

saveMetadataInPage を抽出することによって、「すべての Item を取得する」というコードが 2 箇所に分割されています。その結果、「saveAllItemMetadata が何をするか」がかえって分かりづらくなりました。単に抽出しやすいという理由で、内部の構造をそのままに抜き出してしまうと、このような状況を引き起こしかねません。リファクタリングで抽出を行う際は、 抽出のしやすさではなく、どのような改善をもたらすかに着目すべき です。そのためにも、抽出を行う際には 「コードが何をするか」の意味のまとまり に注意する必要があります。

今回の場合、内側のループを抽出するよりも Item の列を取得する関数を作成するとよいでしょう。Kotlin ならば、Sequence<Item> や Iterator<Item> といったクラスで表現することができます。以下の requestItemSequence では、Item の列を Sequence<Item> で返しています。

fun saveAllItemMetadata() {
    for (item in requestItemSequence()) {
        val itemMetadata = calculateItemMetadata(item)
        repository.saveMetadata(item.id, itemMetadata)
    }
}

fun requestItemSequence(): Sequence<Item> = sequence {
    var page: Page? = requestPage(0u)
    while (page != null) {
        yieldAll(page.chunks)
        page = if (page.hasNext) requestPage(page.index + 1u) else null
    }
}

このようにすることで、ネストされたループを単一のループと見せかけることができます。その結果、requestItemSequence の内容を理解しなくても、saveAllItemMetadata を読むだけで「各 Item についてメタデータを作り、保存する」という流れが分かるようになりました。

このリファクタリングの考え方は、ループのネストのみならず、条件分岐のネストやデータ構造のネストにも適用できます。抽出を行う際は、既存の構造を残したほうがよいのか、それとも再構成したほうがよいのかを検討しましょう。

一言まとめ: 抽出を行う際は、「コードが何をするか」の意味のまとまりに注意する。

キーワード: extraction, boundaries of concept, nested loop