code smellリファクタリング[feature envy編]

第三回のリファクタリングは、feature envyです。

feature envyとは

あるモジュールの関数が、内部のモジュールよりも、外部のモジュールの関数やデータ構造とやりとりしているということ。

例えば、よくある例としては、他のモジュールのgetメソッドをチェーンして使っている場合など。

悪いコード例

例えば、以下のコードを見てください。

Houseクラスから、Personクラス Adressクラスを辿って、AdressクラスcalcurateDistanceメソッドを呼び出しています。

明らかに、Houseクラスは他のクラスを知りすぎており、参照しすぎています。

House().getPerson().getAdress().calcurateDistance()

なぜ悪いのか

では、そもそもなぜこのような過度の外部モジュールへの参照が悪いのでしょうか?

そもそも良い設計をする時に、内部モジュールとよくやりとりし、外部モジュールとのやりとりは最小限にすることが良いとされています。

こうすることで、

処理をまとめることで凝集度が上がり、変更しても影響が少なくなる。

ソリューション

主に考えられるソリューションは以下の二つがあります。

  • 関数の移動

  • フィールド値の移動

では実際に、コード例を見ていきます。

このコード例では、四つのモデルがあります。 House Price Address SalesPerson

家の値段を計算したり、家までの距離を計算したりするプログラムだと仮定します。

リファクタリング

class Main {
  def main(args: Array[String]): Unit = {
    val address = Address(1L, 1L, 100L)
    val price = Price(10000)
    val house = House(address, price)
    val salesPerson = SalesPerson(1, "kin", house)

    salesPerson.calculatePrice

  }

  case class House(address: Address, price: Price)

  case class Price(number: Int)

  case class Address(
                      longitude: Long,
                      latitude: Long,
                      price: Long
                    )


  case class SalesPerson(
                          id: Int,
                          name: String,
                          house: House
                        ) {
    def calculatePrice: Int = {
      (house.price.number * 1.1).toInt
    }

    def calculateDistance: Long = {
      house.address.longitude
    }
  }
 
}

SalesPersonクラスにsmellがありそうですね。

よく見てみると、calculatePriceメソッドとcalculateDistanceメソッドの両方が、houseクラスのフィールド値を参照しています。

典型的なfeature envy smellですね。つまり、自分のフィールド値よりも外部モジュールのフィールド値をよく参照しています。

これを、リファクタリングしていきます。

この場合のリファクタリングには、フィールド値の移動・関数の移動のどちらが適しているでしょうか。

フィールド値の移動を行う、

case class House() {
  }

  case class SalesPerson(id: Int, name: String, house: House, address: Address, price: Price) {
    def calculatePrice: Int = {
      (price.number * 1.1).toInt
    }

    def calculateDistance: Long = {
      address.longitude
    }
  }

二つの関数内のsmellは無くなったかもしれませんが、modelの凝集度が破綻していますね。

SlaesPersonクラスが、address priceなどの関係のないデータを持つようになってしまいます。

これは、素直に関数の移動をしてあげましょう。

リファクタリング


def main(args: Array[String]): Unit = {
    val address = Address(1L, 1L, 100L)
    val price = Price(10000)
    val house = House(address, price)
    val salesPerson = SalesPerson(1, "kin", house)

//    salesPerson.calculatePrice
    house.calculatePrice

    house.calculateDistance
    
  }

  case class House(address: Address, price: Price) {
    def calculatePrice: Int = {
      (price.number * 1.1).toInt
    }

    def calculateDistance: Long = {
      address.longitude
    }
  }

  case class SalesPerson(
                          id: Int,
                          name: String,
                          house: House
                        )

二つの関数をHouseクラスに移動させることで、feature envy smellは消え、さらにHouseクラスの振る舞いも増えて凝集度が高まりました。

まとめ

僕の感覚として、 隣のさらにまた隣のモジュールのデータにまでアクセスしようとしているなら、それはfeature envyだと疑った方がいいかと思います。

上記のリファクタリングのように、関数を定義する場所が違うのか、それとも、オブジェクトのフィールド値を移動させた方がいいかと。

最後までありがとうございました。

次回は、lazy classに関しての記事を書きます。