Java
Android
リファクタリング

[読書メモ]現場で役立つシステム設計の原則 - 「小さくまとめてわかりやすくする」

(Why)なぜわかりやすくする必要があるか?

製品のデリバリーにかかるコスト(※)を左右するのは、そのプログラムのわかりやすさであり、それは設計に帰着するため。

※ リリースにかかる工数と、障害の可能性の意味であると自分なりに解釈しています。

(How)どうやるのか?

ポイントは以下の5つです。

  1. コード整理の基本は名前と段落
  2. 短いメソッド、小さなクラスを使ってコードを整理する
  3. 値オブジェクトでわかりやすく安全にする
  4. コレクションオブジェクトで、複雑なロジックを集約して整理する
  5. クラス名やメソッド名と業務の用語が一致するほど、プログラムの意図がわかりやすくなり、変更が楽で安全になる。

(What)具体的に?

1. コード整理の基本は名前と段落

わかりにくい例.kt
var a = 2000
val b = if (c < 2014) 1.05 else 1.08
a = a * b
a = if (a < 2500) a * 0.8 else a
println("合計${a}円です。");
わかりやすい例.kt
val basePrice = 2000

val taxRate = if (year < 2014) 1.05 else 1.08
val afterTax = basePrice * taxRate

val result = if (afterTax < 2500) afterTax * 0.8 else afterTax

println("合計${result}円です。");

ポイント
- 「名は体を表す」ように変数名をつける
- 変数には一つの役割のみ持たせる
- 変数に代入を繰り返していると、修正の影響範囲が広くなってしまうため。副作用の原因になります。
- 段落ごとにプログラムを区切った後にも同じ変数に代入を繰り返していると、段落ごとの結合度が強くなってしまう。
- 意味のまとまりごとに段落を分ける(だらだら書かない)

2. 短いメソッド、小さなクラスを使ってコードを整理する

上記の変更をしたおかげで、メソッドの切り出しが容易になりました。
まずは同一クラス内で、処理をメソッドに抽出する例です。

わかりやすい例(再掲).kt
val basePrice = 2000

val taxRate = if (year < 2014) 1.05 else 1.08
val afterTax = basePrice * taxRate

val result = if (afterTax < 2500) afterTax * 0.8 else afterTax

println("合計${result}円です。");
メソッドを抽出した例.kt
fun main(args: Array<String>) {
    val basePrice = 2000

    val afterTax = getAfterTaxAmount(year, basePrice)

    val result = getDiscountedPrice(afterTax)

    println("合計${result}円です。");
}

fun getAfterTaxAmount(year: Int, basePrice: Int): Double {
    return basePrice * if (year < 2014) 1.05 else 1.08
}

fun getDiscountedPrice(amount: Int): Int {
    return if (amount < 2500) amount * 0.8 else amount
}

メソッドを抽出するメリットは、以下の通りです。

  1. 今後行われる変更がそのメソッド内に閉じ込められるので自信を持って修正作業を行うことができるようになること。
  2. コードにメソッド名という形で意図を伝えることができるようになること。
  3. 処理を再利用することが可能になること。

以下は異なるクラスでの重複コードをなくす例です。

User.kt
class User(name: String, prefecture: String) {
    fun isInTokyo(): Boolean {
        return prefecture == "Tokyo"
    }
}
Article.kt
class User(title: String, prefecture: String) {
    fun isInTokyo(): Boolean {
        return prefecture == "Tokyo"
    }
}

上記二つのオブジェクトは、同一のprefectureという値を持っていて、それに基づいた処理をするメソッドも持っています。
これはコードの重複ですね。

この場合、リファクタリングの方針としては下記の二つで方法が異なります。
i. 二つのオブジェクトの間に参照関係はない
ii. 二つのオブジェクトの間に参照関係がある

i. 二つのオブジェクトの間に参照関係はない

Prefectureというクラスを作成しそっちに重複したロジックを持って行くのがいいですね。
そうすれば、prefectureに基づいた判定ロジックを追加、修正、削除した時にも修正するのはPrefectureクラスのみになります。修正がしやすくなると同時に、修正漏れによる思わぬバグ等が起きにくくなります。

Prefecture.kt
class Prefecture(name: String) {
    fun isInTokyo(): Boolean {
        return name == "Tokyo"
    }
}

UserArticleは以下のようになります。それぞれメソッドを委譲する形でisInTokyoメソッドを実現します。
そのため、修正後もクライアントには変更ありません。

User.kt
class User(name: String, prefecture: Prefecture) {
    fun isInTokyo(): Boolean {
        return prefecture.isInTokyo()
    }
}
Article.kt
class User(title: String, prefecture: Prefecture) {
    fun isInTokyo(): Boolean {
        return prefecture.isInTokyo()
    }
}

ii. 二つのオブジェクトの間に参照関係がある

UserがArticleを持っているとすると、実装コードは以下のようになります。
所有するオブジェクトのメソッドを呼び出すようにします。(委譲します。)
(Articleの実装は変わりません)

User.kt
User(name: String, prefecture: String) {
    private val article: Article
    fun isInTokyo(): Boolean {
        return article.isInTokyo()
    }
}

3. 値オブジェクトでわかりやすく安全にする

極端な例ですが、以下のようなコードを見たことがあるかと思います。

基本データ型.kt
class Item {
    val price: Int
    val quantity: Int
}

これの何が問題かというと、上記の場合はprice, quantity共に許容する値が-21億~ +21億であるという宣言をしていることになる点です。
価格は一般的に考えると、適切な上限というものがあるはずです。
なので、それを表すMoneyクラスを定義し、それをItemクラスに持たせるべきです。
そうすることで、priceの処理をする時に必要になってくる異常値のチェックなどをMoneyクラスに委譲でき、Item内でのコードの見通しがすっきりします。

例えば、合計購入金額を求めるメソッドがあるとしてください。
さらに、それには以下のような制約があるものとします。

  1. 単価が-の場合はエラーを投げる。
  2. 単価が10,000を超える場合はエラーを出す。
基本データ型.kt
class Item(price: Int, quantity: Int) {
    private val price: Int
    private val quantity: Int

    init {
        this.price = price
        this.quantity = quantity
    }

    fun calculateTotalAmount(): Int {
        if (price < 0 || price > 100000) throw IllegalArgumentException()

        return price * quantity
    }
}

fun main(args: Array<String>) {
        val price = 10000000
        val item: Item = Item(price, 1)
        item.calculateTotalAmount()    
}

本当であれば、calculateTotalAmount()のやりたいことは一行で良いはずですが、その3倍かかってしまっています。
これは値チェックをしなければならなかったためです。
この値チェックをMoneyクラスに委譲してしまいます。

ValueObject.kt
class Item(price: Money, quantity: Int) {
    private val price: Money
    private val quantity: Int

    init {
        this.price = price
        this.quantity = quantity
    }

    fun calculateTotalAmount(): Int {
        return price.getPrice() * quantity
    }
}

class Money(price: Int) {
    private val price: Int

    init {
        if (price < 0 || price > 100000) throw IllegalArgumentException()

        this.price = price
    }

    fun getPrice(): Int {
        return price
    }
}

これでItem#calculateTotalAmount内は一行で収まり、余計な処理がなくなったので見通しが良くなりました。
オブジェクトの処理の責任としても、金額のチェックはMoneyクラスがやったほうが論理的に納得行きやすいですね。

4. コレクションオブジェクトで、複雑なロジックを集約して整理する

明日書く!

所感

Kotlinで書くのはすごく時間かかりました。。。
最初は仕方ないのかな。
新しい言語と新しい情報っていうのだとかなり負荷がかかってやになっちゃうきがするので、適度なバランスでやって行きます。