Rubyワーストプラクティスの問題点と解決策

Rubyベストプラクティスを読みました。
Rubyワーストプラクティスの問題点と解決策」という観点から学んだことをまとめようと思います。

サブクラス化を許すクラスに、クラス変数を使う

問題点

想定していない定義や変更が起きる

class Car
  @@instances = []

  def self.count
    @@instances.count
  end

  def make
    @@instances << self
  end
end

class Foo < Car
end

class Bar < Car
end

foocar = Foo.new
foocar.make
Foo.count #=> 1
Bar.count #=> 1

解決策

クラス変数をクラスインスタンス変数に変更する

class Car
  def self.instances
    @instances ||= []
  end

  def self.count
    instances.count
  end

  def make
    self.class.instances << self
  end
end

foocar = Foo.new
foocar.make
Foo.count #=> 1
Bar.count #=> 0

クラス名をハードコードする

問題点

クラス名をリネームしたい際、すべてのクラスメソッドを書き換える必要が発生する

class A
  def A.foo
    # ..
  end

  def A.bar
    # ..
  end
end

解決策

selfを使用する

class A
  def self.foo
    # ..
  end

  def self.bar
    # ..
  end
end

通常の継承を使って、分野横断的な関心事をモデル化しようとする

問題点

Rubyは単一継承のみをサポートしているため、通常の継承を使うだけでは、分野横断的な関心事をモデル化できない。
下記の例では、Barのみrepairできるようにしたいが、継承を使うだけでは実現できない。

class Car
  def self.instances
    @instances ||= []
  end

  def self.count
    instances.count
  end

  def make
    self.class.instances << self
  end
end

class Foo < Car
end

class Bar < Car
end

class Repairer
  class << self
    def repaired_instances
      @repaired_instances ||= []
    end

    def repaired_count
      repaired_instances.count
    end
  end

  def repair
    self.class.repaired_instances << self
  end
end

解決策

モジュールを定義し、インクルードする

class Foo < Car
end

class Bar < Car
  include Repairer
end

module Repairer
  module ClassMethods
    def repaired_instances
      @repaired_instances ||= []
    end

    def repaired_count
      repaired_instances.count
    end
  end

  def repair
    self.class.repaired_instances << self
  end

  def self.included(base)
    base.extend ClassMethods
  end
end

barcar = Bar.new
barcar.repair
Bar.repaired_count #=> 1
Foo.new.repair #=> NoMethodError: undefined method `repair' for #<Foo:0x007fdd6be21a58>
Foo.repaired_count #=> NoMethodError: undefined method `repaired_count' for Foo:Class

文字列に対してeval()を使う

問題点

任意のシステムコマンドを実行したり、オブジェクト内部を破壊したりできる

class Car
  attr_reader :name, :price

  def initialize(name, options)
    @name = name
    @price = options[:price]
  end
end

class Filter
  def initialize(enum)
    @collection = enum
  end

  def search(query)
    @collection.select { |e| e.instance_eval(query) }
  end
end

car1 = Car.new("light", price: 800_000)
car2 = Car.new("mini-ban", price: 2_000_000)
car3 = Car.new("sedan", price: 2_500_000)

f = Filter.new([car1, car2, car3])
f.search("price >= 2_000_000") #=> [car2, car3]

f.search("@price = 0")
#=> [#<Foo:0x007ffb84dc9c90 @name="light", @price=0>, 
     #<Foo:0x007ffb84a58d18 @name="mini-ban", @price=0>, 
     #<Foo:0x007ffb84ebb130 @name="sedan", @price=0>] 

解決策

Rubyが提供しているツールを使用する。以下の例では、public_sendを使用。

class Filter
  def initialize(enum)
    @collection = enum
  end

  def search(query)
    data = query.match(/^(?<attr>\w+) (?<op>[><!]=?|==) (?<val>[\d_]+)$/)
    raise "invalid query" if data.blank?

    @collection.select do |e|
      attr = e.public_send(data[:attr])
      attr.public_send(data[:op], Integer(data[:val]))
    end
  end
end

car1 = Car.new("light", price: 800_000)
car2 = Car.new("mini-ban", price: 2_000_000)
car3 = Car.new("sedan", price: 2_500_000)

f = Filter.new([car1, car2, car3])
f.search("price >= 2_000_000") #=> [car2, car3]

f.search("@price = 0") #=> RuntimeError: invalid query
f.search("price == 0\n@price = 0") #=> []
car1.price #=> 800000

無条件のrescueを使う

問題点

アプリケーションが黙って失敗する原因になる

class Car
  attr_reader :name, :price

  def initialize(name, options={})
    @name = name
    @price = options[:price]
  end

  def price_including_tax
    pric * 1.08 # 意図的にtypoを入れる
  end

  def formatted_price
    price_including_tax.to_s.reverse.gsub(/(\d{3})(?=\d)/, '\1,').reverse rescue "0"
  end
end

car1 = Car.new("light")
car1.formatted_price #=> "0"
car2 = Car.new("sedan", price: 2_500_000)
car2.formatted_price #=> "0"

解決策

  • 無条件のraiseと組み合わせて使う。
  • リスクを伴うことを知って使うか、無条件のrescueを使用しない。できれば後者である方が望ましい。
Class Car
  attr_reader :name, :price

  def initialize(name, options={})
    @name = name
    @price = options[:price]
  end

  def price_including_tax
    return 0 unless price
    pric * 1.08 # 意図的にtypoを入れる
  end

  def formatted_price
    price_including_tax.to_s.reverse.gsub(/(\d{3})(?=\d)/, '\1,').reverse
  end
end

car1 = Car.new("light")
car1.formatted_price #=> "0"
car2 = Car.new("sedan", price: 2_500_000)
car2.formatted_price #=> NameError: undefined local variable or method `pric' for #<Car:0x007f97ccadaca8 @name="sedan", @price=2500000>

元のObject定義へ全ての情報を委譲しないmethod_missingフックを使う

問題点1

オブジェクトが実際に処理する必要のないメッセージに対しても、エラーが出なくなる

class Car
  def self.instances_normal
    @instances_normal ||= {}
  end

  def self.instances_with_deadline
    @instances_with_deadline ||= {}
  end

  def method_missing(id, *args, &block)
    case id.to_s
    when /^make_(.*)/
      self.class.send("instances_#{$1}")[self] = args[0]
      self
    end
  end
end

car = Car.new
car.make_with_deadline(Date.today + 7.day) #=> #<Car:0x007fd172d329a8> 
car.invalid_method #=> nil

問題点2

継承チェーンの上部にあるものを壊してしまう

class Car
  def self.instances_normal
    @instances_normal ||= {}
  end

  def self.instances_with_deadline
    @instances_with_deadline ||= {}
  end

  def method_missing(id, *args, &block)
    case id.to_s
    when /^make_(.*)/
      self.class.send("instances_#{$1}")[self] = args[0]
      self
    end
  end
end

class Bar < Car
  include Repairer
end

module Repairer
  module ClassMethods
    def repaired_all_instances
      @repaired_all_instances ||= []
    end

    def repaired_tire_instances
      @repaired_tire_instances ||= []
    end
  end

  def self.included(base)
    base.extend ClassMethods
  end

  def method_missing(id)
    case id.to_s
    when /^repair_(.*)/
      self.class.send("repaired_#{$1}_instances") << self
    else
      super
    end
  end
end

barcar = Bar.new
barcar.make_with_deadline(Date.today + 7.day) #=> ArgumentError: wrong number of arguments (2 for 1)

解決策

  • method_missing()呼び出しの内部では必ずsuperを呼び出す
  • 元のオブジェクトに委譲するだけでなく、情報を失うことなく委譲する
class Car
  def self.instances_normal
    @instances_normal ||= {}
  end

  def self.instances_with_deadline
    @instances_with_deadline ||= {}
  end

  def method_missing(id, *args, &block)
    case id.to_s
    when /^make_(.*)/
      self.class.send("instances_#{$1}")[self] = args[0]
      self
    else
      super
    end
  end
end

class Bar < Car
  include Repairer
end

module Repairer
  module ClassMethods
    def repaired_all_instances
      @repaired_all_instances ||= []
    end

    def repaired_tire_instances
      @repaired_tire_instances ||= []
    end
  end

  def self.included(base)
    base.extend ClassMethods
  end

  def method_missing(id, *args, &block)
    case id.to_s
    when /^repair_(.*)/
      self.class.send("repaired_#{$1}_instances") << self
      self
    else
      super
    end
  end
end

barcar = Bar.new
barcar.make_with_deadline(Date.today + 7.day) #=> #<Bar:0x007fef3d4516c0> 
barcar.invalid_method #=> NoMethodError: undefined method `invalid_method' for #<Bar:0x007fef3d4516c0>

Rubyベストプラクティス -プロフェッショナルによるコードとテクニック

Rubyベストプラクティス -プロフェッショナルによるコードとテクニック