ruby on rails - ¿Cómo ordenarías esta lógica de controlador?

ruby-on-rails refactoring

Question

Tengo un poco de lógica en un controlador que establece el estado de un objeto si se cumplen ciertas condiciones:

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

que comparto entre crear y actualizar acciones. ¿Cómo harías para refaccionar esta maldad? Buscando las mejores prácticas.

Nuevo en la programación y dispuesto a limpiar mi código.

TIA.

Answer 1

Desglosarlo en declaraciones if anidadas.

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 2

Eso parece ser lógica de negocios, por lo que debería estar en el modelo realmente.

Es probable que su modelo necesite un par de atributos: consulted_legal y consulta_marketing, y un método para establecer el estado cuando cualquiera de ellos se modifique de la siguiente manera:

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

Esta es mi opinión sobre ella. Yo lo llamo 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]}

Alternativamente, un enfoque más convencional: extenso pero legible:

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}

Nota : parece que su código original está verificando los valores de las casillas de verificación. Los valores en hash params son siempre Strings , no Fixnum s, por lo que mi código compara cadenas. Si por alguna razón comparar lo de Fixnum es lo que se requiere para su situación, solo Fixnum las comillas de los números.

Answer 4

Esto parece una regla de negocios para mí. Como tal, es posible que desee envolverlo en una función:

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";
    }
}

Esto también separa los detalles de cómo se codifican los booleanos en su interfaz a partir de la implementación real de la regla de negocios.

Pero la estructura real del código me parece buena, ya que probablemente modele mejor la regla comercial.

Answer 5

Puede hacer que su código sea menos dependiente de ambas condiciones y hacerlo mucho más flexible.

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}

Versión para crear y actualizar sin filtro:

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

O con un before_filter:

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

def create
  ...
end

def update
  ...
end

Si puede moverlo a su vista, se ve aún mejor:

module ConceptHelper
  def get_concept_status
    ...
  end
end

<%= get_concept_status %>

Answer 6

puede pensar que la lista de departamentos consultados es un concepto suficientemente fijo para justificar las variables llamadas consulta_marketing, etc., pero para el crecimiento y la sequedad (en cierto modo), prefiero una lista de departamentos.

Lo que realmente tiene aquí es un flujo de trabajo o máquina de estados, creo que una lista de departamentos con transiciones daría como resultado el código más limpio y creíble.

¡El código es información! Modele sus datos y el código será trivial.