SIerで仕事やっていてよく見るコードと直し方 - デメテルの法則違反

仕事やっていてよく見るアンチパターンをまとめていこうと思ってます。今回はデメテルの法則違反です。
コードはScalaですが、RubyやPythonやJavaでも基本同じです。

トピックは以下。

• 違反したコード
• とりあえず直す
• とりあえず直したあとのテスト

デメテルの法則に違反したコード

デメテルの法則とは、「直接インスタンス化したもの」か「引数として渡されたもの」以外のものを使ってはいけないという法則です。

class Profile(name: String, age: Int) {
  def showProfile(): Unit = println(s"my name is $name (age: $age)")
}

class Applicant(id: String, val profile: Profile)

object Order {
  // デメテルの法則に違反している
  def showApplicant(applicant: Applicant): Unit = {
    applicant.profile.showProfile()
  }
}

引数でapplicantが渡されているけど、メソッド内では、
applicant.profileでprofileを取得してからshowProfile()メソッドを実行しているので違反しています。

applicant.profile.showProfile()がやりたいがために、
class Applicant(id: String, val profile: Profile)のようにvalをつけてゲッターでプロパティを公開するのはいったん躊躇したほうがいいと思う。

呼び出したい形としては、以下。

  def showApplicant(applicant: Applicant): Unit = {
    applicant.showProfile()
  }

ここでは、以下について記載している。

• 継承による解決方法
• 転送メソッド(委譲)による解決法

継承による解決方法

class Profile(name: String, age: Int) {
  def showProfile(): Unit = println(s"my name is $name (age: $age)")
}

// ApplicantがProfileを継承する
class Applicant(id: String, name: String, age: Int) extends Profile(name, age)

object Order {
  def showApplicant(applicant: Applicant): Unit = {
    applicant.showProfile()
  }
}

↑だといろいろ違和感あるので、転送メソッドで修正する。

転送メソッド(委譲)による解決方法

class Profile(name: String, age: Int) {
  def showProfile(): Unit = println(s"my name is $name (age: $age)")
}

// profileはゲッターで公開する必要がなくなったので
// valを外した。
class Applicant(id: String, profile: Profile) {
  // 転送メソッド
  def showProfile(): Unit = profile.showProfile()
}

object Order {
  def showApplicant(applicant: Applicant): Unit = {
    applicant.showProfile()
  }
}

テストついて

ここから、転送メソッドで修正したOrder.showApplicant()メソッドのテストについて考える。

object SimpleTest extends App {
  // 必要なオブジェクトを作る
  val profile = new Profile("taro", 22)
  val applicant = new Applicant("001", profile)
  
  // テスト対象のメソッドを実行する
  Order.showApplicant(applicant)
}

上の例だと、Applicantインスタンスの生成は簡単だけど、例えば、下のように、Applicantが様々なクラスを集約していてインスタンス生成が億劫な場合を考える。

大事なことは、テスト対象がOrder.showApplicant()メソッドであり、転送メソッドなので、呼んだことを確認できればいい。ということ。

object ComplexTest extends App {
  // インスタンスの生成が複雑
  val profile1 = ...
  val profile2 = ...
  val profile3 = ...
  val applicant = new Applicant("001", profile1, profile2, profile3, ...)

  // テスト対象のメソッドを実行したいだけだが、
  // インスタンス生成が億劫。
  Order.showApplicant(applicant)
}

テストを簡単にできるようにするためにリファクタリング

Order.showApplicant()メソッドを実行出来ればいいので、引数のapplicantの中身は何でもよいものとして考える。
なので、インタフェースを挟み、Order.showApplicant()が抽象に依存するようにする。

// 抽象化しておく
trait IApplicant {
  def showProfile(): Unit
}

class Applicant(id: String, profile: Profile) extends IApplicant {
  def showProfile(): Unit = profile.showProfile()
}

object Order {
  // applicantの型をIApplicantとして抽象に依存するようにした。
  def showApplicant(applicant: IApplicant): Unit = {
    applicant.showProfile()
  }
}

Order.showApplicant()メソッドの引数に必要なapplicantは以下のように生成できるので、テストも多少は楽になる。

object RefactorComplexTest extends App {
  // インスタンスの生成が複雑
  val applicant = new IApplicant {
    override def showProfile(): Unit = println("てきとう。てきとう。てきとう")
  }
  
  Order.showApplicant(applicant)
}

以下のようにデメテルの法則に違反したままだと、テストがやりづらいので、デメテルの法則違反があったら直したほうがいいと個人的には思う次第です。

モックを使うなどすればテストはできますが、モックを使う前に設計を見直すのがいいと 個人的には思う次第です。

  def showApplicant(applicant: Applicant): Unit = {
    // デメテルの法則違反したままだとテストがやりづらい
    applicant.profile.showProfile()
  }