Как бы вы привести в порядок эту логику контроллера?

Question

У меня есть некоторая логика в контроллере, который устанавливает состояние объекта при соблюдении определенных условий:

if params[:concept][:consulted_legal] == 0 && params[:concept][:consulted_marketing] == 1
  @concept.attributes = {:status => 'Awaiting Compliance Approval'}
elsif params[:concept][:consulted_marketing] == 0 && params[:concept][:consulted_legal] == 1 
  @concept.attributes = {:status => 'Awaiting Marketing Approval'}
elsif params[:concept][:consulted_marketing] == 0 && params[:concept][:consulted_legal] == 0
  @concept.attributes = {:status => 'Awaiting Marketing & Legal Approval'}
else
  @concept.attributes = {:status => 'Pending Approval'}
end

, который я разделяю между действиями создания и обновления. Как бы вы занялись рефакторингом этой гадости? В поисках лучших практик.

Новичок в программировании и стремится очистить мой код.

ТИА.

Answer 1

Вы можете сделать свой код менее зависимым от обоих условий и сделать его намного более гибким.

waiting_on = []
waiting_on << 'Compliance' unless params[:concept][:consulted_marketing]
waiting_on << 'Legal' unless params[:concept][:consulted_legal]
status = waiting_on.empty? ? "Awaiting #{waiting_on.join(' & ')} Approval" : 'Pending Approval'
@concept.attributes = {:status => status}

Версия для создания и обновления без фильтра:

def create
  set_concept_status_attribute
  ...
end

def update
  set_concept_status_attribute
  ...
end

private
  def set_concept_status_attribute
    waiting_on = []
    waiting_on << 'Compliance' unless params[:concept][:consulted_marketing]
    waiting_on << 'Legal' unless params[:concept][:consulted_legal]
    status = waiting_on.empty? ? "Awaiting #{waiting_on.join(' & ')} Approval" : 'Pending Approval'
    @concept.attributes = {:status => status}
  end

Или с фильтром before_:

before_filter :set_concept_status_attribute, :only => [:create, :update]

def create
  ...
end

def update
  ...
end

Если вы можете переместить его в вид, он выглядит еще лучше:

module ConceptHelper
  def get_concept_status
    ...
  end
end

<%= get_concept_status %>

Answer 2

Это выглядит как бизнес-логика, поэтому она должна быть в модели действительно.

Ваша модель, вероятно, нуждается в паре атрибутов: consulted_legal и consulted_marketing, а также метод для установки статуса, когда один из них изменяется, что-то вроде этого:

before_update :set_status

def set_status
  if consulted_legal && consulted_marketing
    status = "Pending Approval"
  elif consulted_legal && !consulted_marketing
    status = "Awaiting Marketing Approval"
  elif !consulted_legal && consulted_marketing
    status = "Awaiting Legal Approval"
  elif !consulted_legal && !consulted_marketing
    status "Awaiting Marketing & Legal Approval"
  end

  true # Needs to return true for the update to go through
end

Answer 3

Это мое мнение. Я называю это Super DRY.

statuses = 
  [
    ['Awaiting Marketing & Legal Approval','Awaiting Compliance Approval'],
    ['Awaiting Marketing Approval','Pending Approval']
  ]

{:status => statuses[params[:concept][:consulted_legal].to_i][params[:concept][:consulted_marketing].to_i]}

В качестве альтернативы, более традиционный подход - длительный, но читаемый:

status = if params[:concept][:consulted_legal] == "0"
  if params[:concept][:consulted_marketing] == "1"
    'Awaiting Compliance Approval'
  else
    'Awaiting Marketing & Legal Approval'
  end
else
  if params[:concept][:consulted_marketing] == "0"
    'Awaiting Marketing Approval'
  else
    'Pending Approval'
  end
end

@concept.attributes = {:status => status}

Примечание : похоже, ваш исходный код проверяет значения флажков. Значения в хэше params всегда Strings, а не Fixnum с, поэтому мой код сравнивает строки. Если по какой-то причине сравнение Fixnum s - это то, что требуется для вашей ситуации, просто заключите в кавычки цифры.

Answer 4

Разбейте его на вложенные операторы if.

if params[:concept][:consulted_legal] == '0'
  if params[:concept][:consulted_marketing] == '1'
    @concept.attributes = { :status => 'Awaiting Compliance Approval' } 
  else
    @concept.attributes = { :status => 'Awaiting Marketing & Legal Approval' }
  end
else
  if params[:consulted_marketing] == '1'
    @concept.attributes = { :status => 'Awaiting Marketing Approval' }
  else
    @concept.attributes = { :status => "Pending Approval" }
  end
end

Answer 5

вы можете подумать, что список отделов, с которыми вы консультировались, является достаточно фиксированной концепцией, чтобы оправдать переменные с именем consulted_marketing и т. Д. Но для роста и сухости (в некотором смысле) я бы предпочел список отделов.

То, что у вас действительно есть, это рабочий процесс или конечный автомат, я думаю, что список отказов с переходами приведет к самому чистому, наиболее расширяемому коду.

Код - это данные! Смоделируйте ваши данные, и код будет тривиальным.

Answer 6

Для меня это выглядит как бизнес-правило. Таким образом, вы можете захотеть заключить его в функцию:

string GetConceptStatus(bool consulted_legal, bool consulted_marketing)
{ 
    if (consulted_legal && consulted_marketing) {
        return "Pending Approval";
    }
    if (consulted_legal && !consulted_marketing) {
        return "Awaiting Marketing Approval";
    }
    if (!consulted_legal && consulted_marketing) {
        return "Awaiting Legal Approval";
    }
    if (!consulted_legal && !consulted_marketing) {
        return "Awaiting Marketing & Legal Approval";
    }
}

Это также отделяет детали того, как bool s кодируются в вашем интерфейсе, от фактической реализации бизнес-правила.

Но фактическая структура кода выглядит хорошо для меня, поскольку она, вероятно, лучше моделирует бизнес-правило.