Как бы вы изменили этот "сложный метод" для кодеклимата?

Я пытаюсь выяснить, как выполнить рефакторинг некоторого кода, основываясь на том, что говорит мне кодеклимат, пока что кажется, что любой метод, который длиннее 5 строк или имеет оператор if, является "сложным". В этом случае я пытаюсь создать событие для календаря на основе записи о собрании, вот модель события:

class Event < ActiveRecord::Base
  validates :title, :eventcolor_id, presence: true
  belongs_to :meeting
  belongs_to :eventcolor

  def self.create_from_meeting(meeting, eventcolor)
    meeting.meeting_date_end ||= meeting.meeting_date_start
    Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
      e.title = meeting.title
      e.datetime_start = Time.zone.local_to_utc(meeting.meeting_date_start)
      e.datetime_end = Time.zone.local_to_utc(meeting.meeting_date_end)
      e.location = meeting.location
      e.address1 = meeting.address1
      e.address2 = meeting.address2
      e.city = meeting.city
      e.state = meeting.state
      e.zip = meeting.zip
      e.description = (!meeting.description.blank?) ? meeting.description : ''
      e.eventcolor_id = eventcolor
      e.save
      e.committees << meeting.committee if !meeting.committee.nil?
    end
  end
end

который вызывается из моего meeting_controller с этой строкой:

Event.create_from_meeting(meeting, params[:eventcolors_select])
Источник

2 ответа

Решение

Зачем вам нужно дублировать все эти Meeting атрибуты в Event? У тебя есть belongs_to отношения здесь, так что, кажется, нет необходимости в таком дублировании. Да, загрузка meeting вместе с event будет включать в себя join введите запрос, но я уверен, что выигрыш в производительности при дублировании этих данных близок к нулю.

Вместо этого я бы предложил сделать что-то вроде этого:

class Event < ActiveRecord::Base
  validates :eventcolor_id, presence: true
  belongs_to :meeting
  belongs_to :eventcolor

  # if you need to access these attributes on event directly
  delegate :title, :location, :etc, to: :meeting, prefix: false

  # note: this could probably be improved further, but we don't know much about 
  # your committees association, so I have left it as is
  def self.create_from_meeting(meeting, eventcolor)
    Event.find_or_initialize_by(meeting_id: meeting.id) do |e|
      e.eventcolor_id = eventcolor
      e.save
      e.committees << meeting.committee if !meeting.committee.nil?
    end
  end

  # for the items that require some manipulation, create custom methods
  # these are pretty cheap manipulations, but you can cache them if you like
  def datetime_start
    @datetime_start ||= Time.zone.local_to_utc(meeting.meeting_date_start)
  end

  def description
    meeting.description || ''
  end

end

AR будет кэшировать объект встречи по событию при первой загрузке. Если вас действительно беспокоит снижение производительности, и если вы собираетесь получать доступ к атрибутам на основе собраний практически для каждого запроса, вы также можете установить (на Event):

default_scope { includes(:meeting) }
Источник

Функция должна иметь легко описываемое и четко определенное задание. Количество строк - плохая метрика для сложности imho. Возможно, вы могли бы наметить инициализацию структуры для метода с таким количеством параметров, но если бы это было более красиво...

Источник
Другие вопросы по тегам