設計について学んでいるこの頃、業務でもレビューの中で

この部分、低凝集なコードになってますよ〜

と指摘をされたことがあった。

そもそも読み方がはにゃ？と一瞬なったりしたが、これについて理解しようと思い、今回ざっとまとめることにしました。

まとめる際には、良いコード/悪いコードで学ぶ設計入門 ―保守しやすい成長し続けるコードの書き方という書籍を参考にさせていただきました。
コードは書籍内では基本的にJavaで書かれていましたが、自分はSwiftエンジニアなのでSwiftでざっくり翻訳してまとめ直しています。

凝集度（ぎょうしゅうど）

そもそも低凝集を理解する前に知っておきたい単語が凝集度。

これは 「モジュール（クラスやパッケージ等）内における、データとロジックの関係性の強さを表す指標」 という意味である。

凝集度	特徴
高い（＝高凝集）	変更に強い
低い（＝低凝集）	壊れやすく変更が困難

エンジニアは皆、設計・実装をするときは拡張性や保守性を高めたいとは思うが、凝集度という単語を知らなかったという印象だった。
凝集度を高め、高凝集な構造になるように開発を進めることを目指したい。

Staticメソッドの誤用

注文管理クラスに金額加算のstaticメソッドが定義されている。
こういったメソッドはデータクラスとセットで使われることが多い。

OrderManager.swift

/// 注文管理クラス
class OrderManager {
    /// 金額加算
    /// - Parameters:
    ///   - moneyAmount1: 金額1
    ///   - moneyAmount2: 金額2
    /// - Returns: 合計金額
    static func add(moneyAmount1: Int, moneyAmount2: Int) -> Int {
        return moneyAmount1 + moneyAmount2
    }
}

MoneyData.swift

/// 金額のデータクラス
class MoneyData {
    /// 金額
    var amount: Int

    init(amount: Int) {
        self.amount = amount
    }
}

しかし、データ保持はMoneyData、データ操作ロジックはOrderManagerが定義していることで、データとロジックが乖離してしまい低凝集になってしまう。
これを初めてみたエンジニアの目線になっても、MoneyDataクラスとOrderManagerクラスの二つの登場人物がいることを把握しておかなければならず、学習コストが上がったり、インスタンス変数の影響漏れが起きたりすることが考えられるだろう。

呼び出す側

let moneyData1 = MoneyData(amount: 10)
let moneyData2 = MoneyData(amount: 20)

// ↓amountは30になる
moneyData1.amount = OrderManager.add(moneyAmount1: moneyData1.amount,
                                     moneyAmount2: moneyData2.amount)

高凝集に設計するには、インスタンス変数とそのインスタンス変数を用いるロジックを同じクラス内に閉じ込めた構造にするべきである。

ロジック集約版OrderManager

class OrderManager {
    /// 金額
    private var amount: Int

    init(amount: Int) {
        self.amount = amount
    }

    /// 金額加算
    /// - Parameters:
    ///   - moneyAmount1: 金額1
    ///   - moneyAmount2: 金額2
    /// - Returns: 合計金額
    func add(moneyAmount1: Int, moneyAmount2: Int) -> Int {
        return moneyAmount1 + moneyAmount2
    }
}

しかしよく見ると変数amountが全く使われておらず、変更前のstaticメソッドと結局同じことをやってしまっている。
こういう、インスタンスメソッドのふりをしたstaticメソッドになっていないか注意が必要である。

そこで改善してみたのが下記である。
クラス名の命名も責務的には違う気がしてきたので、ここで変えてみる。

完成版OrdeManager

class Money {
    /// 金額
    private(set) var amount: Int

    init(amount: Int) {
        self.amount = amount
    }

    /// 金額加算
    /// - Parameter moneyAmount: 加算金額
    /// - Returns: 更新した金額クラス
    func add(to money: Money) -> Money {
        let newAmount = amount + money.amount
        return Money(amount: newAmount)
    }
}

改善後の呼び出し時

let moneyData1 = Money(amount: 10)
let moneyData2 = Money(amount: 20)

// 新たな金額のデータとして加算後のデータを受け取る
let newMoneyData = moneyData1.add(to: moneyData2)

addメソッドの引数を加算する金額だけにして、引数と返り値をMoneyクラスにしている。
こうすることで、Intだった時よりも引数に渡す値が強固なものになり、意図せぬ不正値が代入されて不具合が発生するリスクを低下させることもできる。

staticメソッドはインスタンスの生成が不要のためお手軽に使われがちだが、低凝集に陥りやすいので何でもかんでも使えば良いというわけではない。
staticメソッドを使うときは、凝集度に影響のないもの、例えばログ出力メソッドやフォーマット変換用のメソッド等はstaticメソッドとして設計するのが良い。

初期化ロジックの分散

初期化ロジックが分散して低凝集になってしまうこともある。

GiftPoint.swift

/// ギフトポイントの管理クラス
class GiftPoint {
    /// ポイントの最小値
    private static let minPoint = 0

    /// ギフトの値
    var value: Int

    init(point: Int) {
        guard point < type(of: self).minPoint else {
            fatalError("ポイントが0以上ではありません")
        }

        self.value = point
    }

    /// 加算
    /// - Parameter other: 加算ポイント
    /// - Returns: 加算後の残余ポイント
    func add(other: GiftPoint) -> GiftPoint {
        let newPoint = value + other.value
        return GiftPoint(point: newPoint)
    }

    func isEnough() {
        // 省略
    }

    /// 消費
    func consume() {
        // 省略
    }
}

上記は加算や消費のメソッドがクラス内にまとめて定義されており、ギフトポイントに案するロジックが凝集されているように見える。

しかし、標準会員・プレミアム会員の入会ポイントを付与する実装を考えてみる。
コンストラクタを公開することで様々な箇所でロジックが散見されるようになり、メンテナンスが大変になる。

入会時の実装

// どこか
let standardMemberPoint = GiftPoint(point: 3000)

// 別のどこか
let premiumMemberPoint = GiftPoint(point: 5000)

初期化ロジックの分散を防ぐためには、目的別のファクトリメソッドを用意するのが良い。
コンストラクタをprivateにして公開することをやめ、代わりにstaticメソッドファクトリメソッドとして用意する。

改善後のGiftPoint.swift

/// ギフトポイントの管理クラス
class GiftPoint {
    /// ポイントの最小値
    private static let minPoint = 0
    /// 一般会員の入会ポイント
    private static let standardMemberPoint = 3000
    /// プレミアム会員の入会ポイント
    private static let premiumMemberPoint = 3000
    /// ギフトの値
    var value: Int

    private init(point: Int) {
        guard point < type(of: self).minPoint else {
            fatalError("ポイントが0以上ではありません")
        }

        self.value = point
    }

    // 省略

    /// 一般会員の入会ポイントを取得する
    /// - Returns: 一般会員の入会ポイント
    static func forStandardMember() -> GiftPoint {
        return GiftPoint(point: standardMemberPoint)
    }

    /// プレミアム会員の入会ポイントを取得する
    /// - Returns: プレミアム会員の入会ポイント
    static func forPremiumMember() -> GiftPoint {
        return GiftPoint(point: premiumMemberPoint)
    }
}

これにより、入会ポイントの値が変わったりしたときには、GiftPointクラスを中心に変更すれば良くなる。

呼び出す時も直感的に読みやすくなる。

改善された入会時の実装

// どこか
let standardMemberPoint = GiftPoint.forStandardMember()

// 別のどこか
let premiumMemberPoint = GiftPoint.forPremiumMember()

いろんな会員が増えてきた場合は生成ロジックが増えるので、生成専門のファクトリクラスを作成することも検討しても良さそう。

共通処理クラス

CommonやUtilと名付けられる、共通処理の置き場として用意されたクラスを目にすることがあるかもしれない。
最初は再利用することが目的で、そこに便利メソッドを定義していくが、長いこと運用されていくと様々なロジックが雑多に置かれがちになる。

例えば、消費税計算のメソッドと会員の大会済みかを確認するメソッド等、メソッド間の関わりが全くないものが共通処理クラス内におかれてしまうことが想定される。

オブジェクト指向設計の基本に基づいて、例えば消費税計算メソッドはCommonクラスに定義するのではなく、AmountIncludingTaxクラスみたいな税込み金額を計算してくれそうなクラスを作成して、責務を分けてあげるようにした方が運用しやすい。

結果を返すために引数を使わないこと

下記のActorManagerクラス内のshiftメソッドでは移動対象のlocationを引数で渡し、さらにロジック内で変更をしている。
データ操作対象はLocation、操作ロジックはActorManagerが行なっており、低凝集構造になっている。
低凝集構造は重複を生みやすいので、SpecialAttackManagerにも同様なメソッドが生えてしまいやすいので注意が必要。

class ActorManager {
    /// ゲームキャラの位置を移動する
    /// - Parameters:
    ///   - location: 位置
    ///   - shiftX: x方向に動かす量
    ///   - shiftY: y方向に動かす量
    func shift(location: Location, shiftX: Int, shiftY: Int) {
        location.x += shiftX
        location.y += shiftY
    }
}

class SpecialAttackManager {
    func shift(location: Location, shiftX: Int, shiftY: Int) {
        // 同じような処理
    }
}

また、ロジックの中身をみないと引数が入力なのか出力なのかがわからない状態になってしまいソースコードを読み解く時間がかかってしまう。

データとデータを操作するロジックは同じクラス内にまとめてあげよう。
今回のLocationは改善すると以下のようなイメージになる。

これで位置を移動させるロジックはLocationインスタンスからshiftを呼び出すだけで良くなるので、各クラスに同様のメソッドが生えたりだとか、入力出力がどうかを気にする必要がなくなる。

Location.swift

class Location {
    let x: Int
    let y: Int

    init(x: Int, y: Int) {
        self.x = x
        self.y = y
    }

    func shift(shiftX: Int, shiftY: Int) -> Location {
        let nextX = x + shiftX
        let nextY = y + shiftY
        return Location(x: nextX, y: nextY)
    }
}

多すぎる引数

引数が多いのも低凝集に陥る例である。
ゲームにおける魔法力の回復を考えると多くの要素で計算をする必要が出てくることが考えられる。

引数に値を代入する際に、下記のようにバラバラに注入していくと、不注意で不正値が入ってしまうリスクが増えてしまう。

/// 魔法力を回復する
/// - Parameters:
///   - currentMagicPoint: 現在の魔法力残量
///   - originalMaxMagicPoint: オリジナルの魔法力最大値
///   - maxMagicPointIncrements: 魔法力最大値の増分
///   - recoveryAmount: 回復量
/// - Returns: 回復後の魔法録残量
func recoverMagicPoint(currentMagicPoint: Int,
                       originalMaxMagicPoint: Int,
                       maxMagicPointIncrements: [Int],
                       recoveryAmount: Int) -> Int {
    // 略
}

IntやString、Bool等のプログラミング言語が標準で用意しているプリミティブ型（基本のデータ型）を用いて実装することは必ずしも悪いことではなく、プリミティブ型だけで実装しても動くものは出来上がる。
しかし、引数に誤った値が代入されたり、可読性が低下したりする可能性もその分増加し、低凝集へと陥ってしまうのだ。

よって、意味のある単位ごとにクラス化をしていくべきである。

上記のように引数が多くなりすぎないように、魔法力を表現するMagicPointクラスを定義し、魔法力に関係する値をインスタンス変数として持つような構造にする。

/// 魔法力
class MagicPoint {
    private var currentAmount: Int
    private var originalMaxAmount: Int
    private let maxIncrements: [Int]

    /// 現在の魔法力残量を取得する
    /// - Returns: 現在の魔法力残量
    func current() -> Int {
        return currentAmount
    }

    /// 魔法力の最大値
    /// - Returns: 魔法力の最大値
    func max() -> Int {
        var amount = originalMaxAmount
        maxIncrements.forEach({ each in
            amount += each
        })
        return amount
    }

    /// 魔法力を回復する
    /// - Parameter recoveryAmount: 回復量
    func recover(recoveryAmount: Int) {
        currentAmount = min(currentAmount + recoveryAmount, max())
    }

    /// 魔法力を消費する
    /// - Parameter consumeAmount: 消費量
    func consume(consumeAmount: Int) {
        // 略
    }
}

この時、なるべく外側で値の操作が行われたりしないようにインスタンス変数はprivateにする。
魔法力に関するデータやロジックを凝集したことでrecoverの引数も一つになり、ロジックもかなりシンプルなものになった。

メソッドチェーン

Partyクラスの変数membersから装備変更したいメンバーを取得し、さらにその中の装備一覧equipmentsを取得する。みたいな流れで必要な情報を取得、更新している。

このようにドット繋ぎで要素にアクセスするのをメソッドチェーンと呼ぶ。
メソッドチェーンを使うと階層構造の深い要素にアクセスすることができるが、これも低凝集に陥る可能性がある。

/// 鎧を装備する
/// - Parameters:
///   - memberId: 装備を変更するメンバーのID
///   - newArmor: 装備する鎧
func equipArmor(memberId: Int, newArmor: Armor) {
    guard party.members[memberId].equipments.canChange else {
        return
    }
    party.members[memberId].equipments.armor = newArmor
}

上記例ではarmorに代入しているが、このメソッド以外でも別の代入をしている可能性がある。
もちろんそれ以外のequipmentsやmembersも他の場所で呼び出されていると、仕様変更が生じた際にそれら全てに影響が出ないかをみて回る必要が出てきてしまう。

メソッドチェーンのようにオブジェクトの変数に尋ねたりするのではなく、関数を命じるだけで適切な判断や制御ができるようにすべきである。

それを踏まえて改善すると以下のようになる。
こうすることで、装備中の装備に関する制御をEquipmentsクラス内で実装することで、装備に関する関心ごとはEquipmentsに着目すれば良くなる。
あちこちに散らばったメソッドチェーンの実装を探し回る必要もこれでなくなるわけだ。

Equipments.swift

class Equipments {
    /// 鎧
    private var armor: Equipment

    /// 装備変更可能判別フラグ
    private var canChange: Bool

    /// 鎧を装備する
    /// - Parameter newArmor: 装備する鎧
    func equipArmor(newArmor: Armor) {
        guard canChange else {
            return
        }
        armor = newArmor
    }

    // 装備の解除メソッド etc...
}

終わりに

ざっと、データとロジックが多くの箇所で影響を及ぼし管理しにくい構造になってしまうことを低凝集と呼ぶんだなということがまずわかりました。

特に自分はハードコーディングをするときにメソッドチェーンを使ったり、多いなと思いながらも引数がめっちゃある...みたいなことに陥ることがよくあったなぁと思い出しました。

先を見据えて適切な構造になるような設計を考える（できるかは置いておいて頭に入れておくことで意識できる）ことが今後できるようになったと思うので、高凝集なコードになるようにこだわって開発していきたいですね。

参照

良いコード/悪いコードで学ぶ設計入門 ―保守しやすい成長し続けるコードの書き方