現状(要件)

reek は Rubyプログラムのコードの臭いを検知するツールとして、弊社の開発でも活用しています。

その reek が発する警告の一つに Repeated Conditional というものがありますが、以下のように「一つのクラス内で、同じ条件判定を繰り返し定義している場合」に起こるものとされています。

# https://github.com/troessner/reek/blob/master/docs/Repeated-Conditional.md から引用
class RepeatedConditionals
  attr_accessor :switch

  def repeat_1
    puts "Repeat 1!" if switch
  end

  def repeat_2
    puts "Repeat 2!" if switch
  end

  def repeat_3
    puts "Repeat 3!" if switch
  end
end

この警告の理由、および対策について同ページでは以下のように説明しています。

If you get this warning then you are probably not using the right abstraction or even more probable, missing an additional abstraction.

(DeepL による翻訳)

この警告が表示された場合は、正しい抽象化を使用していないか、さらには追加の抽象化を見逃している可能性があります。

ただ、これだけの説明だと「具体的にはどう解決すれば良いのか?」という疑問が生じます。

そこで、上記の例を「実際に解決する方法」を考えてみます。

1. まずは「抽象クラス」を切り出す

説明文には「抽象化を使用していない、または足りない」とあります。

「抽象化」の方法としてすぐに考えられるのは「継承を用いる方法」です。

上述の例の RepeatedConditionals クラスに、抽象クラスとして AbstractConditionals クラスを追加してみましょう。

# まずは単純に抽象クラスを追加します
class AbstractConditionals
  attr_accessor :switch

  def repeat_1
    raise NotImplementedError
  end

  def repeat_2
    raise NotImplementedError
  end

  def repeat_3
    raise NotImplementedError
  end
end

# 抽象クラスを継承した具象クラスとする
class RepeatedConditionals < AbstractConditionals
  def repeat_1
    puts "Repeat 1!" if switch
  end

  def repeat_2
    puts "Repeat 2!" if switch
  end

  def repeat_3
    puts "Repeat 3!" if switch
  end
end

後の説明のために「この実装の使い方」も載せます

conditionals = RepeatedConditionals.new
conditionals.switch = true
conditionals.repeat_1

conditionals.switch = false
conditionals.repeat_2

まだこの時点では reek の Repeated Conditional 警告が出されたままです。

ではもう一歩、警告解消に向けて進むにはどうすれば良いでしょうか。

2. 具象クラスを(条件結果ごとに)分割する

警告解消のためには、これまでの説明通り「同じ条件判定を繰り返し定義している」のを解消すれば良いのですから、条件判定を「メソッド内」で行わずに「クラス単位」、すなわち「インスタンス生成時点」で行うようにしてみます。

そのためには switch が真の場合のクラスと、偽の場合のクラスに分ければ良いことになります。

# switch というアクセサ自体が無くなっています
class AbstractConditionals
  def repeat_1
    raise NotImplementedError
  end

  def repeat_2
    raise NotImplementedError
  end

  def repeat_3
    raise NotImplementedError
  end
end

# こちらは「swith が真だった場合の動作」を表すクラスです
# switch が真であるという前提なので、 `if switch` が無くなっています
class SwitchedOnConditionals < AbstractConditionals
  def repeat_1
    puts "Repeat 1!"
  end

  def repeat_2
    puts "Repeat 2!"
  end

  def repeat_3
    puts "Repeat 3!"
  end
end

# こちらは「swith が偽だった場合の動作」を表すクラスです
class SwitchedOffConditionals < AbstractConditionals
  def repeat_1; end

  def repeat_2; end

  def repeat_3; end
end

この時点で、reek の Repeated Conditional 警告は解消されます。

しかし、この対処の場合、インスタンスを生成する時点で「どちらのクラスを使うのか」を決定する必要が有るので、以下のような使い方になります。

switch = true
conditionals = (switch ? SwitchedOnConditionals : SwitchedOffConditionals).new
conditionals.repeat_1

switch = false
conditionals = (switch ? SwitchedOnConditionals : SwitchedOffConditionals).new
conditionals.repeat_2

これはベタに言えば「イケてない」感じがヒシヒシとします。

なぜなら、使用する側にとっては「どのクラスからインスタンスを生成したか」が関心事なのではなく、「switch の値に応じて正しい結果が返されるインスタンスである」ことだけが関心事だと考えられるからです。

3. どのクラスのインスタンスが生成されるかを隠蔽する

とすれば、ここで思い出されるのが「GoFによるデザインパターン」の一つである Factory Method パターンです。

このパターンでは、何らかの方法で「具体的にどのクラスのインスタンスが生成されるのか」を隠蔽します。

今回は「生成メソッドの引数に switch 引数を与える」ことで、適切なクラスのインスタンスが生成されるようにします。

# 生成メソッドをクラスメソッドで定義するため、Abstract の prefix を取り止め、
# 実際に呼ばれることを目的としたクラス名とする
class Conditionals
  class << self
    def build(switch)
      (switch ? SwitchedOnConditionals : SwitchedOffConditionals).new
    end
  end

  def repeat_1
    raise NotImplementedError
  end

  def repeat_2
    raise NotImplementedError
  end

  def repeat_3
    raise NotImplementedError
  end
end

# 以下はこれまでと同じですが、後述のコードと比較しやすいように再度全体を載せます
class SwitchedOnConditionals < Conditionals
  def repeat_1
    puts "Repeat 1!"
  end

  def repeat_2
    puts "Repeat 2!"
  end

  def repeat_3
    puts "Repeat 3!"
  end
end

class SwitchedOffConditionals < Conditionals
  def repeat_1; end

  def repeat_2; end

  def repeat_3; end
end

使い方は以下のようになります。

conditionals = Conditionals.build(true)
conditionals.repeat_1

conditionals = Conditionals.build(false)
conditionals.repeat_2

随分と簡単になりました。

ちょっと廻り道をしたような形になりましたが、しかし結果的にこれで

reek の Repeated Conditional 警告を解消し、
かつ、使い方はこれまでとほぼ同じ(switch の値を使うのがインスタンス生成時になっただけ)

という、ほぼ理想に近い状況になったことと思います。

4. (さらに考えて)インスタンス生成時に条件判定が出来ない場合は?

これまでの記事では

インスタンス生成時に条件判定をすることによって、メソッド実行時の条件判定を省ける

ということを書いてきましたが、必ずしも「インスタンス生成時に条件判定が出来る」とは限らないと思います。

この記事の最初の例(Repeated Conditional 警告の説明ページの例)に立ち返って考えると、たとえば

conditionals = Conditionals.new
conditionals.switch = true
conditionals.repeat_1

conditionals.switch = false
conditionals.repeat_2

のように、インスタンスの生成後に条件判定が変わる場合が有ります。これでは「インスタンスの属するクラスを分ける」対処が出来ません。

これの対処として考えられることは「委譲を使う」ことになります。

すなわち、条件判定を要するメソッドを別クラスに分離して、switch の値が切り替わるごとに差し替えてしまうことです。

以下にコード例を示します。

class Conditionals
  attr_reader :switch

  def switch=(value)
    @repeater = value ? SwitchedOnRepeater.new : SwitchedOffRepeater.new
    @switch   = value
  end

  # 実際は `activesupport` gem の delegate で実装する方が簡単です
  # ```ruby
  # delegate :repeat_1, to: :repeater
  # ```
  # https://api.rubyonrails.org/classes/Module.html#method-i-delegate
  def repeat_1
    repeater.repeat_1
  end

  def repeat_2
    repeater.repeat_2
  end

  def repeat_3
    repeater.repeat_3
  end

  private

  attr_reader :repeater
end

# 以下は「親クラスが Conditionals になっていない」だけで、これまでの例と同じです
class SwitchedOnRepeater
  def repeat_1
    puts "Repeat 1!"
  end

  def repeat_2
    puts "Repeat 2!"
  end

  def repeat_3
    puts "Repeat 3!"
  end
end

class SwitchedOffRepeater
  def repeat_1; end

  def repeat_2; end

  def repeat_3; end
end

まとめ

reek の Repeated Conditional 警告を解消するには

具象クラスを作成するなど「継承」を用いるのが良い
あるいは、その部分を「委譲」するのが良い

といったことを書きました。

他にも reek の警告には「説明を読んでも、どう対処したら良いのかすぐには分からない」ものがありますので、何か解決策が見つかれば、その都度、記していきたいと思います。