コード品質向上のテクニック：第48回ワイルドすぎる引数

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

この記事は、毎週木曜の定期連載 "Weekly Report" 共有の第 48 回です。 LINEヤフー社内には、高い開発生産性を維持するための Review Committee という活動があります。ここで集まった知見を、Weekly Report と称して毎週社内に共有しており、その一部を本ブログ上でも公開しています。（Weekly Report の詳細については、過去の記事一覧を参照してください）

ワイルドすぎる引数

「図形」というデータが、スペースで区切られた文字列として表現されているとします。以下は、その文字列の例です。スペース区切りの最初の値は、図形の種類を示す「タグ」と呼ばれるもので、2 つ目以降は図形ごとの属性 (座標など) だとします。

circle 10 20 30
rectangle 10 20 30 40
line 10 20 30 40

この文字列を解釈するコードを実装するとしましょう。各行のデータは、以下のようなあらかじめ定義されたデータモデルに変換されます。

class Circle(centerX: Int, centerY: Int, radius: Int): Shape
class Rectangle(left: Int, top: Int, right: Int, bottom: Int): Shape
class Line(startX: Int, startY: Int, endX: Int, endY: Int): Shape

図形の種類によらず、文字列の形式は似ています。そのため、ある開発者は図形共通のファクトリ関数 create を、以下のように作るのが良いと考えました。

enum class ShapeFactory(private val tag: String) {
    CIRCLE("circle") {
        override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape =
            Circle(param1, param2, param3)
    },
    RECTANGLE("rectangle") {
        override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape {
            requireNotNull(param4)
            return Rectangle(param1, param2, param3, param4)
        }
    },
    LINE("line") {
        override fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape {
            requireNotNull(param4)
            return Line(param1, param2, param3, param4)
        }
    };
    
    abstract fun create(param1: Int, param2: Int, param3: Int, param4: Int?): Shape
    
    companion object {
        val TAG_TO_FACTORY_MAP: Map<String, ShapeFactory> =
            ShapeFactory.entries.associateBy(ShapeFactory::tag)
    }
}

このファクトリと String.split を利用することで、文字列を解釈するコードは以下のように実装できます。

fun parseShape(shapeString: String): Shape? {
    val params = shapeString.split(" ")
    val factory = ShapeFactory.TAG_TO_FACTORY_MAP[params[0]]
        ?: return null

    val param1 = params[1].toInt()
    val param2 = params[2].toInt()
    val param3 = params[3].toInt()
    val param4 = params.getOrNull(4)?.toInt()
    return factory.create(param1, param2, param3, param4)
}

これらのコードになにか改善点はありますか？

ワイルドカードは伏せておく

基本的には、複数の意味に解釈されるような、汎用的すぎる引数は避けるべき です。create の引数の意味は、図形の種類によって全く異なるものになってしまっています。パラメータ param1 は、Circle であるならば中心の x 座標を意味し、Rectangle であれば左端の座標を意味しています。このような、他の引数やレシーバの状態などに依存して、意味が変わるような引数の定義は好ましくありません。

このような引数によって引き起こされる問題には、以下のようなものが挙げられます。

柔軟性について: 引数の追加、削除、更新が困難になる。例えば、5 つの引数を必要とする図形を追加しようとした場合、他の図形についても create 関数シグネチャの更新が必要になる。他にも、今の引数型とは異なる引数 (String や enum) が必要になった場合、過剰に複雑な定義になってしまう。
頑健性について: 図形の種類によって引数の数が異なる場合、引数の数を検証するコードが必要になる。このような検証のコードが増えると、エラーの取り扱いも煩雑になりがちになる。特に、不要なランタイムエラーの可能性を発生させてしまうと、頑健性に大きな影響が発生する。

この問題をイメージしやすいよう、「ワードラベル」という 1 単語のテキストを表示する図形を追加することを想定しましょう。ワードラベルのデータは、図形の左上の座標 (x, y) と、スペース以外の任意の文字列で構成されます。

label 10 20 word

対応するデータモデルは以下のようになるでしょう。

class WordLabel(x: Int, y: Int, word: String): Shape

このパーサを実装しようとすると、create の引数の型を変えなければなりません。図形によって、param3 の型は Int にも String にもなり得るからです。param3 で Int と String の両方を扱うには、Any にする方法や、param3 用の sealed class を定義する方法、タイプパラメータを使う方法などがあります。しかし、どれも一長一短な上、既存の図形のコードが影響を受けてしまいます。

この問題は、「文字列をパースする」責任を適切な場所に置くことで解決できます。タグを分解するコードは共通のロジックとしてまとめても良いのですが、値の解釈の部分は各図形と強くひもづけられるべきです。以下の修正案の createByMatchResult において、引数 group は「タグを取り除いた値の部分」という共通の意味を持ちます。

enum class ShapeFactory(private val tag: String, private val parameterRegex: Regex) {
    CIRCLE("circle", """(-?\d+) (-?\d+) (\d+)""".toRegex()) {
        override fun createByMatchResult(group: List<String>): Shape =
            Circle(group[1].toInt(), group[2].toInt(), group[3].toInt())
    },
    RECTANGLE("rectangle", """(-?\d+) (-?\d+) (-?\d+) (-?\d+)""".toRegex()) {
        override fun createByMatchResult(group: List<String>): Shape =
            Rectangle(group[1].toInt(), group[2].toInt(), group[3].toInt(), group[4].toInt())
    },
    LINE("line", """(-?\d+) (-?\d+) (-?\d+) (-?\d+)""".toRegex()) {
        override fun createByMatchResult(group: List<String>): Shape =
            Line(group[1].toInt(), group[2].toInt(), group[3].toInt(), group[4].toInt())
    };

    fun create(parameterText: String): Shape? {
        val matchResult = parameterRegex.matchEntire(parameterText)
            ?: return null
        return createByMatchResult(matchResult.groupValues)
    }

    protected abstract fun createByMatchResult(group: List<String>): Shape

    companion object {
        val TAG_TO_FACTORY_MAP: Map<String, ShapeFactory> =
            ShapeFactory.entries.associateBy(ShapeFactory::tag)
    }
}

このファクトリを使った文字列解釈のコードは、以下のようになります。

fun parseShape(shapeString: String): Shape? {
    val tagAndParams = shapeString.split(" ", limit = 2)
    val factory = ShapeFactory.TAG_TO_FACTORY_MAP[tagAndParams[0]]
    val params = tagAndParams.getOrNull(1).orEmpty()
    return factory?.create(params)
}

この書き方でもなお group[x].toInt() というコードはあるものの、それに対応する正規表現のグループ(-?\d+) や (\d+) との関連が分かりやすくなっています。そのため、引数の使い方を間違う可能性は軽減されていると言えるでしょう。なお、今回の実装ではタグの分解や正規表現のテストを createByMatchResult の外で行っていますが、これらを createByMatchResult の内部で行うことも考えられます。

このリファクタリングにより、parameterText は「（タグを取り除いた）データ部の文字列」という意味を持つとみなせます。このように、引数は単一のものとして解釈可能にするのが重要です。

一言まとめ

複数の意味で解釈できるような汎用的すぎる引数の定義を避ける。

キーワード: parameter, semantics

コード品質向上のテクニックの他の記事を読む

コード品質向上のテクニックの記事一覧