Рефакторинг этого кода на Ruby и Rails

Question

У меня есть эта модель:

class Event < Registration
  serialize :fields, Hash
  Activities=['Annonce', 'Butiksaktivitet', 'Salgskonkurrence']

  CUSTOM_FIELDS=[:activity, :description, :date_from, :date_to, :budget_pieces, :budget_amount, :actual_pieces, :actual_amount]
  attr_accessor *CUSTOM_FIELDS

  before_save :gather_fields
  after_find :distribute_fields

  private

  def gather_fields
    self.fields={}
    CUSTOM_FIELDS.each do |cf|
      self.fields[cf]=eval("self.#{cf.to_s}")
    end
  end

  def distribute_fields
    unless self.fields.nil?
      self.fields.each do |k,v|
        eval("self.#{k.to_s}=v") 
      end
    end
  end
end

У меня такое ощущение, что это можно сделать короче и элегантнее. У кого-нибудь есть идея?

• Jacob

КСТАТИ. Может кто-нибудь сказать мне, что делает звездочка перед CUSTOM_FIELDS? Я знаю, что он делает в определении метода (def foo (* args)), но не здесь ...

Answer 1

Хорошо, сначала: никогда 10000000000.times { puts "ever" } используйте eval, когда вы не знаете, что делаете.Это ядерная бомба мира Ruby, способная нанести ущерб обширной области, вызывая сходные симптомы радиационного отравления во всем коде.Только не надо.

Имея это в виду:

class Event < Registration
  serialize :fields, Hash
  Activities = ['Annonce', 'Butiksaktivitet', 'Salgskonkurrence']

  CUSTOM_FIELDS = [:activity, 
                 :description,
                 :date_from,
                 :date_to,
                 :budget_pieces,
                 :budget_amount,
                 :actual_pieces,
                 :actual_amount] #1
  attr_accessor *CUSTOM_FIELDS #2

  before_save :gather_fields
  after_find :distribute_fields

  private

  def gather_fields
    CUSTOM_FIELDS.each do |cf|
      self.fields[cf] = send(cf) #3
    end
  end

  def distribute_fields
    unless self.fields.empty?
      self.fields.each do |k,v|
        send("#{k.to_s}=", v) #3
      end
    end
  end
end

Теперь о некоторых заметках:

1. Поместив каждое настраиваемое поле в отдельную строку, выповысить читаемость кода.Я не хочу прокручивать до конца строки, чтобы прочитать все возможные настраиваемые поля или добавить свое собственное.
2. *CUSTOM_FIELDS, переданный в attr_accessor, использует то, что называется"оператор сплат".Вызывая его таким образом, элементы массива CUSTOM_FIELDS будут передаваться как отдельные аргументы методу attr_accessor, а не как один (сам массив)
3. Наконец, мы используем sendметод для вызова методов, имена которых мы не знаем во время программирования, а не зло eval.

Кроме этого, я не могу найти что-либо еще для рефакторинга об этом коде.

Answer 2

Я согласен с предыдущими постерами. Кроме того, я бы, возможно, переместил методы collect_fields и distribte_fields в родительскую модель, чтобы избежать необходимости повторять код в каждой дочерней модели.

class Registration < ActiveRecord::Base
  ...

protected
  def gather_fields(custom_fields)
    custom_fields.each do |cf|
      self.fields[cf] = send(cf)
    end
  end

  def distribute_fields
    unless self.fields.empty?
      self.fields.each do |k,v|
        send("#{k.to_s}=", v)
      end
    end
  end
end

class Event < Registration
  ...

  before_save :gather_fields
  after_find :distribute_fields

private
  def gather_fields(custom_fields = CUSTOM_FIELDS)
    super
  end
end

Answer 3

Для этой звездочки * посмотрите этот пост: Для чего нужен оператор splat / унарный / звездочка?

Activities=['Annonce', 'Butiksaktivitet', 'Salgskonkurrence']

можно записать: ACTIVITIES = %w(Annonce, Butiksaktivitet, Salgskonkurrence).freeze, поскольку вы определяете константу.

def distribute_fields
  unless self.fields.empty?
    self.fields.each do |k,v|
      send("#{k.to_s}=", v) #3
    end
  end
end

может быть записано в виде одной строки:

def distribute_fields 
  self.fields.each { |k,v| send("#{k.to_s}=", v) } unless self.fields.empty?
end

Райан Бигг, дал хороший ответ.

Answer 4

Вы можете заменить два зла на отправку звонков:

self.fields[cf] = self.send(cf.to_s)


self.send("#{k}=", v)

"# {}" делает to_s, поэтому вам не нужно k.to_s

Деятельность, будучи постоянной, вероятно, должна быть ДЕЯТЕЛЬНОСТЬЮ.